libsel4vm: Enqueue IRQs only once by hlyytine · Pull Request #28 · seL4/seL4_projects_libs

hlyytine · 2022-03-02T12:00:02Z

The real GIC has exactly zero or one pending request for any IRQ.

Signed-off-by: Hannu Lyytinen hannux@ssrc.tii.ae

hlyytine · 2022-03-02T12:08:34Z

It appears that vgic_vcpu_inject_irq() always succeeded, either by succeeding to load the IRQ into hardware-backed LR registers (4) or if that fails, into the software-extended 64 LR registers. Because it always succeeded, vgic_handle_overflow_cpu() (part of vGIC maintenance code) would always consume the queued IRQ. But in case vgic_vcpu_inject_irq() failed to load IRQ into HW list register, the consumed IRQ was appended back to software LR registers.

So that's how the code used to work, for reference. The real problem was that there was not any check whether or not the IRQ was already loaded into these list registers. I have had a fix for this already, but it was rather clumsy. Now I realized a simple refactoring is all that is needed.

A little background: we have been playing with Raspberry Pi 4 for some time now and found out virtio-console makes this code bail out, saying the software-extended LRs are overflowing. The more cores you enable in the guest VM, the sooner that happens (with single core it happens never in practice, with four core it is a matter of seconds). This fix is a big improvement to that.

axel-h · 2022-03-08T16:34:35Z

It appears that vgic_vcpu_inject_irq() always succeeded, either by succeeding to load the IRQ into hardware-backed LR registers (4) or if that fails, into the software-extended 64 LR registers

Not sure I follow, but the list could be full (unlikely(lr_overflow->full)), so this could still fail in theory in extreme situations unless there is a guarantee the list can really holds all possible (1021?) interrupts.

axel-h · 2022-03-08T17:43:35Z

From my side this seems the proper fix and I'd consider this as a bug.

hlyytine · 2022-03-09T11:46:56Z

It appears that vgic_vcpu_inject_irq() always succeeded, either by succeeding to load the IRQ into hardware-backed LR registers (4) or if that fails, into the software-extended 64 LR registers

Not sure I follow, but the list could be full (unlikely(lr_overflow->full)), so this could still fail in theory in extreme situations unless there is a guarantee the list can really holds all possible (1021?) interrupts.

It is entirely possible in theory, but I would refrain addressing that issue now. If we continue using the ring buffer for overflowed IRQs, it should be just a matter of enlarging the buffer. But perhaps some day we would like to emulate more recent GICs also and in the future there are more and more ARM chips that support up to 8192 interrupt IDs.

Another thing is whether or not we want to support IRQ priorities some day. For that the ring buffer is not the optimal data structure IMO and for example, priority queue based on binary heap could be more proper solution. That's why I would like to leave it as it is now and guest Linux does not need priorities anyway.

What I meant to say in the text you quoted was that the ill-named vgic_vcpu_inject_irq() does not fail normally, it either succeeds loading the IRQ into HW registers (or to the abstraction for them within seL4 kernel but that's another story) or if that fails, it succeeds appending the IRQ to overflow list. And this "always succeeds" resulted in unnecessary shuffling of IRQs out of that list, just to be put into that again.

The first iteration that I subjected to review had to solve that problem in order to figure out whether or not the IRQ was queued. But now I realized we could use is_pending() instead and the whole bugfix would reduce to a one-liner. But since it appears we agree that the code is rather complex, I took some liberties and renamed variables, fields and functions and even added one commit which I believe adds even more readability. I'm happy to add more comments but I believe the code itself must be improved first.

hlyytine · 2022-03-09T12:56:53Z

Any opinions whether I should split the first commit into two, one having only this "if (is_pending(...))" (the real fix) and another having the renames? The third that gets rid of per-CPU helper functions is perhaps best to left unmodified?

Or one for is_pending() fix and the another for readability improvement? Personally I think the former (three commits) is the best.

Ivan-Velickovic · 2022-03-10T01:53:13Z

The number of list registers (NUM_LRS) is implementation defined and so isn't the same across all systems (although 4 seems to be the common number), would it be worth adding a comment above the #define noting this?

hlyytine · 2022-03-10T23:29:40Z

Redid the patch, personally I think it's one more step towards readability, let me know what you think.

kent-mcleod · 2022-03-11T00:19:30Z

Any opinions whether I should split the first commit into two, one having only this "if (is_pending(...))" (the real fix) and another having the renames? The third that gets rid of per-CPU helper functions is perhaps best to left unmodified?

It would make it easier to review and integrate these changes if they were in separate commits. (There's something like 4 or 5 different branches that implement gicv3 virt that may need to cherry-pick or rebase over these changes).

hlyytine · 2022-03-11T00:21:44Z

Any opinions whether I should split the first commit into two, one having only this "if (is_pending(...))" (the real fix) and another having the renames? The third that gets rid of per-CPU helper functions is perhaps best to left unmodified?

It would make it easier to review and integrate these changes if they were in separate commits. (There's something like 4 or 5 different branches that implement gicv3 virt that may need to cherry-pick or rebase over these changes).

That's my thinking as well. Right now I squashed all my previous changes into one, because the intermediate commits made some changes just to be changed again in later commits, so I hope this squashed commit is easier to review. After we agree on the contents, I can proceed with splitting the PR into several commits.

axel-h · 2022-03-11T09:33:31Z

Thanks for the work, the code is really getting nicer now.

hlyytine · 2022-03-11T11:02:53Z

Thank you for your suggestions, I applied them to whole file and added two commits -- one that addresses the locations you mentioned and the other one addresses the rest. Kept them separate now for clarity.

hlyytine · 2022-03-11T11:11:46Z

Just pondering; shouldn't we reserve the bool return type to predicates (function name should contain is or some adjective)? Thinking about this vgic_irq_enqueue(), the function name or the if statement containing that is not asking about the truth value, therefore it feels a bit funny for it to answer true or false to something that in expressed in an imperative mood. Maybe old fashioned zero on success, non-zero otherwise would be better together with your idea of first assigning it to err variable and then checking it?

axel-h · 2022-03-11T11:12:38Z

-    vgic_handle_overflow(vgic, vcpu);
+    struct virq_handle *virq = vgic_irq_dequeue(vgic, vcpu);
+    if (virq) {
+        return vgic_vcpu_load_list_reg(vgic, vcpu, idx, virq);


I wonder, if we should print an error message here, because this would indicate a serious problem in the maintenance operation

Suggested change

return vgic_vcpu_load_list_reg(vgic, vcpu, idx, virq);

int err = vgic_vcpu_load_list_reg(vgic, vcpu, idx, virq);

if (err) {

ZF_LOGE("IRQ injection failed, code %d\n", err);

}

return err

But vgic_vcpu_load_list_reg() will print an error message regardless who calls it?

yes, you are right. It would make a nice error stack though seeing this bubble up though the layers then.

Now that is something I can agree.

axel-h · 2022-03-11T11:14:26Z

    assert(idx >= 0);

-    err = handle_vgic_maintenance(vcpu, idx);
+    int err = handle_vgic_maintenance(vcpu, idx);


Maybe we should also log a message here if this fails, as things are badly broken then. See also comment https://github.com/seL4/seL4_projects_libs/pull/28/files#r824617661

I think we should have a way to halt the VM execution at this point -- I think it should execute correctly or not execute at all, something in between only results in frustation and wasted time debugging something flaky.

It's a lost interrupt in this case, which is not necessarily this fatal, but very annoying. Logging is the best one can do to allow post-mortem analysis. The error behavior should become a follow-up discussion to leave out od this PR, otherwise this never gets merged ;)

True -- actually that reminds me I have pondered whether these readability improvements should be a separate PR (or moving the real bugfix into its own PR).

From my side, having this in a separate commit is sufficient. It's more about how to get the others to agree to merge all the improvements, there are a few other PRs now also that seem useful (#29, #31 ...)
Opinions, @yyshen, @kent-mcleod, @lsf37

axel-h · 2022-03-11T11:22:56Z

-        vgic->irq[inject_vcpu->vcpu_id][i] = irq;
+    int err = seL4_ARM_VCPU_InjectIRQ(vcpu->vcpu.cptr, irq->virq, 0, 0, idx);
+    if (err) {
+        ZF_LOGF("Failure loading vGIC list register");


Suggested change

ZF_LOGF("Failure loading vGIC list register");

ZF_LOGF("Failure loading vGIC list register, code %d", err);

hlyytine · 2022-03-11T11:51:28Z

Pushed one more commit that reverts vgic_irq_enqueue() using zero/non-zero return values ar per discussed above. Now I think I'm done, waiting for further discussion.

wom-bat · 2022-03-23T01:18:45Z

@hlyytine is it your intention to split this into separate PRs or are you happy for this one to be updated and merged?

wom-bat

This looks good; but needs rebasing to make it mergeable.

The real GIC has exactly zero or one pending request for any IRQ. Signed-off-by: Hannu Lyytinen <hannux@ssrc.tii.ae>

hlyytine · 2022-03-23T20:31:44Z

@hlyytine is it your intention to split this into separate PRs or are you happy for this one to be updated and merged?

Yes, I reorganized these into three commits: one for bugfix, one for refactoring in general and last one for style issues (mainly converting to C99 as suggested by @axel-h).

axel-h · 2022-03-23T20:32:22Z

-    bool full;
 };

-static inline size_t LR_OF_NEXT(size_t idx)


Please leave this in unless there is a strong reason to go back to the macro.

I can do that, but I would like you to first review my patch set fully; I'm getting rid of that full field of struct lr_of and ring buffer code is much more comprehensible now IMHO. I was thinking my static_assert about the requirement of size of power of two and the and-operation for finding out the next index would be self-documenting. TBH I found your addition not exactly helping understanding the code and feel that the data structure is implemented in non-optimal way if it needs helpers/comments like that. But not really wanting to start a flame war, I just would like you to read all my code first.

Sorry, my bad. Looked like a merge conflict to me first, but with the 2^n size this is better.
My argument for the function is, that this allows a strong binding to the actual array size in the struct and not just a constant. It's supposed to make the code more robust and the compiler is able to optimize this like the macro.

But that stronger array binding could also be done in the macro. I may make a separate PR for this, so it's no blocker for this PR.

Yeah, my intention was to ping you and ask how do you like my proposal, but you were faster. And I can see where you are coming from. We could have sanity checks with this 2^n also, but I guess then it boils down to more fundamental questions like a) what else should we assert and b) shouldn't we be doing the assert before using the index (right before dereferencing an array), not just when derive a new index.

Ideally the data structures should be in a separate header/source file, having them in between the code that uses them makes it quite hard to grasp sometimes. But something like that is a PITA to do in C.

Forgot to mention that I am not that worried about performance in this context. It is just readability that matters to me.

My argument for the function is, that this allows a strong binding to the actual array size in the struct and not just a constant. It's supposed to make the code more robust and the compiler is able to optimize this like the macro.

I think this issue could be resolved outside of this PR. The macro and array definitions are defined within 10 lines of each other and use the same size constant which is already good enough.

kent-mcleod

I also think this is a nice improvement. My only request is that the middle commit message has a description of the irq_queue related changes also mentions that this sets up for supporting interrupt priorities in the future as well as allowing the maintenance handling code to become simpler.

This is mostly rearranging the code so that the IRQ queueing mechanism becomes more apparent, making also the maintenance handler much simpler. The ring buffer is implemented in a bit more readable way and it is decoupled from the rest; this makes it easier to support IRQ priorities in the future. Signed-off-by: Hannu Lyytinen <hannux@ssrc.tii.ae>

Signed-off-by: Hannu Lyytinen <hannux@ssrc.tii.ae>

hlyytine · 2022-03-24T09:08:24Z

True, added that now.

The requested changes have been made now

hlyytine force-pushed the upstream/vgic-fix branch from acac68b to 276c55d Compare March 2, 2022 13:42

axel-h requested a review from yyshen March 8, 2022 12:29

axel-h reviewed Mar 8, 2022

View reviewed changes

Comment thread libsel4vm/src/arch/arm/vgic/vgic.c Outdated

axel-h reviewed Mar 8, 2022

View reviewed changes

Comment thread libsel4vm/src/arch/arm/vgic/vgic.c Outdated

axel-h reviewed Mar 8, 2022

View reviewed changes

Comment thread libsel4vm/src/arch/arm/vgic/vgic.c Outdated

axel-h reviewed Mar 8, 2022

View reviewed changes

Comment thread libsel4vm/src/arch/arm/vgic/vgic.c

axel-h added the bug label Mar 8, 2022

hlyytine force-pushed the upstream/vgic-fix branch from 276c55d to 985f110 Compare March 9, 2022 11:23

hlyytine force-pushed the upstream/vgic-fix branch from 985f110 to 20d4c25 Compare March 9, 2022 12:00

axel-h reviewed Mar 10, 2022

View reviewed changes

Comment thread libsel4vm/src/arch/arm/vgic/vgic.c Outdated

axel-h reviewed Mar 10, 2022

View reviewed changes

Comment thread libsel4vm/src/arch/arm/vgic/vgic.c Outdated

axel-h reviewed Mar 10, 2022

View reviewed changes

Comment thread libsel4vm/src/arch/arm/vgic/vgic.c Outdated

axel-h reviewed Mar 10, 2022

View reviewed changes

Comment thread libsel4vm/src/arch/arm/vgic/vgic.c Outdated

axel-h reviewed Mar 10, 2022

View reviewed changes

Comment thread libsel4vm/src/arch/arm/vgic/vgic.c Outdated

axel-h reviewed Mar 10, 2022

View reviewed changes

Comment thread libsel4vm/src/arch/arm/vgic/vgic.c Outdated

axel-h reviewed Mar 10, 2022

View reviewed changes

Comment thread libsel4vm/src/arch/arm/vgic/vgic.c Outdated

hlyytine force-pushed the upstream/vgic-fix branch from 20d4c25 to bef99fc Compare March 10, 2022 23:23

hlyytine force-pushed the upstream/vgic-fix branch 3 times, most recently from fdc6591 to 2d7a65e Compare March 11, 2022 00:17

hlyytine force-pushed the upstream/vgic-fix branch from 2d7a65e to 6b466d2 Compare March 11, 2022 07:52

axel-h reviewed Mar 11, 2022

View reviewed changes

Comment thread libsel4vm/src/arch/arm/vgic/vgic.c Outdated

axel-h reviewed Mar 11, 2022

View reviewed changes

Comment thread libsel4vm/src/arch/arm/vgic/vgic.c

axel-h reviewed Mar 11, 2022

View reviewed changes

Comment thread libsel4vm/src/arch/arm/vgic/vgic.c Outdated

axel-h reviewed Mar 11, 2022

View reviewed changes

Comment thread libsel4vm/src/arch/arm/vgic/vgic.c

hlyytine force-pushed the upstream/vgic-fix branch from 560c73c to 17b5a41 Compare March 11, 2022 11:06

axel-h reviewed Mar 11, 2022

View reviewed changes

wom-bat previously requested changes Mar 23, 2022

View reviewed changes

hlyytine force-pushed the upstream/vgic-fix branch from 59936b4 to e6eccc7 Compare March 23, 2022 20:20

libsel4vm: enqueue IRQs only once

be647d8

The real GIC has exactly zero or one pending request for any IRQ. Signed-off-by: Hannu Lyytinen <hannux@ssrc.tii.ae>

hlyytine force-pushed the upstream/vgic-fix branch from e6eccc7 to 54fefe0 Compare March 23, 2022 20:29

axel-h reviewed Mar 23, 2022

View reviewed changes

This was referenced Mar 24, 2022

make LR_OF_NEXT() more robust #37

Closed

trivial: cleanup helper functions setting VGIC distributor registers #31

Merged

libsel4vm: fix number of GIC active_clr registers #29

Merged

kent-mcleod approved these changes Mar 24, 2022

View reviewed changes

hlyytine added 2 commits March 24, 2022 10:47

trivial: improve style for readability

1e25455

Signed-off-by: Hannu Lyytinen <hannux@ssrc.tii.ae>

hlyytine force-pushed the upstream/vgic-fix branch from 54fefe0 to 1e25455 Compare March 24, 2022 09:07

kent-mcleod merged commit 557b1fa into seL4:master Mar 24, 2022

hlyytine deleted the upstream/vgic-fix branch March 24, 2022 09:34

-        return vgic_vcpu_load_list_reg(vgic, vcpu, idx, virq);
+        int err = vgic_vcpu_load_list_reg(vgic, vcpu, idx, virq);
+        if (err) {
+            ZF_LOGE("IRQ injection failed, code %d\n", err);
+        }
+        return err

	ZF_LOGF("Failure loading vGIC list register");
	ZF_LOGF("Failure loading vGIC list register, code %d", err);

Conversation

hlyytine commented Mar 2, 2022

Uh oh!

hlyytine commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

axel-h commented Mar 8, 2022

Uh oh!

Uh oh!

axel-h commented Mar 8, 2022

Uh oh!

hlyytine commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hlyytine commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ivan-Velickovic commented Mar 10, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hlyytine commented Mar 10, 2022

Uh oh!

kent-mcleod commented Mar 11, 2022

Uh oh!

hlyytine commented Mar 11, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

axel-h commented Mar 11, 2022

Uh oh!

hlyytine commented Mar 11, 2022

Uh oh!

hlyytine commented Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

hlyytine commented Mar 11, 2022

Uh oh!

wom-bat commented Mar 23, 2022

Uh oh!

wom-bat left a comment

Choose a reason for hiding this comment

Uh oh!

hlyytine commented Mar 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hlyytine Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

hlyytine commented Mar 2, 2022 •

edited

Loading

hlyytine commented Mar 9, 2022 •

edited

Loading

hlyytine commented Mar 9, 2022 •

edited

Loading

hlyytine commented Mar 11, 2022 •

edited

Loading

hlyytine Mar 23, 2022 •

edited

Loading