Re: Query on ARM SMMUv3 nested support

[Nicolin Chen <nicolinc@nvidia.com>]

On Mon, Apr 08, 2024 at 11:13:02PM -0700, Nicolin Chen wrote:
> Sorry for my belated reply.
> 
> On Tue, Apr 02, 2024 at 07:25:56AM +0000, Shameerali Kolothum Thodi wrote:
> 
> > Here we have specified nested-smmuv3 and have  a PCIe root port(ioh3420) and
> > a virtio-pci dev(for virtfs). Once this VM is up and running, both the ioh3420
> > and virtio-9p-pci will not work as expected as we return sysmem address space
> > for them.
> > 
> > And then later when you try to hotplug a vfio dev to the PCIe root port,
> > device_add vfio-pci,host=0000:7d:02.1,bus=rp1,id=net1,iommufd=iommufd0
> > 
> > it won't work either because the address space for ioh3420 is wrong.
> > 
> > Taking another look at this, I think we can replace my earlier attempt to fix
> > this by detecting if the dev is vfio or not and add that into the check below,
> > 
> > --- a/hw/arm/smmu-common.c
> > +++ b/hw/arm/smmu-common.c
> > @@ -619,8 +619,15 @@ static AddressSpace *smmu_find_add_as(PCIBus
> > *bus, void *opaque, int devfn)
> >      SMMUState *s = opaque;
> >      SMMUPciBus *sbus = smmu_get_sbus(s, bus);
> >      SMMUDevice *sdev = smmu_get_sdev(s, sbus, bus, devfn);
> > +    bool is_vfio = false;
> > +    PCIDevice *pdev;
> > 
> > -    if (s->nested && !s->s2_hwpt) {
> > +    pdev = pci_find_device(bus, pci_bus_num(bus), devfn);
> > +    if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
> > +        is_vfio = true;
> > +    }
> > +
> > +    if (is_vfio && s->nested && !s->s2_hwpt) {
> >          return &sdev->as_sysmem;
> >      } else {
> >          return &sdev->as;
> > 
> > Please let me know your thoughts on this.
> 
> Hmm, checking "vfio-pci" here feels a bit of layer violation..
> But I understand how you got there...
> 
> I think that the current way of enabling the "nested" flag for
> a device is a bit messy:
>     if -machine has "iommu=nested-smmuv3" &&
>        -device is "vfio_pci" &&
>        -device has "iommufd"
> 
> Any other device that doesn't meet the latter two conditions
> is ambiguously added to a soft smmuv3, which feels wrong to
> me now.
> 
> Also, the callbacks from hw/vfio/pci.c to iommu are tricky as
> well:
>     vfio_realize() {
>         pci_device_iommu_address_space() {
>             smmu_find_add_as();   // Can only assume vfio-pci is behind
>                                   // a nested SMMU. So returns system AS
>         }
>         vfio_attach_device() {
>             ioctl(VFIO_DEVICE_BIND_IOMMUFD);
>             ioctl(VFIO_DEVICE_ATTACH_IOMMUFD_PT);
>             listener_add_address_space() {
>                 if (memory_region_is_iommu(section->mr)) { // Skip
>                     iommu_notifier_init(IOMMU_NOTIFIER_IOTLB_EVENTS);
>                     memory_region_iommu_replay();
>                 }
>             }
>         }
>     }
> 
> In my view, we should sort this out with a cleaner enabling
> command first. And then, as Jason pointed out, other devices
> (non vfio-pci) should probably not be added to the SMMU, i.e.
> they might need to be added to a separate PCI bus/bridge with
> out iommu. In this case, the vSMMU module would not need an
> iommu AS at all when "iommu=nested-smmuv3".

Just recalled that kvm_arch_fixup_msi_route still requires a
"vfio-pci" device to return an IOMMUMemoryRegion v.s. system
memory. And that's why there's a "!s2_hwpt" condition (hack)
when returning the system AS, so that (at the beginning) the
vfio attach could bypass the iotlb registeration using system
AS and (after s2_hwpt is allocated) MSI could translate the
MSI address using iommu AS.

Otherwise, such a device could (likely "should" in my view)
stay with the system AS, since it doesn't need the emulated 
iotlb/cache at all. Yet in that case, we would need another
pci_device_msi_address_space for MSI..

@Eric,
Would it be possible for you shed some light also?

In a summary, we have the following opens:
1. How should we enable "nesting" for a passthrough device?
   - Currently is a bit ambiguous:
     if -machine has "iommu=nested-smmuv3" &&
        -device is "vfio_pci" &&
        -device has "iommufd"
   - Actually "-device has 'iommufd'" only guarantees using
     the IOMMUFD cdev APIs v.s. the legacy one, i.e. it's not
     necessary to enable nesting supposedly?

2. How should we handle a non passthrough device (no nesting)?
   - Current is a bit ambiguous again by adding it to the soft
     SMMUv3. But, is it necessary? It doesn't seem to have any
     positive performance gain.
   - Yet not adding it behind the SMMUv3 would require another
     noiommu PCI bus to dock such a device, I assume?

3. Should we keep a nesting-enabled passthrough device in the
   system AS v.s. iommu AS?
   - The reasoning is that it doesn't need an emulated iotlb
     or cache since it uses the HW one.
   - Also, with a HW-accelerated VCMDQ on NVIDIA Grace, the
     emulated iotlb should be turned off because there will be
     no TLBI/ATC command trapped by QEMU, as all those commands
     will be issued to the VCMDQ HW directly.
   - The only usage of an iommu AS is the MSI code, as it's a
     bit unique requiring for a gIOVA to gPA translation. So,
     keeping such a device in the system AS would need another
     pci_device_msi_address_space, which feels reasonable since
     we need something similar in the kernel too:
     https://lore.kernel.org/lkml/f65a3d7e3e0e0c7b2f5cfc03ada9618aa8b523bb.1683688960.git.nicolinc@nvidia.com/

Thanks
Nicolin