VEP 186: Host Devices Assignment with IOMMUFD - #187

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign vladikr for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Hi, Thank you for the detailed proposal and PR, @luckyh
This is important work and a great write-up.

That said, I would prefer a diffrent approach. I strongly prefer avoiding any API changes in the VMI spec (no iommufd: {} field on GPUs/hostDevices/SRIOV)
Adding new fields such as iommufd risks unnecessary complexity for users. The per device field feels like an unnecessary configuration - something that should be an infrastracture concern.

From my point of view, the node labeller can detect whether iommufd is supported on the node and add a label such as kubevirt.io/iommufd=available
virt-launcher, on start up can either use this label or detect the presence of iommufd on the node by itself and add to VFIO enabled devices (my understanding that one mentioning is enough)
It will fall back to legacy vfio if itiommufd wasn't detected.

Device plugins / DRA drivers will own the compatibility decidion. If a given device, such as the modern NVIDIA devices, require iommufd, the driver will advertise the resource on that node only if it detects iommufd presense.

This way, we don't need to confuse our users and there is a clear separation of responsibilities.

What do you think?

Hi, Thank you for the detailed proposal and PR, @luckyh This is important work and a great write-up.

That said, I would prefer a diffrent approach. I strongly prefer avoiding any API changes in the VMI spec (no iommufd: {} field on GPUs/hostDevices/SRIOV) Adding new fields such as iommufd risks unnecessary complexity for users. The per device field feels like an unnecessary configuration - something that should be an infrastracture concern.

From my point of view, the node labeller can detect whether iommufd is supported on the node and add a label such as kubevirt.io/iommufd=available virt-launcher, on start up can either use this label or detect the presence of iommufd on the node by itself and add to VFIO enabled devices (my understanding that one mentioning is enough) It will fall back to legacy vfio if itiommufd wasn't detected.

Device plugins / DRA drivers will own the compatibility decidion. If a given device, such as the modern NVIDIA devices, require iommufd, the driver will advertise the resource on that node only if it detects iommufd presense.

This way, we don't need to confuse our users and there is a clear separation of responsibilities.

What do you think?

Actually I think we need the device plugins to offer the iommufd device and the vfio device (not group as with the legacy approach), from there we just need the relevant changes in launcher. The additional label would be miss leading as only the plugin can know what it will offer...

Hi, Thank you for the detailed proposal and PR, @luckyh This is important work and a great write-up.
That said, I would prefer a diffrent approach. I strongly prefer avoiding any API changes in the VMI spec (no iommufd: {} field on GPUs/hostDevices/SRIOV) Adding new fields such as iommufd risks unnecessary complexity for users. The per device field feels like an unnecessary configuration - something that should be an infrastracture concern.
From my point of view, the node labeller can detect whether iommufd is supported on the node and add a label such as kubevirt.io/iommufd=available virt-launcher, on start up can either use this label or detect the presence of iommufd on the node by itself and add to VFIO enabled devices (my understanding that one mentioning is enough) It will fall back to legacy vfio if itiommufd wasn't detected.
Device plugins / DRA drivers will own the compatibility decidion. If a given device, such as the modern NVIDIA devices, require iommufd, the driver will advertise the resource on that node only if it detects iommufd presense.
This way, we don't need to confuse our users and there is a clear separation of responsibilities.
What do you think?

Actually I think we need the device plugins to offer the iommufd device and the vfio device (not group as with the legacy approach), from there we just need the relevant changes in launcher. The additional label would be miss leading as only the plugin can know what it will offer...

@xpivarc we don't need the iommufd device in the pod per se. We only need to know if the host support iommufd. This could be discovered from sysfs in the pod as wee, without the label on the vmi.

Hi, Thank you for the detailed proposal and PR, @luckyh This is important work and a great write-up.

Hi @vladikr , thank you for your review! Let me comment inline.

That said, I would prefer a diffrent approach. I strongly prefer avoiding any API changes in the VMI spec (no iommufd: {} field on GPUs/hostDevices/SRIOV) Adding new fields such as iommufd risks unnecessary complexity for users. The per device field feels like an unnecessary configuration - something that should be an infrastracture concern.

From my point of view, the node labeller can detect whether iommufd is supported on the node and add a label such as kubevirt.io/iommufd=available virt-launcher, on start up can either use this label or detect the presence of iommufd on the node by itself and add to VFIO enabled devices (my understanding that one mentioning is enough) It will fall back to legacy vfio if itiommufd wasn't detected.

Agree.

Only one thing is that, regarding the live migration case (i.e. introduced by VEP #109), We need to check if qemu supports VFIO legacy <-> iommufd migration (I will investigate this). If not, looks some mechanism is required for noting the type of the allocated resources within virt-launcher. Do you have any preference, I mean, like metadata?

Device plugins / DRA drivers will own the compatibility decidion. If a given device, such as the modern NVIDIA devices, require iommufd, the driver will advertise the resource on that node only if it detects iommufd presense.

Make sense.

This way, we don't need to confuse our users and there is a clear separation of responsibilities.

What do you think?

Thanks.

@xpivarc thanks for the comment!

Actually I think we need the device plugins to offer the iommufd device and the vfio device (not group as with the legacy approach), from there we just need the relevant changes in launcher.

What I'm concerned is, I'm not sure if all the device plugins would follow that. For example, the NVIDIA sandbox-device-plugin (well, for Kata) used to offer the iommufd device but recently removed by the PR with no clear reason I could find (originally, it was introduced as the same, see commit). Anyway..

The additional label would be miss leading as only the plugin can know what it will offer...

But seems we still need a label for indicating the capability of libvirt?

Hi, Thank you for the detailed proposal and PR, @luckyh This is important work and a great write-up.
That said, I would prefer a diffrent approach. I strongly prefer avoiding any API changes in the VMI spec (no iommufd: {} field on GPUs/hostDevices/SRIOV) Adding new fields such as iommufd risks unnecessary complexity for users. The per device field feels like an unnecessary configuration - something that should be an infrastracture concern.
From my point of view, the node labeller can detect whether iommufd is supported on the node and add a label such as kubevirt.io/iommufd=available virt-launcher, on start up can either use this label or detect the presence of iommufd on the node by itself and add to VFIO enabled devices (my understanding that one mentioning is enough) It will fall back to legacy vfio if itiommufd wasn't detected.
Device plugins / DRA drivers will own the compatibility decidion. If a given device, such as the modern NVIDIA devices, require iommufd, the driver will advertise the resource on that node only if it detects iommufd presense.
This way, we don't need to confuse our users and there is a clear separation of responsibilities.
What do you think?

Actually I think we need the device plugins to offer the iommufd device and the vfio device (not group as with the legacy approach), from there we just need the relevant changes in launcher. The additional label would be miss leading as only the plugin can know what it will offer...

I double checked and you are right, /dev/iommu needs to be present in the vmi pod for ATS and CUDA on GH hosts - but the primary interface remains /dev/vfio/

@xpivarc thanks for the comment!

Actually I think we need the device plugins to offer the iommufd device and the vfio device (not group as with the legacy approach), from there we just need the relevant changes in launcher.

What I'm concerned is, I'm not sure if all the device plugins would follow that. For example, the NVIDIA sandbox-device-plugin (well, for Kata) used to offer the iommufd device but recently removed by the PR with no clear reason I could find (originally, it was introduced as the same, see commit). Anyway..

I wouldn't worry about Kata's device plugin because we are not using the same anyway. We need to make sure that we are clearly defining what KubeVirt supports and that we have a correct implementation in the NVIDIA KubeVirt Device plugin and later in DRA.

The additional label would be miss leading as only the plugin can know what it will offer...

But seems we still need a label for indicating the capability of libvirt?

Yes, I think so. We would still need this.

Updated per @vladikr 's suggestion.

Several notes:

• With the current approach, both /dev/iommu and /dev/vfio/devices/vfioX are expected to be allocated by device plugins/DRA drivers, while in the previous design /dev/iommu is allocated by KubeVirt as a generic device.
• The new node label is set when /dev/iommu is present and Libvirt exposes the capability, which could help the schedule of VMI upon VFIO live migration.
• It is confirmed that, migrating a VFIO device with using different backends in src/dst VMI is NOT a valid case, which means, some mechanism is required to ensure the consistency of VFIO usage. This update didn't cover this part as I'd like to open a discussion here. (I feel like metadata could be considered?)

The libvirt patch is merged on upstream (commits), FYI.

Regarding the approach of informing launcher about the iommufd node label, I'd like to propose using the virtualMachineOptions, like:

diff --git a/pkg/virt-handler/vm.go b/pkg/virt-handler/vm.go
index 90ce01cc68..ed029d2851 100644
--- a/pkg/virt-handler/vm.go
+++ b/pkg/virt-handler/vm.go
@@ -2104,6 +2104,12 @@ func (c *VirtualMachineController) syncVirtualMachine(client cmdclient.LauncherC
        options := virtualMachineOptions(smbios, period, preallocatedVolumes, c.capabilities, c.clusterConfig)
        options.InterfaceDomainAttachment = domainspec.DomainAttachmentByInterfaceName(vmi.Spec.Domain.Devices.Interfaces, c.clusterConfig.GetNetworkBindings())
 
+       if c.clusterConfig.IOMMUFDEnabled() {
+               if c.isVMIOwnedByNode(vmi) && util.IsVFIOVMI(vmi) {
+                       options.IOMMUFDAvailabe = c.isIOMMUFDAvailabe()
+               }
+       }
+
        err := client.SyncVirtualMachine(vmi, options)
        if err != nil {
                if strings.Contains(err.Error(), "EFI OVMF rom missing") {
@@ -2114,6 +2120,35 @@ func (c *VirtualMachineController) syncVirtualMachine(client cmdclient.LauncherC
        return err
 }
 
+func (c *VirtualMachineController) isIOMMUFDAvailabe() bool {
+       node, err := c.getNode()
+       if err != nil {
+               // log error
+               return false
+       }
+       val, exist := node.Labels[v1.IOMMUFDLabel]
+       if !exist || val != "available" {
+               return false
+       }
+}
+
+func (c *VirtualMachineController) getNode() (*k8sv1.Node, error) {
+       nodeObj, exists, err := c.nodeStore.GetByKey(c.host)
+       if err != nil {
+               return nil, err
+       }
+       if !exists {
+               return nil, fmt.Errorf("node %s does not exist", c.host)
+       }
+
+       node, ok := nodeObj.(*k8sv1.Node)
+       if !ok {
+               return nil, fmt.Errorf("unknown object type found in node informer")
+       }
+
+       return node, nil
+}
+
 func (c *VirtualMachineController) handleHousekeeping(vmi *v1.VirtualMachineInstance, cgroupManager cgroup.Manager, domainExists bool) error {
        if vmi.IsCPUDedicated() && vmi.Spec.Domain.CPU.IsolateEmulatorThread {
                err := c.configureHousekeepingCgroup(vmi, cgroupManager)

@vladikr @xpivarc does it look good to you, or would you like to suggest a different one?

@luckyh virt-launcher will have all the info and that is
Did the plugin provide /dev/iommu? Did the device has the /dev/vfio/<group> or /dev/vfio/devices/<devid> pattern? And this is enough to know if we should enable the new API or not

@xpivarc sorry for the delayed reply.

@luckyh virt-launcher will have all the info and that is Did the plugin provide /dev/iommu? Did the device has the /dev/vfio/<group> or /dev/vfio/devices/<devid> pattern? And this is enough to know if we should enable the new API or not

Right, the logic of detecting the presence of iommufd and vfio devices can be handled within virt-launcher. However, as the iommufd support on libvirt side is just newly introduced, I suppose that kubevirt needs to cover both the supported and unsupported cases of iommufd.

The iommufd setting of hostdevs (i.e. <driver iommufd='yes'>) will be silently discarded by libvirt when using a previous version (instead of doing the validation and raising errors, because virt-launcher calls CreateWithFlags without the validation flag being set), while newer libvirt can properly assign host devices with iommufd. This (seems) won't lead into a real problem except the live migration case (i.e. vGPU migration between old and new versions), therefore we have to ensure libvirt behaves in the same manner for source and destination vmi since the vfio legacy <-> iommufd migration is invalid.

Hence, in my proposal, the iommufd label is passed from handler to launcher for giving the hint of the corresponding libvirt domain capability, for enforcing the deterministic outcome for source domain configuration. That is, the iommufd xml settings will be rendered by virt-launcher only if,

1. both iommufd and vfio cdev device are present in launcher pod AND
2. libvirt reports the iommufd capability as supported

otherwise, it will fall back to legacy vfio.

Apart from that, the domain capabilities could be also probed within the launcher process, but I'm not sure if that is a suggested approach, do we?

Updated the proposal for adopting the latest change of libvirt (since 12.2.0), that is, now the per-domain configuration <iommufd enabled='yes' .../> is primarily used together with <driver iommufd='no'> for configuring the vfio backend [1].

Apart from that, it expects kubevirt to handle the configuration of the iommufd device instead of libvirt. Due to this requires the SYS_RESOURCE capability the operation has to be performed in virt-handler, therefore, a new mechanism is needed to be introduced for passing file descriptors between handler and launcher.

[1] https://libvirt.org/formatdomain.html#host-device-iommufd

Hi @vladikr @xpivarc , how do you think about taking a separated VEP for introducing a generic fd-passing facility?

Updated with adding the IOMMUFD device plugin content.

cc @vladikr @lyarwood @fossedihelm @xpivarc

Updated the proposal for adopting the latest change of libvirt (since 12.2.0), that is, now the per-domain configuration <iommufd enabled='yes' .../> is primarily used together with <driver iommufd='no'> for configuring the vfio backend [1].

Apart from that, it expects kubevirt to handle the configuration of the iommufd device instead of libvirt. Due to this requires the SYS_RESOURCE capability the operation has to be performed in virt-handler, therefore, a new mechanism is needed to be introduced for passing file descriptors between handler and launcher.

[1] https://libvirt.org/formatdomain.html#host-device-iommufd

Am I understanding this correctly? Basically we need either a webhook or expand virt-handler to do some fd-passing. Currently virt-handler doesn't have this capability correct?

Am I understanding this correctly? Basically we need either a webhook or expand virt-handler to do some fd-passing. Currently virt-handler doesn't have this capability correct?

@ajcaldelas the original idea came up to my mind is that, we could let virt-hander open the /dev/iommu device node, configure the opened fd (which is about to set the per-process memlock accounting mode in order to ensure the proper rlimit setting for VMs, and this requires SYS_RESOURCE), and then pass the fd to virt-launcher.

However, @vladikr gives another proposal, that is, introducing a new device plugin [1] for handling the above procedure. Based on that, it needs to use either the virt-controller or a webhook to inject the iommufd resource request for the device assignments to VMs.

resources:
  limits:
    devices.kubevirt.io/iommufd: "1"

[1] https://github.com/kubevirt/iommufd-device-plugin

Am I understanding this correctly? Basically we need either a webhook or expand virt-handler to do some fd-passing. Currently virt-handler doesn't have this capability correct?

@ajcaldelas the original idea came up to my mind is that, we could let virt-hander open the /dev/iommu device node, configure the opened fd (which is about to set the per-process memlock accounting mode in order to ensure the proper rlimit setting for VMs, and this requires SYS_RESOURCE), and then pass the fd to virt-launcher.

However, @vladikr gives another proposal, that is, introducing a new device plugin [1] for handling the above procedure. Based on that, it needs to use either the virt-controller or a webhook to inject the iommufd resource request for the device assignments to VMs.

resources:
  limits:
    devices.kubevirt.io/iommufd: "1"

[1] kubevirt/iommufd-device-plugin

@luckyh The iommu-device-plugin was created as an external plugin.
The idea is to include it in virt-handler.
@ajcaldelas FYI.
Please let's continue this in #267