NVIDIA: SAUCE: hw/vfio: thread vf-token through iommufd bind

[tdavenvidia]

A VF fails to bind through the iommufd/cdev backend when its PF is bound to vfio-pci and requires a matching VF token.

QEMU currently initializes struct vfio_device_bind_iommufd without the token flag or token UUID.

Pass the existing vf-token device property to the IOMMUFD bind ioctl for PCI devices.

[tdavenvidia]

@nvmochs

Please help review.

[nirmoy]

BaseOS Kernel Review

Warning

⚠️ Review needs attention

The common iommufd backend now references a VFIO PCI-only symbol, causing link failures when IOMMUFD is enabled without VFIO PCI, such as s390x VFIO CCW/AP builds.

Findings: Critical 0 · High 0 · Medium 1 · Low 0

🔍 Review artifacts

• 🟠 Boro review: 1 finding

Review metadata

• Reviewed head: 59ef90a9f06e
• Overall status: attention needed

This comment is maintained by BaseOS Reviewer and updated when the GitHub watcher publishes a newer review.

[nvmochs]

@tdavenvidia

Were you seeing -EINVAL returned?

Codex states it should be -EACCES instead:

Commit message reports the wrong errno: It states that omitting the VF token causes -EINVAL, but the NVIDIA BOS kernel returns -EACCES when a token-protected VF is bound without a token. The implementation is correct; only the commit message needs updating.

---

Nits:

• checkpatch.pl: 0 errors, 1 warning—the multiline comment should start with /* on its own line.
• Prefer the existing vfio_pci_from_vfio_device() helper over the raw type check plus container_of().

[shamiali2008]

I think you can use VFIOPCIDevice *vdev = vfio_pci_from_vfio_device(vbasedev);

Also should we add a check for guarding mdev here? I think a PCI mdev is still type VFIO_DEVICE_TYPE_PCI. Something like below;

VFIOPCIDevice *vdev = vfio_pci_from_vfio_device(vbasedev);
...

if ( !vbasedev->mdev && vdev && !qemu_uuid_is_null(&vdev->vf_token)) {
.....
}

I do have a concern that this patch will cause a regression on mainline kernels < 6.17 where support for vf_token was not there. As Matt mentioned the return error value you mentioned in commit log may change as well based on kernel version I think. The regression concern may not be that relevant for downstream but if you plan to send this to upstream, please check that.

[tdavenvidia]

@nvmochs @shamiali2008

I pushed the changes that addresses you review comments, please take a look.

With an older host kernel that does not support VF token binding, a QEMU launch using vf-token will fail during the device bind. The host kernel may report an error in the kernel log, but that diagnostic is internal to the kernel and does not require any additional handling/mention in QEMU.

[nvmochs]

Thanks Tushar, no further issues from me.

Acked-by: Matthew R. Ochs <[email protected]>

[shamiali2008]

@nvmochs @shamiali2008

I pushed the changes that addresses you review comments, please take a look.

With an older host kernel that does not support VF token binding, a QEMU launch using vf-token will fail during the device bind. The host kernel may report an error in the kernel log, but that diagnostic is internal to the kernel and does not require any additional handling/mention in QEMU.

Yeah. QEMU previously never used vf-token in IOMMUFD cases. So if someone has a <6.17 host kernel and used vf-token in their command line(or libvirt), it used to work. But if they do a QEMU binary update their VM launch will now fail.

For eg: On a 6.14.0-1015-nvidia-64k host kernel:
qemu-system-aarch64: -device vfio-pci,host=0008:06:00.0,rombar=0,id=dev0,iommufd=iommufd0,bus=pcie.port1,vf-token=bd8d9d2b-5a5f-4f5a-a211-f591514ba1f3: vfio 0008:06:00.0: error bind device fd=34 to iommufd=33: Argument list too long

Not 100% sure this falls under "regression" or not. But something to be aware while upstream.

[shamiali2008]

I don't think that boro review on vfio_pci_from_vfio_device() stands as it is already used in the same file. Anyway please double check.

Acked-by: Shameer Kolothum [email protected]

[tdavenvidia]

I don't think that boro review on vfio_pci_from_vfio_device() stands as it is already used in the same file. Anyway please double check.

Acked-by: Shameer Kolothum [email protected]

BORO review

@nvmochs @shamiali2008
I pushed the changes that addresses you review comments, please take a look.
With an older host kernel that does not support VF token binding, a QEMU launch using vf-token will fail during the device bind. The host kernel may report an error in the kernel log, but that diagnostic is internal to the kernel and does not require any additional handling/mention in QEMU.

Yeah. QEMU previously never used vf-token in IOMMUFD cases. So if someone has a <6.17 host kernel and used vf-token in their command line(or libvirt), it used to work. But if they do a QEMU binary update their VM launch will now fail.

For eg: On a 6.14.0-1015-nvidia-64k host kernel: qemu-system-aarch64: -device vfio-pci,host=0008:06:00.0,rombar=0,id=dev0,iommufd=iommufd0,bus=pcie.port1,vf-token=bd8d9d2b-5a5f-4f5a-a211-f591514ba1f3: vfio 0008:06:00.0: error bind device fd=34 to iommufd=33: Argument list too long

Not 100% sure this falls under "regression" or not. But something to be aware while upstream.

That is correct. I saw same.

The "regression" in this commit means that while kernel iommufd path "re-added" the vf-token, the qemu left without it, causing VM launch fail with vf-token passed!
(e.g. commit 86624ba3b522b6512def25534341da93356c8da4 vfio/pci: Do vf_token checks for VFIO_DEVICE_BIND_IOMMUFD)

[tdavenvidia]

I don't think that boro review on vfio_pci_from_vfio_device() stands as it is already used in the same file. Anyway please double check.

Acked-by: Shameer Kolothum [email protected]

I agree. the same function is used in this file.

[tdavenvidia]

@shamiali2008

I updated commit description - should have not call this regression. Take a look.

[MitchellAugustin]

Thanks @tdavenvidia @nvmochs !

This patch looks correct to me. However, it seems like it is probably generally applicable beyond NVIDIA hardware. Has it also been submitted upstream? If so, please include a Link: to the upstream mailing list thread in the commit so we can track the provenance here.

[mxc42]

LGTM, and ran a successful test build. +1 on @MitchellAugustin's comment.

Acked-by: Morgan Hunter <[email protected]>

[tdavenvidia]

Thanks @tdavenvidia @nvmochs !

This patch looks correct to me. However, it seems like it is probably generally applicable beyond NVIDIA hardware. Has it also been submitted upstream? If so, please include a Link: to the upstream mailing list thread in the commit so we can track the provenance here.

I will be sending this upstream.