Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Eric Auger <eric.auger@redhat.com>]

Hi,
On 5/2/25 8:16 PM, Donald Dutile wrote:
>
>
> On 5/2/25 6:27 AM, Shameer Kolothum wrote:
>> Although this change does not affect functionality at present, it lays
>> the groundwork for enabling user-created SMMUv3 devices in
>> future patches
>>
>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>> ---
>>   hw/arm/smmuv3.c | 26 ++++++++++++++++++++++++++
>>   hw/arm/virt.c   |  3 ++-
>>   2 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>> index ab67972353..605de9b721 100644
>> --- a/hw/arm/smmuv3.c
>> +++ b/hw/arm/smmuv3.c
>> @@ -24,6 +24,7 @@
>>   #include "hw/qdev-properties.h"
>>   #include "hw/qdev-core.h"
>>   #include "hw/pci/pci.h"
>> +#include "hw/pci/pci_bridge.h"
>>   #include "cpu.h"
>>   #include "exec/target_page.h"
>>   #include "trace.h"
>> @@ -1874,6 +1875,25 @@ static void smmu_reset_exit(Object *obj,
>> ResetType type)
>>       smmuv3_init_regs(s);
>>   }
>>   +static int smmuv3_pcie_bus(Object *obj, void *opaque)
>> +{
>> +    DeviceState *d = opaque;
>> +    PCIBus *bus;
>> +
>> +    if (!object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
>> +        return 0;
>> +    }
>> +
>> +    bus = PCI_HOST_BRIDGE(obj)->bus;
>> +    if (d->parent_bus && !strcmp(bus->qbus.name,
>> d->parent_bus->name)) {
>> +        object_property_set_link(OBJECT(d), "primary-bus", OBJECT(bus),
>> +                                 &error_abort);
>> +        /* Return non-zero as we got the bus and don't need further
>> iteration.*/
>> +        return 1;
>> +    }
>> +    return 0;
>> +}
>> +
>>   static void smmu_realize(DeviceState *d, Error **errp)
>>   {
>>       SMMUState *sys = ARM_SMMU(d);
>> @@ -1882,6 +1902,10 @@ static void smmu_realize(DeviceState *d, Error
>> **errp)
>>       SysBusDevice *dev = SYS_BUS_DEVICE(d);
>>       Error *local_err = NULL;
>>   +    if (!object_property_get_link(OBJECT(d), "primary-bus",
>> &error_abort)) {
>> +        object_child_foreach_recursive(object_get_root(),
>> smmuv3_pcie_bus, d);
>> +    }
>> +
>>       c->parent_realize(d, &local_err);
>>       if (local_err) {
>>           error_propagate(errp, local_err);
>> @@ -1996,6 +2020,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->bus_type = TYPE_PCIE_BUS;
> Does this force legacy SMMUv3 to be tied to a PCIe bus now?
> if so, will that break some existing legacy smmuv3 configs?, i.e.,
> virtio-scsi attached to a legacy smmuv3.

Previously the SMMU was already always attached to a PCI primary-bus
(vms->bus ie. pci0). virtio-scsi-pci is the device being protected. The
SMMU is not able to protect platforms devices atm.

My only concern is we are highjacking the "bus" prop to record the bus
hierarchy the SMMU is protecting. While the SMMU is a platform device
and does not inherit the PCI device base class its bus type becomes
"TYPE_PCIE_BUS". So in terms of qom hierachy is is seen as a PCI device
now? I don't know if it is a problem. An alternative could be to keep
the bus pointer and type as it was before and introduce a primary-bus
property. Adding Markus, Peter, Daniel and Alex in to.

At some point it was envisionned to support protected platform devices
(I think this was need for CCA). My fear is that if we turn the bus type
to PCIE it may be difficult to extend the support to non PCIe protected
devices. The SMMU shall remain a platform device being able to protect
either PCI devices and, in the future, platform devices.

Thanks

Eric
>
>>   }
>>     static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 177f3dd22c..3bae4e374f 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"
>> @@ -1442,7 +1443,7 @@ static void create_smmu(const VirtMachineState
>> *vms,
>>       }
>>       object_property_set_link(OBJECT(dev), "primary-bus", OBJECT(bus),
>>                                &error_abort);
>> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> +    qdev_realize_and_unref(dev, &bus->qbus, &error_fatal);
>>       sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
>>       for (i = 0; i < NUM_SMMU_IRQS; i++) {
>>           sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
>