Re: [RFC PATCH v2 05/20] hw/arm/smmuv3-accel: Associate a pxb-pcie bus

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Mar 24, 2025 at 03:56:12PM +0100, Eric Auger wrote:
> 
> 
> On 3/21/25 1:59 AM, Donald Dutile wrote:
> >
> >
> > On 3/19/25 2:21 PM, Eric Auger wrote:
> >> Hi Don,
> >>
> >>
> >> On 3/19/25 5:21 PM, Donald Dutile wrote:
> >>>
> >>>
> >>> On 3/19/25 5:26 AM, Shameerali Kolothum Thodi wrote:
> >>>> Hi Don,
> >>>>
> >>> Hey!
> >>>
> >>>>> -----Original Message-----
> >>>>> From: Donald Dutile <ddutile@redhat.com>
> >>>>> Sent: Tuesday, March 18, 2025 10:12 PM
> >>>>> To: Shameerali Kolothum Thodi
> >>>>> <shameerali.kolothum.thodi@huawei.com>; qemu-arm@nongnu.org;
> >>>>> qemu-devel@nongnu.org
> >>>>> Cc: eric.auger@redhat.com; peter.maydell@linaro.org; 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: [RFC PATCH v2 05/20] hw/arm/smmuv3-accel: Associate a
> >>>>> pxb-
> >>>>> pcie bus
> >>>>>
> >>>>> Shameer,
> >>>>>
> >>>>> Hi!
> >>>>>
> >>>>> On 3/11/25 10:10 AM, Shameer Kolothum wrote:
> >>>>>> User must associate a pxb-pcie root bus to smmuv3-accel
> >>>>>> and that is set as the primary-bus for the smmu dev.
> >>>>>>
> >>>>>> Signed-off-by: Shameer Kolothum
> >>>>> <shameerali.kolothum.thodi@huawei.com>
> >>>>>> ---
> >>>>>>     hw/arm/smmuv3-accel.c | 19 +++++++++++++++++++
> >>>>>>     1 file changed, 19 insertions(+)
> >>>>>>
> >>>>>> diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
> >>>>>> index c327661636..1471b65374 100644
> >>>>>> --- a/hw/arm/smmuv3-accel.c
> >>>>>> +++ b/hw/arm/smmuv3-accel.c
> >>>>>> @@ -9,6 +9,21 @@
> >>>>>>     #include "qemu/osdep.h"
> >>>>>>
> >>>>>>     #include "hw/arm/smmuv3-accel.h"
> >>>>>> +#include "hw/pci/pci_bridge.h"
> >>>>>> +
> >>>>>> +static int smmuv3_accel_pxb_pcie_bus(Object *obj, void *opaque)
> >>>>>> +{
> >>>>>> +    DeviceState *d = opaque;
> >>>>>> +
> >>>>>> +    if (object_dynamic_cast(obj, "pxb-pcie-bus")) {
> >>>>>> +        PCIBus *bus = PCI_HOST_BRIDGE(obj->parent)->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 0;
> >>>>>> +}
> >>>>>>
> >>>>>>     static void smmu_accel_realize(DeviceState *d, Error **errp)
> >>>>>>     {
> >>>>>> @@ -17,6 +32,9 @@ static void smmu_accel_realize(DeviceState *d,
> >>>>>> Error
> >>>>> **errp)
> >>>>>>         SysBusDevice *dev = SYS_BUS_DEVICE(d);
> >>>>>>         Error *local_err = NULL;
> >>>>>>
> >>>>>> +    object_child_foreach_recursive(object_get_root(),
> >>>>>> +                                   smmuv3_accel_pxb_pcie_bus, d);
> >>>>>> +
> >>>>>>         object_property_set_bool(OBJECT(dev), "accel", true,
> >>>>>> &error_abort);
> >>>>>>         c->parent_realize(d, &local_err);
> >>>>>>         if (local_err) {
> >>>>>> @@ -33,6 +51,7 @@ static void smmuv3_accel_class_init(ObjectClass
> >>>>> *klass, void *data)
> >>>>>>         device_class_set_parent_realize(dc, smmu_accel_realize,
> >>>>>>                                         &c->parent_realize);
> >>>>>>         dc->hotpluggable = false;
> >>>>>> +    dc->bus_type = TYPE_PCIE_BUS;
> >>>>>>     }
> >>>>>>
> >>>>>>     static const TypeInfo smmuv3_accel_type_info = {
> >>>>>
> >>>>> I am not seeing the need for a pxb-pcie bus(switch) introduced for
> >>>>> each
> >>>>> 'accel'.
> >>>>> Isn't the IORT able to define different SMMUs for different RIDs?
> >>>>> if so,
> >>>>> itsn't that sufficient
> >>>>> to associate (define) an SMMU<->RID association without introducing a
> >>>>> pxb-pcie?
> >>>>> and again, I'm not sure how that improves/enables the device<->SMMU
> >>>>> associativity?
> >>>>
> >>>> Thanks for taking a look at the series. As discussed elsewhere in
> >>>> this thread(with
> >>>> Eric), normally in physical world (or atleast in the most common
> >>>> cases) SMMUv3
> >>>> is attached to PCIe Root Complex and if you take a look at the IORT
> >>>> spec, it describes
> >>>> association of ID mappings between a RC node and SMMUV3 node.
> >>>>
> >>>> And if my understanding is correct, in Qemu, only pxb-pcie allows you
> >>>> to add
> >>>> extra root complexes even though it is still plugged to
> >>>> parent(pcie.0). ie, for all
> >>>> devices downstream it acts as a root complex but still plugged into a
> >>>> parent pcie.0.
> >>>> This allows us to add/describe multiple "smmuv3-accel" each
> >>>> associated with a RC.
> >>>>
> >>> I find the qemu statements a bit unclear here as well.
> >>> I looked at the hot plug statement(s) in docs/pcie.txt, as I figured
> >>> that's where dynamic
> >>> IORT changes would be needed as well.  There, it says you can hot-add
> >>> PCIe devices to RPs,
> >>> one has to define/add RP's to the machine model for that plug-in.
> >>>
> >>> Using libvirt, it could auto-add the needed RPs to do dynmaic smmuv3
> >>> additions,
> >> I am not sure I understand your statement here. we don't want "dynamic"
> >> SMMUv3 instantiation. SMMUv3 is a platform device which is supposed to
> >> be coldplugged on a pre-existing PCIe hierarchy. The SMMUv3 device is
> >> not something that is meant to be hotplugged or hotunplugged.
> >> To me we hijack the bus= property to provide information about the IORT
> >> IDMAP
> >>
> > Dynamic in the sense that if one adds smmuv3 for multiple devices,
> > libvirt will dynamically figure out how to instantiate one, two,
> > three... smmu's
> > in the machine at cold boot.
> > If you want a machine to be able to hot-plug a device that would
> > require another smmu,
> > than the config, and smmu, would have to be explicilty stated; as is
> > done today for
> > hot-plug PCIe if the simple machine that libvirt would make is not
> > sufficient to
> > hot-add a PCIe device.
> 
> Hum this will need to be discussed with libvirt guys but I am not sure
> they will be inclined to support such kind of policy, esp because vIOMMU
> is a pretty marginal use case as of now. They do automatic instantiation
> for pcie, usb controllers but I am not sure they will take care of the
> vIOMMU tbh

Honestly I've lost track of what's going on this thread design-wise.

As general precedence though, the PCI(e) hierarchies libvirt auto-creates
are very flat - no PXBs for example, no association of host/guest NUMA,
etc. Libvirt does as little as possible in order to get PCI devices
working and anything even slightly "fancy" is left upto the mgmt app
to define. IIRC we have a few scenarios where IOMMUs get auto-added,
but mostly we expect the mgmt app to define them.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|