Re: [PATCH v3 1/6] hw/arm/smmuv3: Check SMMUv3 has PCIe Root Complex association

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 5 Jun 2025 11:29:59 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:

> > -----Original Message-----
> > From: Igor Mammedov <imammedo@redhat.com>
> > Sent: Thursday, June 5, 2025 11:55 AM
> > To: Shameer Kolothum via <qemu-devel@nongnu.org>
> > Cc: Shameerali Kolothum Thodi
> > <shameerali.kolothum.thodi@huawei.com>; qemu-arm@nongnu.org;
> > eric.auger@redhat.com; peter.maydell@linaro.org; jgg@nvidia.com;
> > nicolinc@nvidia.com; ddutile@redhat.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 v3 1/6] hw/arm/smmuv3: Check SMMUv3 has PCIe Root
> > Complex association
> > 
> > On Mon, 2 Jun 2025 16:41:05 +0100
> > Shameer Kolothum via <qemu-devel@nongnu.org> wrote:
> >   
> > > Although this change does not affect functionality at present, it is
> > > required when we add support for user-creatable SMMUv3 devices in
> > > future patches.
> > >
> > > Signed-off-by: Shameer Kolothum  
> > <shameerali.kolothum.thodi@huawei.com>  
> > > ---
> > >  hw/arm/smmuv3.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> > > index ab67972353..7e934336c2 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"
> > > @@ -1881,6 +1882,13 @@ static void smmu_realize(DeviceState *d, Error  
> > **errp)  
> > >      SMMUv3Class *c = ARM_SMMUV3_GET_CLASS(s);
> > >      SysBusDevice *dev = SYS_BUS_DEVICE(d);
> > >      Error *local_err = NULL;
> > > +    Object *bus;
> > > +
> > > +    bus = object_property_get_link(OBJECT(d), "primary-bus",  
> > &error_abort);
> > I'd replace this with direct field access like in smmu_base_realize  
> 
> Ok.
>  
> > in QEMU with PCI, usually we specify bus to attach to with 'bus' property,
> > wouldn't it better to rename "primary-bus" to 'bus' to be consistent with
> > the rest of PCI code (and before "primary-bus" shows up as a CLI option,
> > so far (before this series) it looks like it's an internal property)?  
> 
> That was tried in v2 and since SMMUv3 is not a pci device by itself(it is a 
> sysbus device) reusing the default "bus" property to establish an association
> with a PCI bus created problems,
> https://lore.kernel.org/qemu-devel/877c2ut0zk.fsf@pond.sub.org/

that was an approach was trying to workaround by patching dc->bus_type,
which is obviously wrong.

I'm not talking about changing device type or something similar,
but about renaming 'primary-bus' property name to 'bus'
so it would be consistent interface wise with PCI or other QEMU devices
that are attached to a bus.

> > > +    if (!bus || !object_dynamic_cast(bus->parent,  
> > TYPE_PCI_HOST_BRIDGE)) {
> > Also looking at smmu_base_realize, it has NULL pointer check already.
> > Which also rises question, shouldn't smmu_base_realize check for
> > TYPE_PCI_HOST_BRIDGE as well (aka can smmu be attached to anything
> > else but a host bridge)?  
> 
> Not at the moment in Qemu. Though the SMMUv3 specification allows it to
> be associated with non-pci devices as well.

then perhaps move, the check to smmu_base_realize() for now?

if smmu + non-pci ever materialize, it can be refactored at that time. 
 
> Thanks,
> Shameer
>