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 <ddut...@redhat.com>
>>>>> Sent: Tuesday, March 18, 2025 10:12 PM
>>>>> To: Shameerali Kolothum Thodi
>>>>> <shameerali.kolothum.th...@huawei.com>; qemu-...@nongnu.org;
>>>>> qemu-devel@nongnu.org
>>>>> Cc: eric.au...@redhat.com; peter.mayd...@linaro.org; j...@nvidia.com;
>>>>> nicol...@nvidia.com; berra...@redhat.com; nath...@nvidia.com;
>>>>> mo...@nvidia.com; smost...@google.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: [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.th...@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
Eric
>
>> Thanks
>>
>> Eric
>>> if I understand how libvirt does that today for pcie devices now (/me
>>> looks at danpb for feedback).
>>>
>>>> Having said that, current code only allows pxb-pcie root complexes
>>>> avoiding
>>>> the pcie.0. The idea behind this was, user can use pcie.0 with a non
>>>> accel SMMUv3
>>>> for any emulated devices avoiding the performance bottlenecks we are
>>>> discussing for emulated dev+smmuv3-accel cases. But based on the
>>>> feedback from
>>>> Eric and Daniel I will relax that restriction and will allow
>>>> association with pcie.0.
>>>>
>>> So, I think this isn't a restriction that this smmuv3 feature should
>>> enforce;
>>> lack of a proper RP or pxb-pcie will yield an invalid config
>>> issue/error, and
>>> the machine definition will be modified to meet the needs for IORT.
>>>
>>>> Thanks,
>>>> Shameer
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>>>> to root complexes.
>>>>> Feel free to enlighten me where I may have mis-read/interpreted the
>>>>> IORT
>>>>> & SMMUv3 specs.
>>>>>
>>>>> Thanks,
>>>>> - Don
>>>>>
>>>>
>>>
>>
>
