Add support for userspace IOAPIC #57

sboeuf · 2019-06-12T08:23:02Z

This pull request implements a userspace IOAPIC and plumbs
it into the cloud-hypervisor VMM to allow getting rid of the in
kernel emulation provided by KVM_CREATE_IRQCHIP.
It allows for what is called a "split" irqchip, which means that
only the local APIC is emulated in kernel.

Fixes #13

sboeuf · 2019-06-12T08:40:37Z

I know clippy is not happy, I'll fix it tomorrow...

sboeuf · 2019-06-12T16:30:25Z

Just fixed clippy.

sboeuf · 2019-06-13T02:20:09Z

@sameo @rbradford PR ready for review. I've added a test too!

sboeuf · 2019-06-13T02:20:57Z

FYI, depends on rust-vmm/kvm#51

vmm/src/vm.rs

src/main.rs

sboeuf · 2019-06-17T21:57:45Z

PR updated.
@sameo @chao-p PTAL

devices/src/legacy/serial.rs

chao-p

If you pulled this from crosvm, could you indicate which commit you used?

vmm/src/vm.rs

sboeuf · 2019-06-18T17:02:25Z

@chao-p

If you pulled this from crosvm, could you indicate which commit you used?

Oh yes thanks for reminding me :)

sboeuf · 2019-06-18T17:34:18Z

@chao-p comments addressed, please take another look.

sboeuf · 2019-06-18T20:23:21Z

@sameo @chao-p BTW, the kvm-ioctls PR has been merged, so whenever you think this PR is ready, we can merge it.

sameo · 2019-06-20T05:16:43Z

devices/src/ioapic.rs

+
+type Result<T> = result::Result<T, Error>;
+
+pub type IoapicInterrupt = Box<Fn(MsiMessage) -> result::Result<(), Error> + Send>;


Could we define a trait for that one instead?

pub trait IoapicInterrupt : Send { fn deliver_msi(message: MsiMessage) -> Result<()> }

And then have struct Ioapic use that trait.

Actually, why do we need this to be dynamic? When will we need anything but what's defined in
4b2d4fe#diff-a375fb00f71d40c3e15532f95f8f8933R426 , i.e. an MSI based interrupt injection through KVM? Is this to not add VmFd dependency on the IOAPIC implementation?

As you mentioned, the reason for an abstraction here (callback or trait) is to prevent the IOAPIC code from being aware about the VmFd, which is KVM specific.

sameo · 2019-06-20T05:18:24Z

devices/src/ioapic.rs

+// 11:    Destination Mode - R/W
+// 10-8:  Delivery Mode - R/W
+// 7-0:   Interrupt Vector - R/W
+pub type RedirectionTableEntry = u64;


Wouldn't it be more idiomatic to have a pub struct RedirectionTableEntry(u64) and then implement all the functions below as accessors?

I thought about it, but then it's complicated to access the RedirectionTableEntry everywhere in the code, as you need to cast it into a u64 every single time you want to use it.

vmm/src/vm.rs

sboeuf · 2019-06-20T20:02:58Z

@sameo PR updated as discussed. The IOAPIC now takes a VmFd as parameter, which allows it to call directly into signal_msi().

sameo

Besides the DeviceInterrupt callback that I'd like to see changed into a trait, this looks good to me.

sameo · 2019-06-21T04:57:04Z

devices/src/lib.rs

    IoError(io::Error),
 }
+
+pub type DeviceInterrupt = Box<Fn() -> result::Result<(), std::io::Error> + Send>;


Could we please have that one as a trait and simply let the serial driver implement it?

Yes I can try this, but to bring some clarification, this needs to go the other way around. The serial is a simple consumer of an object that needs to implement the DeviceInterrupt trait. And we need two distincts structures (UserIoapicIrq and KernelIoapicIrq for instance) which implement this trait. And one last point, this trait would only require a method deliver_interrupt() to be implemented.
Is this correct?

sameo · 2019-06-21T05:08:20Z

vmm/src/vm.rs

+                    .map_err(DeviceManagerError::Irq)?;
+
                Box::new(move |_p: InterruptParameters| irqfd.write(1)) as InterruptDelivery
+            };


Just a longer term comment: We have DeviceInterrupt and InterruptDelivery callbacks, for essentially doing the same thing: Delivering an interrupt to the guest. I think we should be looking at converging those 2 into one single trait.
This would make the VMM code clearer and easier to follow.

Agreed. To be specific, I'm looking for something more generic Event (or EventChannel) trait and implementations, just need some abstract around Linux-specific EventFd. It's not only limited to host-to-guest event, but also guest-to-host event, and even inter-communication events between vmm and vhost-user-backend, etc.

trait Event { pub notify(value) } struct IrqEvent { eventfd: Option<EvendFd>; } impl Event for IrqEvent { pub new(eventfd: EventFd) { } pub notify(value: Option) { eventfd.write(1); } }

Your latest version is very close to this and only the naming could be more generic.

This commit anticipate the future need from having support for both in kernel and userspace IOAPIC. The way to signal an interrupt from the serial device will vary depending on the use case, but this should be independent from the serial implementation itself. That's why this patch provides a generic trait for the serial device to call from, so that it can trigger interrupts independently from the IOAPIC type chosen (in kernel vs userspace). Signed-off-by: Sebastien Boeuf <[email protected]>

The goal for cloud-hypervisor is to keep the host safe. With this in mind, we want to emulate as much as possible in userspace instead of in kernel directly. The IOAPIC is a good candidate to move from kernel to userspace, which is why this commit introduces a userspace implementation of the IOAPIC 82093AA based on the documentation: https://pdos.csail.mit.edu/6.828/2016/readings/ia32/ioapic.pdf This code is inspired from the files devices/src/ioapic.rs and devices/src/split_irqchip_common.rs from the crosvm codebase. The reference version used being 6c1e23eee3065b3f3d6fc4fb992ac9884dbabf68. Signed-off-by: Sebastien Boeuf <[email protected]>

The previous commit introduced a userspace implementation of an IOAPIC and this commits aims to plumb it into the cloud-hypervisor VMM. Here is the list of new things brought by this patch: - Update the rust-vmm/kvm-ioctls dependency to benefit from latest patches including the support for split irqchip, and the vector being returned when a VM exit is caused by an EOI. - Enable the split irqchip (which means no IOAPIC or PIC is emulated in kernel). This is done conditionally based on the support of the TSC_DEADLINE_TIMER from both KVM and the underlying CPU. The dependency on TSC_DEADLINE_TIMER is related to KVM which does not support creating the in kernel PIT if it has a split irqchip. - Rely on callbacks to handle the following use cases: - in kernel IOAPIC + serial IRQ (pin based) - in kernel IOAPIC + virtio-pci MSI-X - in kernel IOAPIC + virtio-pci IRQ (pin based) - userspace IOAPIC + serial IRQ (pin based) - userspace IOAPIC + virtio-pci MSI-X - userspace IOAPIC + virtio-pci IRQ (pin based) Fixes cloud-hypervisor#13 Signed-off-by: Sebastien Boeuf <[email protected]>

Based on the newly added code, we expect the split irqchip to be used. This means we should not see any "timer" or "cascade" components attached to the IOAPIC since our userspace IOAPIC does not advertise those. Signed-off-by: Sebastien Boeuf <[email protected]>

sboeuf · 2019-06-21T07:21:16Z

@sameo PR updated. The callback has been removed and replaced by a trait :)
Let me know what you think about this new version!

sameo

I'm ok merging that one as is. There's an ongoing discussion about how to make the interrupt delivery mechanism more generic, but I think this should be part of a separate PR.

If @chao-p is fine with it, we can merge.

chao-p · 2019-06-21T08:02:27Z

I'm ok merging that one as is. There's an ongoing discussion about how to make the interrupt delivery mechanism more generic, but I think this should be part of a separate PR.

If @chao-p is fine with it, we can merge.

yes, we can merge this.

sboeuf requested review from rbradford and sameo June 12, 2019 08:51

sboeuf force-pushed the userspace_ioapic branch from 2f4ca2f to 94bd79a Compare June 12, 2019 16:30

sboeuf force-pushed the userspace_ioapic branch from 94bd79a to 8d07a67 Compare June 12, 2019 21:58

sboeuf changed the title ~~[WIP] Add support for userspace IOAPIC~~ Add support for userspace IOAPIC Jun 12, 2019

sboeuf force-pushed the userspace_ioapic branch 2 times, most recently from 7540e9b to 2d221c4 Compare June 13, 2019 02:19

rbradford reviewed Jun 13, 2019

View reviewed changes

vmm/src/vm.rs Outdated Show resolved Hide resolved

rbradford reviewed Jun 13, 2019

View reviewed changes

src/main.rs Show resolved Hide resolved

sboeuf force-pushed the userspace_ioapic branch 2 times, most recently from ed69207 to 7fb542d Compare June 13, 2019 16:13

rbradford reviewed Jun 13, 2019

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

sboeuf force-pushed the userspace_ioapic branch 2 times, most recently from 70be2f1 to 8acb396 Compare June 17, 2019 21:55

chao-p reviewed Jun 18, 2019

View reviewed changes

devices/src/legacy/serial.rs Outdated Show resolved Hide resolved

chao-p reviewed Jun 18, 2019

View reviewed changes

vmm/src/vm.rs Outdated Show resolved Hide resolved

chao-p reviewed Jun 18, 2019

View reviewed changes

vmm/src/vm.rs Show resolved Hide resolved

sboeuf force-pushed the userspace_ioapic branch from 8acb396 to e6cd989 Compare June 18, 2019 17:33

sameo reviewed Jun 20, 2019

View reviewed changes

sboeuf force-pushed the userspace_ioapic branch 2 times, most recently from 482671f to 7957beb Compare June 20, 2019 19:58

sameo reviewed Jun 21, 2019

View reviewed changes

Sebastien Boeuf added 4 commits June 21, 2019 00:15

sboeuf force-pushed the userspace_ioapic branch from 7957beb to a93ee21 Compare June 21, 2019 07:19

sameo approved these changes Jun 21, 2019

View reviewed changes

chao-p approved these changes Jun 21, 2019

View reviewed changes

sameo merged commit 5f7d520 into cloud-hypervisor:master Jun 21, 2019

sboeuf deleted the userspace_ioapic branch December 5, 2019 07:20


		type Result<T> = result::Result<T, Error>;

		pub type IoapicInterrupt = Box<Fn(MsiMessage) -> result::Result<(), Error> + Send>;

Add support for userspace IOAPIC #57

Add support for userspace IOAPIC #57

Uh oh!

Conversation

sboeuf commented Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sboeuf commented Jun 12, 2019

Uh oh!

sboeuf commented Jun 12, 2019

Uh oh!

sboeuf commented Jun 13, 2019

Uh oh!

sboeuf commented Jun 13, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sboeuf commented Jun 17, 2019

Uh oh!

Uh oh!

chao-p left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sboeuf commented Jun 18, 2019

Uh oh!

sboeuf commented Jun 18, 2019

Uh oh!

sboeuf commented Jun 18, 2019

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!

Uh oh!

Uh oh!

sboeuf commented Jun 20, 2019

Uh oh!

sameo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sboeuf Jun 21, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sboeuf commented Jun 21, 2019

Uh oh!

sameo left a comment

Choose a reason for hiding this comment

Uh oh!

chao-p commented Jun 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sboeuf commented Jun 12, 2019 •

edited

Loading

sboeuf Jun 21, 2019 •

edited

Loading