RE: [PATCH v6 08/12] hw/arm/virt: Allow user-creatable SMMUv3 dev instantiation

[Shameerali Kolothum Thodi via <qemu-arm@nongnu.org>]

Hi Eric,

> -----Original Message-----
> From: Eric Auger <eric.auger@redhat.com>
> Sent: Tuesday, July 8, 2025 8:41 AM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; qemu-arm@nongnu.org;
> qemu-devel@nongnu.org
> Cc: peter.maydell@linaro.org; jgg@nvidia.com; nicolinc@nvidia.com;
> ddutile@redhat.com; berrange@redhat.com; imammedo@redhat.com;
> nathanc@nvidia.com; mochs@nvidia.com; smostafa@google.com;
> gustavo.romero@linaro.org; mst@redhat.com;
> marcel.apfelbaum@gmail.com; Linuxarm <linuxarm@huawei.com>;
> Wangzhou (B) <wangzhou1@hisilicon.com>; jiangkunkun
> <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH v6 08/12] hw/arm/virt: Allow user-creatable SMMUv3
> dev instantiation
> 
> Hi Shameer,
> 
> On 7/3/25 10:46 AM, Shameer Kolothum wrote:
> > Allow cold-plugging of an SMMUv3 device on the virt machine when no
> > global (legacy) SMMUv3 is present or when a virtio-iommu is specified.
> >
> > This user-created SMMUv3 device is tied to a specific PCI bus provided
> > by the user, so ensure the IOMMU ops are configured accordingly.
> >
> > Due to current limitations in QEMU’s device tree support, specifically
> > its inability to properly present pxb-pcie based root complexes and
> > their devices, the device tree support for the new SMMUv3 device is
> > limited to cases where it is attached to the default pcie.0 root complex.
> >
> > Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> > Reviewed-by: Eric Auger <eric.auger@redhat.com>
> > Tested-by: Nathan Chen <nathanc@nvidia.com>
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  hw/arm/smmu-common.c         |  8 +++++-
> >  hw/arm/smmuv3.c              |  2 ++
> >  hw/arm/virt.c                | 50 ++++++++++++++++++++++++++++++++++++
> >  hw/core/sysbus-fdt.c         |  3 +++
> >  include/hw/arm/smmu-common.h |  1 +
> >  5 files changed, 63 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> > index b15e7fd0e4..2ee4691299 100644
> > --- a/hw/arm/smmu-common.c
> > +++ b/hw/arm/smmu-common.c
> > @@ -959,7 +959,12 @@ static void smmu_base_realize(DeviceState *dev,
> Error **errp)
> >                  goto out_err;
> >              }
> >          }
> > -        pci_setup_iommu(pci_bus, &smmu_ops, s);
> > +
> > +        if (s->smmu_per_bus) {
> > +            pci_setup_iommu_per_bus(pci_bus, &smmu_ops, s);
> > +        } else {
> > +            pci_setup_iommu(pci_bus, &smmu_ops, s);
> > +        }
> >          return;
> >      }
> >  out_err:
> > @@ -984,6 +989,7 @@ static void smmu_base_reset_exit(Object *obj,
> ResetType type)
> >
> >  static const Property smmu_dev_properties[] = {
> >      DEFINE_PROP_UINT8("bus_num", SMMUState, bus_num, 0),
> > +    DEFINE_PROP_BOOL("smmu_per_bus", SMMUState, smmu_per_bus,
> false),
> >      DEFINE_PROP_LINK("primary-bus", SMMUState, primary_bus,
> >                       TYPE_PCI_BUS, PCIBus *),
> >  };
> > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> > index ab67972353..bcf8af8dc7 100644
> > --- a/hw/arm/smmuv3.c
> > +++ b/hw/arm/smmuv3.c
> > @@ -1996,6 +1996,8 @@ static void smmuv3_class_init(ObjectClass
> *klass, const void *data)
> >      device_class_set_parent_realize(dc, smmu_realize,
> >                                      &c->parent_realize);
> >      device_class_set_props(dc, smmuv3_properties);
> > +    dc->hotpluggable = false;
> > +    dc->user_creatable = true;
> >  }
> >
> >  static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 05a14881cf..8662173c43 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -56,6 +56,7 @@
> >  #include "qemu/cutils.h"
> >  #include "qemu/error-report.h"
> >  #include "qemu/module.h"
> > +#include "hw/pci/pci_bus.h"
> >  #include "hw/pci-host/gpex.h"
> >  #include "hw/virtio/virtio-pci.h"
> >  #include "hw/core/sysbus-fdt.h"
> > @@ -1440,6 +1441,28 @@ static void create_smmuv3_dt_bindings(const
> VirtMachineState *vms, hwaddr base,
> >      g_free(node);
> >  }
> >
> > +static void create_smmuv3_dev_dtb(VirtMachineState *vms,
> > +                                  DeviceState *dev, PCIBus *bus)
> > +{
> > +    PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms-
> >platform_bus_dev);
> > +    SysBusDevice *sbdev = SYS_BUS_DEVICE(dev);
> > +    int irq = platform_bus_get_irqn(pbus, sbdev, 0);
> > +    hwaddr base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
> > +    MachineState *ms = MACHINE(vms);
> > +
> > +    if (strcmp("pcie.0", bus->qbus.name)) {
> > +        warn_report("SMMUv3 device only supported with pcie.0 for DT");
> while testing the series I hit the warning with a rhel guest which boots
> with ACPI.
> I think we shall make the check smarter to avoid that.
> maybe also check firmware_loaded and virt_is_acpi_enabled()?

Thanks for giving it a spin. Yes, just confirmed that the warning appears.
The above check will work, but can we make use of vms->acpi_dev for
this check instead? It is essentially the same and I think that will work. 

    if (!vms->acpi_dev && strcmp("pcie.0", bus->qbus.name))

Please let me know.

Thanks,
Shameer