Directnic nvidia stable 10.1

[tdavenvidia]

This PR adds DirectNIC QEMU support to nvidia_stable-10.1.

[nvmochs]

I confirmed that these commits match what I had previously reviewed internally. I don't think it makes sense to keep the README since end-users will be relying on documentation rather than a commit message - I plan on dropping that when merging.

Acked-by: Matthew R. Ochs <mochs@nvidia.com>

[shamiali2008]

I understand this is a rebase of an existing solution onto this branch and that it is highly NVIDIA specific. Given that, I haven’t done a thorough review at this point. I’m fine proceeding based on the test and verification results, and if those look good, please go ahead

[MitchellAugustin]

Thanks all - I left a few questions that I'd like your input on, as well as some minor change requests.

Since this is a somewhat large delta not intended for upstream, I'd say my main focus is to ensure that any potentially user-visible differences are well documented in relevant parts of the source and in Nvidia's end-user documentation, if it is expected that the behavior for some common action might differ between our qemu's behavior and mainline.

I only sparsely commented areas that were primarily concentrated with very hardware-specific changes (such as specific address mappings that I don't have the full context on), so lmk if you would like additional input from me on any of those areas.

[MitchellAugustin]

nit: s/avoind/avoid

[tdavenvidia]

Sure thing.

[MitchellAugustin]

I'm wondering if there's any opportunity / benefit of optimizing these nested loops - perhaps by tracking the highest address of the new highest shifted BAR within an iteration, and then using that as the alignment input for the pbar[j]? (if I understand correctly, this outermost while(overlap) is only needed since a given BAR could theoretically be adjusted multiple times, which I think that might alleviate.)

As-is, I don't think this would cause a performance issue since I don't see any expensive operations here (and since AFAIK we only have a single-digit number of BARS we're iterating through), but it seems like something that could require optimization if any expensive operations are ever added to these inner loops in the future.

WDYT, worthwhile optimization or not necessary here?

[tdavenvidia]

This GPA=HPA changes will be updated with more generic solution - I will address any optimization then. Right now it looks good IMO.

[MitchellAugustin]

Would you mind adding a comment to the code here to briefly describe the format of what is being read from the file descriptors later in this function, and how that content is being used in the context of this function?

[tdavenvidia]

That inconsistency is stylistic; there’s no semantic difference. I’ll unify the loop bounds for clarity in when I make generic solution. would that be okay?

[MitchellAugustin]

Assuming this reply was meant for my above comment - in this thread, I was curious about the syntax of the data read from {vdev->vbasedev.sysfsdev}/resource here on line 292.

[tdavenvidia]

My comment about, "That inconsistency is stylistic; ..." was about inner and outer for loop comment that you asked.

Would you mind adding a comment to the code here to briefly describe the format of what is being read from the file descriptors later in this function, and how that content is being used in the context of this function?

It’s the sysfs path string for the VFIO device. In QEMU’s VFIO stack, VFIODevice has a char *sysfsdev field that points to the kernel sysfs entry of the host device (e.g., /sys/bus/pci/devices/0009:06:00.0). I don't think we need comment here.

[MitchellAugustin]

Is this TODO anything we should be concerned about within the scope of this PR, or for a future PR and expected to remain a TODO at this stage?

[tdavenvidia]

No that TODO is not necessary.

[tdavenvidia]

Thanks for review, I replied to your comments, let me know.

[tdavenvidia]

Sure, I will remove IORESOURCE_MEM_64

[tdavenvidia]

This GPA=HPA changes will be updated with more generic solution - I will address any optimization then. Right now it looks good IMO.

[tdavenvidia]

That inconsistency is stylistic; there’s no semantic difference. I’ll unify the loop bounds for clarity in when I make generic solution. would that be okay?

[tdavenvidia]

No that TODO is not necessary.

[tdavenvidia]

Thank you for your thorough review :)

[tdavenvidia]

Mainly for my understanding: If this is only needed for GPUDirect RDMA, is it necessary to make this change for all PCI devices (as opposed to just NICs and GPUs?)

We are adding 'acs_caps' as a generic option, right? ACS is per device primary applicable RP and Downstream Ports , and so 'acs_caps'.

I'd also like to have a better understanding of whether you think this kind of change might cause any unexpected end-user-visible behavior when doing passthrough operations,

Admin who is launching QEMU and using ACS should know what they are doing. Like kernel which allows to use 'config_acs' kernel parameter , acs_caps serves the same purpose, for example in this case to allow p2p between pcie devices e.g. GPU and NIC.

Also, making the ACS read-only to the guest seems like something that might be useful to other hardware, and also might be nice to have as an explicit qemu configuration option rather than being implemented in this function - are there plans to make this a more general configurable in that future upstream PR?

The acs_caps is generic option for all PCIe.

FWIW, here is the Testing command for GB200/GB300 GPUDirect RDMA using Nvidia GPU, CX8 and Data Direct Interface:

qemu-system-aarch64 \
          -object iommufd,id=iommufd0 \
          -machine hmat=on -machine virt,accel=kvm,gic-version=3,ras=on,grace-pcie-mmio-identity=on,highmem-mmio-size=4T \
          -cpu host -smp cpus=16 -m size=16G,slots=2,maxmem=256G -nographic \
          -object memory-backend-ram,size=8G,id=m0 \
          -object memory-backend-ram,size=8G,id=m1 \
          -numa node,memdev=m0,cpus=0-15,nodeid=0 -numa node,memdev=m1,nodeid=1 \
          -numa node,nodeid=2 -numa node,nodeid=3 -numa node,nodeid=4 -numa node,nodeid=5\
          -numa node,nodeid=6 -numa node,nodeid=7 -numa node,nodeid=8 -numa node,nodeid=9\
          -device pxb-pcie,id=pcie.1,bus_nr=1,bus=pcie.0 \
          -device arm-smmuv3,primary-bus=pcie.1,id=smmuv3.1,accel=on,ats=on,ril=off,pasid=on,oas=48,cmdqv=on \
          -device pcie-root-port,id=pcie.port1,bus=pcie.1,chassis=1,io-reserve=0,acs-caps=0x1C \
          -device x3130-upstream,id=upstream1,bus=pcie.port1 \
          -device xio3130-downstream,id=downstream1_1,bus=upstream1,chassis=1,slot=1,acs-caps=0x19 \
          -device vfio-pci,host=0018:03:00.0,bus=downstream1_1,id=dmapf1,iommufd=iommufd0 \
          -device xio3130-downstream,id=downstream1_2,bus=upstream1,chassis=1,slot=2,acs-caps=0x15 \
          -device vfio-pci-nohotplug,host=0018:06:00.0,bus=downstream1_2,rombar=0,id=dev0,iommufd=iommufd0 \
          -object acpi-generic-initiator,id=gi0,pci-dev=dev0,node=2 \
          -object acpi-generic-initiator,id=gi1,pci-dev=dev0,node=3 \
          -object acpi-generic-initiator,id=gi2,pci-dev=dev0,node=4 \
          -object acpi-generic-initiator,id=gi3,pci-dev=dev0,node=5 \
          -object acpi-generic-initiator,id=gi4,pci-dev=dev0,node=6 \
          -object acpi-generic-initiator,id=gi5,pci-dev=dev0,node=7 \
          -object acpi-generic-initiator,id=gi6,pci-dev=dev0,node=8 \
          -object acpi-generic-initiator,id=gi7,pci-dev=dev0,node=9 \
          -bios /usr/share/AAVMF/AAVMF_CODE.fd \
          -device nvme,drive=nvme0,serial=deadbeaf1,bus=pcie.0 \
          -drive file=<IMG.qcow2>,index=0,media=disk,format=qcow2,if=none,id=nvme0 \
          -device e1000,netdev=net0,bus=pcie.0 \
          -device pxb-pcie,id=pcie.9,bus_nr=9,bus=pcie.0 \
          -device arm-smmuv3,primary-bus=pcie.9,id=smmuv3.2,accel=on,ats=on,ril=off,pasid=on,oas=48,cmdqv=on \
          -device pcie-root-port,id=pcie.port9,bus=pcie.9,chassis=4,io-reserve=0 \
          -device x3130-upstream,id=upstream9,bus=pcie.port9 \
          -device xio3130-downstream,id=downstream9_1,bus=upstream9,chassis=4,slot=1 \
          -device vfio-pci,host=0012:03:00.1,bus=downstream9_1,id=nic1,iommufd=iommufd0 \
          -netdev user,id=net0,hostfwd=tcp::5558-:22,hostfwd=tcp::5586-:5586 \

[tdavenvidia]

"val" refers to as a PCIe ACS Register value, so IMO it is fine.

[MitchellAugustin]

Thanks the quick responses to my review comments/questions. This all looks good to me now.

Acked-by: Mitchell Augustin <mitchell.augustin@canonical.com>

[nvmochs]

Merged and will be present in 1:10.1.0+nvidia5-1 (and later). Closing PR.