First box to call NVIC owns the interrupt #403

niklarm · 2017-01-26T17:15:29Z

This also copies the initial VTOR table from flash to ram and sets correct priority. That way, statically allocated interrupts are preserved and need only be enabled.

cc @AlessandroA @Patater

Patater

Use command tense in commit messages.

Consider replacing First box to call NVIC owns the interrupt with Make vIRQ functions claim unclaimed IRQs

Patater · 2017-01-30T10:29:20Z

core/system/src/unvic.c

-     * table is not 0 */
-    is_irqn_registered = uv->hdlr ? 1 : 0;
+    /* an IRQn slot is considered as registered if the vector id is not unused. */
+    is_irqn_registered = (uv->id != UNVIC_VECTOR_UNUSED);


We wouldn't need this check if, instead of UNVIC_VECTOR_UNUSED, we use an invalid box ID define (which we may need to create). Simply checking uv->id != g_active_box would be good enough.

Patater · 2017-01-30T10:42:00Z

core/system/src/unvic.c

    NVIC_SetPriorityGrouping(0);
+
+    /* Make sure the UNVIC_VECTOR_UNUSED is definitely NOT a valid box id. */
+    if (vmpu_is_box_id_valid(UNVIC_VECTOR_UNUSED)) {


We wouldn't have to do this check if UNVIC_VECTOR_UNUSED were replaced with an invalid box ID define.

Yup, this same mechanism is used in page allocator. I'll make it generic then.

Patater · 2017-01-30T10:53:31Z

core/system/src/unvic.c

+        g_unvic_vector[ii].id = UNVIC_VECTOR_UNUSED;
+        g_unvic_vector[ii].hdlr = (TIsrVector) user_vtor[ii + NVIC_OFFSET];
+        /* set default priority (SVC must always be higher) */
+        NVIC_SetPriority(ii, __UVISOR_NVIC_MIN_PRIORITY);


Is registering the user interrupt table interrupts with the public box really necessary? It sounds like a nice thing to do at first glance, but given that interrupts must be enabled before use, isn't just having enable (and the other vIRQ functions) register for the interrupt good enough? Is there some case I'm missing where an interrupt in the user interrupt table can be used without calling one of the NVIC or vIRQ functions?

If it is possible for a user to use an interrupt without calling any NVIC (or vIRQ) functions, how easy is it for a user to change the user interrupt table? Does a user need to change mbed OS code?

On the surface, I agree with this change. Interrupts in the user interrupt table are "legacy" interrupts and should be removed in favor of using the "non-legacy" vIRQ API. This fits with the uVisor story well of: the public box is for backwards compatibility; to do things with secure boxes you need to use uVisor APIs. However, if it is too hard to remove some interrupt from the user interrupt table (and I'd argue that having to change a local copy of mbed OS is too hard), then I think usability goes out the window if there is no other way to unregister the interrupt from the public box. We may need to rethink including the ability for a box to unregister interrupts.

Nevermind. You are only setting the priority, not claiming the IRQ. Good job.

Patater · 2017-01-30T11:04:01Z

core/system/src/unvic.c

-    if(uv->id || uv->hdlr)
+    if(uv->id != UNVIC_VECTOR_UNUSED)
    {
        HALT_ERROR(PERMISSION_DENIED,


Instead of halting, which shuts down the entire system even on release builds, consider debug asserting so we can catch programmer mistakes, but silently not fulfill the request in release builds. This will allow us to write tests that make sure we handle interrupt ownership contention. Note that this will require changes to all functions that call unvic_acl_check, as the function can now return in the case where the IRQ is registered, but not to the caller. All functions using unvic_acl_check will have to deal with: IRQ unclaimed, IRQ claimed by current box, IRQ claimed by other box.

niklarm · 2017-01-31T11:17:23Z

Updated for review.

Patater · 2017-01-31T11:31:21Z

core/vmpu/inc/vmpu.h

 extern uint32_t vmpu_round_up_region(uint32_t addr, uint32_t size);

+/** Invalid box id for use in marking objects with invalid ownership. */
+#define UVISOR_BOX_ID_INVALID ((uint8_t) -1)


If this were in vmpu_exports.h, we wouldn't have to redefine it as -1 in the unsupported files.

Good point.

Patater · 2017-01-31T11:37:02Z

core/system/src/unvic.c

-        HALT_ERROR(NOT_ALLOWED, "IRQ %d is unregistered; state cannot be changed", irqn);
-    }
+    g_unvic_vector[irqn].id = g_active_box;
+    DPRINTF("IRQ %d registered to box %d\n\r", irqn, g_active_box);


The registration of an irqn to a box is something we do often in these functions. Consider placing this into an itty bitty static function, so we don't have to repeat the code and DPRINTF string so much.

Heck, even the call and check of unvic_acl_check is done very often. Maybe a new function like unvic_irq_register would be good. You'd use it like so.

/* Attempt to register for the IRQ. */ if (!unvic_irq_register(irqn)) { /* We couldn't claim the interrupt. */ return 0; }

This is just a suggestion. Do what you think makes sense.

Patater · 2017-01-31T11:43:27Z

core/system/src/unvic.c

+    for (uint32_t ii=0; ii < NVIC_VECTORS; ii++) {
+        g_unvic_vector[ii].id = UVISOR_BOX_ID_INVALID;
+        g_unvic_vector[ii].hdlr = (TIsrVector) user_vtor[ii + NVIC_OFFSET];
+        /* set default priority (SVC must always be higher) */


Use complete sentences (with capital letter and correct punctuation) for new comments.

Patater · 2017-02-01T10:09:02Z

core/system/src/unvic.c

+static int unvic_isr_register(uint32_t irqn)
+{
+    int isr_status = unvic_acl_check(irqn);
+    switch(isr_status)


Add space before (

Patater

Other than a little missing space, LGTM

Patater · 2017-02-02T10:11:51Z

core/system/src/unvic.c


    /* check if IRQ entry is populated */
-    if(uv->id || uv->hdlr)
+    if(uv->id != UVISOR_BOX_ID_INVALID)


Add space before (

AlessandroA · 2017-02-02T11:16:47Z

@niklas-arm As part of this PR you should also quickly update the docs. Please scan the docs folder for occurrences of the NVIC APIs. While the APIs haven't changed, the underlying behavior has.

This includes at least:

Please specify the new behavior (no need to set an ISR first; first box touching an IRQ gets the IRQ; you cannot release ownership of an IRQ; if you don't set an IRQ you get the box 0 default one).

AlessandroA · 2017-02-02T11:06:41Z

core/system/src/unvic.c

-        NVIC_DisableIRQ(irqn);
-        g_unvic_vector[irqn].id = 0;
+    if (!unvic_isr_register(irqn)) {
+        return;


Here and in other similar instances return should be replaced with an HALT_ERROR, so the user gets a notification, the NVIC API does not silently fail, and the system halts exactly at that point.

The backward compatibility introduced by this PR still allows legacy code to work as expected.

As @Patater points out this is a source of DoS, and it will be solved when we will support individually-killable boxes.

I "fixed" this by halting in the unvic_acl_check function. This prevents duplicate code and restores the original behavior.

Yup! The return gives the wrong expectation, though. Even if we use hardcoded IRQ ACLs, the API will never be meant to return if the IRQ is not registered to the calling box.

Consider replacing occurrences of:

if (!unvic_isr_register(irqn)) { return; }

with:

/* This function halts if the IRQ is owned by another box or by uVisor. */ unvic_isr_register(irqn)

(If instead you want to keep the semantics by which unvic_isr_register(irqn) returns the ownership status of irqn, you will have to halt from every function that calls that function. I like this semantics so I'd rather duplicate the HALT_ERROR. @Patater ?)

-1 for duplication, I'll call the function without if.

AlessandroA · 2017-02-02T11:10:11Z

core/system/src/unvic.c

    NVIC_SetPriorityGrouping(0);
+
+    /* Copy the user interrupt table into the handlers. */
+    for (uint32_t ii=0; ii < NVIC_VECTORS; ii++) {


-uint32_t ii=0; +uint32_t ii = 0;

The ii is very MATLAB-y 😄

AlessandroA · 2017-02-02T11:16:56Z

core/system/src/unvic.c

     * the same priority level. */
    NVIC_SetPriorityGrouping(0);
+
+    /* Copy the user interrupt table into the handlers. */


Could you add a comment here specifying the reason why this is needed and the consequences? In particular, it's important to specify that if a secure box code enables an IRQ before setting its ISR, the IRQ might be immediately served by the static ISR, which is out of the secure box control (because it's usually set by box 0/the host OS).

Related to this: #403 (comment).

AlessandroA · 2017-02-02T11:49:41Z

core/system/src/unvic.c

-        NVIC_DisableIRQ(irqn);
-        g_unvic_vector[irqn].id = 0;
+    if (!unvic_isr_register(irqn)) {
+        return;


Yup! The return gives the wrong expectation, though. Even if we use hardcoded IRQ ACLs, the API will never be meant to return if the IRQ is not registered to the calling box.

Consider replacing occurrences of:

if (!unvic_isr_register(irqn)) { return; }

with:

/* This function halts if the IRQ is owned by another box or by uVisor. */ unvic_isr_register(irqn)

AlessandroA · 2017-02-02T13:30:35Z

core/system/src/unvic.c

+    if (uv->id == UVISOR_BOX_ID_INVALID) {
+        return UNVIC_ISR_OWNER_NONE;
+    }
+    HALT_ERROR(PERMISSION_DENIED, "Permission denied: IRQ %d is owned by box %d\n\r", irqn, uv->id)


Now the return values from both unvic_acl_check and unvic_isr_register do not make sense anymore. I'd rather have unvic_acl_check return the ownership status and unvic_isr_register be a void function that either registers the IRQ or halts.

niklarm · 2017-02-02T15:10:52Z

Updated.

AlessandroA

Thanks a lot for the changes, it's a very neat PR! I left a couple of comments for minor fixes (remember to fix the style when you happen to modify a line with a broken style).

I think this doc still needs an update: https://github.com/ARMmbed/uvisor/blob/master/docs/api/API.md#low-level-apis (the vIRQ_SetVector API in particular).

LGTM after those fixes 👍

AlessandroA · 2017-02-02T15:48:36Z

core/system/src/unvic.c

    }
-
-    return is_irqn_registered;
+    HALT_ERROR(PERMISSION_DENIED, "Permission denied: IRQ %d is owned by another box!\n\r", irqn);


\n\r ---> \r\n

AlessandroA · 2017-02-02T15:49:16Z

core/system/src/unvic.c

-    else {
-        HALT_ERROR(NOT_ALLOWED, "IRQ %d is unregistered; state cannot be changed", irqn);
-    }
+    /* clear pending IRQ */


/* Clear pending IRQ. */

AlessandroA · 2017-02-02T15:49:30Z

core/system/src/unvic.c

-    else {
-        HALT_ERROR(NOT_ALLOWED, "IRQ %d is unregistered; state cannot be changed", irqn);
-    }
+    /* set pending IRQ */


/* Set pending IRQ. */

AlessandroA · 2017-02-02T15:50:25Z

core/system/src/unvic.c

-            irqn,
-            vector ? "registered to" : "released by",
-            g_active_box);
+    /* save unprivileged handler */


/* Save the unprivileged handler. */

AlessandroA · 2017-02-02T17:13:20Z

retest uvisor

AlessandroA · 2017-02-03T11:53:35Z

retest uvisor

AlessandroA · 2017-02-03T17:12:51Z

retest uvisor

AlessandroA · 2017-02-07T11:09:49Z

retest uvisor

Patater suggested changes Jan 30, 2017

View reviewed changes

niklarm force-pushed the feature/nvic_ownership branch 4 times, most recently from f11a3d8 to f0dac5b Compare January 30, 2017 16:28

Patater suggested changes Jan 31, 2017

View reviewed changes

Extract UVISOR_BOX_ID_INVALID to vmpu.h

2bb3468

niklarm force-pushed the feature/nvic_ownership branch 2 times, most recently from 6fb95f7 to 3f5ed64 Compare January 31, 2017 14:00

Patater reviewed Feb 1, 2017

View reviewed changes

niklarm force-pushed the feature/nvic_ownership branch from 3f5ed64 to 85cf97a Compare February 1, 2017 10:54

Patater approved these changes Feb 2, 2017

View reviewed changes

niklarm force-pushed the feature/nvic_ownership branch from 85cf97a to 0cea838 Compare February 2, 2017 11:05

AlessandroA suggested changes Feb 2, 2017

View reviewed changes

niklarm force-pushed the feature/nvic_ownership branch from 0cea838 to 7fce949 Compare February 2, 2017 11:26

AlessandroA suggested changes Feb 2, 2017

View reviewed changes

niklarm force-pushed the feature/nvic_ownership branch from 7fce949 to 7713290 Compare February 2, 2017 12:19

AlessandroA suggested changes Feb 2, 2017

View reviewed changes

niklarm force-pushed the feature/nvic_ownership branch 2 times, most recently from a097bd3 to de4ec48 Compare February 2, 2017 13:38

AlessandroA suggested changes Feb 2, 2017

View reviewed changes

niklarm added 2 commits February 2, 2017 16:34

Make vIRQ functions claim IRQs on first call

327c9ad

Update quickstart guide with IRQ API change

ae6c8d7

niklarm force-pushed the feature/nvic_ownership branch from 11cc71a to ae6c8d7 Compare February 2, 2017 16:34

AlessandroA approved these changes Feb 3, 2017

View reviewed changes

AlessandroA merged commit afb2bd8 into ARMmbed:master Feb 7, 2017

First box to call NVIC owns the interrupt #403

First box to call NVIC owns the interrupt #403

Uh oh!

Conversation

niklarm commented Jan 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niklarm commented Jan 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

Patater Feb 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlessandroA commented Feb 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niklarm commented Feb 2, 2017

Uh oh!

AlessandroA left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlessandroA commented Feb 2, 2017

Uh oh!

AlessandroA commented Feb 3, 2017

Uh oh!

niklarm commented Jan 26, 2017 •

edited

Loading

Patater Feb 2, 2017 •

edited

Loading