> -----Original Message-----
> From: Eric Auger <eric.au...@redhat.com>
> Sent: Tuesday, July 8, 2025 10:47 AM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.th...@huawei.com>; qemu-...@nongnu.org;
> qemu-devel@nongnu.org
> Cc: peter.mayd...@linaro.org; j...@nvidia.com; nicol...@nvidia.com;
> ddut...@redhat.com; berra...@redhat.com; imamm...@redhat.com;
> nath...@nvidia.com; mo...@nvidia.com; smost...@google.com;
> gustavo.rom...@linaro.org; m...@redhat.com;
> marcel.apfelb...@gmail.com; Linuxarm <linux...@huawei.com>;
> Wangzhou (B) <wangzh...@hisilicon.com>; jiangkunkun
> <jiangkun...@huawei.com>; Jonathan Cameron
> <jonathan.came...@huawei.com>; zhangfei....@linaro.org
> Subject: Re: [PATCH v6 08/12] hw/arm/virt: Allow user-creatable SMMUv3
> dev instantiation
>
> Hi Shameer,
>
> On 7/8/25 10:54 AM, Shameerali Kolothum Thodi wrote:
> > Hi Eric,
> >
> >> -----Original Message-----
> >> From: Eric Auger <eric.au...@redhat.com>
> >> Sent: Tuesday, July 8, 2025 8:41 AM
> >> To: Shameerali Kolothum Thodi
> >> <shameerali.kolothum.th...@huawei.com>; qemu-...@nongnu.org;
> >> qemu-devel@nongnu.org
> >> Cc: peter.mayd...@linaro.org; j...@nvidia.com; nicol...@nvidia.com;
> >> ddut...@redhat.com; berra...@redhat.com; imamm...@redhat.com;
> >> nath...@nvidia.com; mo...@nvidia.com; smost...@google.com;
> >> gustavo.rom...@linaro.org; m...@redhat.com;
> >> marcel.apfelb...@gmail.com; Linuxarm <linux...@huawei.com>;
> >> Wangzhou (B) <wangzh...@hisilicon.com>; jiangkunkun
> >> <jiangkun...@huawei.com>; Jonathan Cameron
> >> <jonathan.came...@huawei.com>; zhangfei....@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.came...@huawei.com>
> >>> Reviewed-by: Eric Auger <eric.au...@redhat.com>
> >>> Tested-by: Nathan Chen <nath...@nvidia.com>
> >>> Signed-off-by: Shameer Kolothum
> >> <shameerali.kolothum.th...@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))
> the logical dependency on acpi_dev is maybe not that straightforward.
> Also it has has_ged and aarch64 check
> I thing I would simply check !(firmware_loaded &&
> virt_is_acpi_enabled(vms)). about the aarch64 check I am not sure this
> is needed.
Right. I think has_ged is enable by default from 4.1 and on aarch64, isn't
SMMUV3 only enabled for ARM64 cases? At least on kernel it is, I think.
Anyway, agree, the firmware_loaded && virt_is_acpi_enabled(vms) is
more readable. I will fix it.
Thanks,
Shameer
