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))
>
> Please let me know.
with that fixed, feel free to add my
*Tested-by: Eric Auger <eric.au...@redhat.com> I have tested non
regression on legacy SMMU, SMMU device protecting pcie.0 and pxb
topologies. Looks good to me. Thanks Eric *
>
> Thanks,
> Shameer
