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

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Eric Auger <eric.auger@redhat.com>
> Sent: Monday, May 5, 2025 9:19 AM
> To: Donald Dutile <ddutile@redhat.com>; Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; qemu-arm@nongnu.org;
> qemu-devel@nongnu.org; Markus Armbruster <armbru@redhat.com>;
> Peter Maydell <peter.maydell@linaro.org>; Daniel P. Berrange
> <berrange@redhat.com>; Alex Bennée <alex.bennee@linaro.org>
> Cc: jgg@nvidia.com; nicolinc@nvidia.com; berrange@redhat.com;
> nathanc@nvidia.com; mochs@nvidia.com; smostafa@google.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 v2 1/6] hw/arm/smmuv3: Add support to associate a
> PCIe RC
> 
> 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.

Yes. The SMMUV3 is a platform device and we are making the bus type
here as PCIE which is a bit odd. As replied elsewhere in this thread,
in the initial RFC we had a specific "pci-bus" property associated with 
SMMUv3 device, 
Eg:
-device arm-smmuv3,pci-bus=pcie.0,...

But then there were feedback that, it is more intuitive and easy for libvirt
if we can just use the default "bus" property associated with a  Qemu device.
Hence the change.

> 
> 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.

Ok. So in the case of platform device how do we envisage to specify the "bus"?

-device arm-smmv3, primary-bus=sysbus

Or something like having a SMMUv3 dev without any bus and each platform
device has to specify the SMMUv3?
Eg:

-device arm-smmv3,id=smmuv3.3
-device xxxx,smmuv3= smmuv3.3

If later, I think we can stick to current one. 

Thanks,
Shameer