Re: [RFC PATCH v3 05/15] hw/arm/smmuv3-accel: Introduce smmuv3 accel device

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

Hi Shameer,

On 7/16/25 11:27 AM, Shameerali Kolothum Thodi wrote:
>
>> -----Original Message-----
>> From: Duan, Zhenzhong <zhenzhong.duan@intel.com>
>> Sent: Wednesday, July 16, 2025 4:39 AM
>> To: Nicolin Chen <nicolinc@nvidia.com>
>> Cc: Shameerali Kolothum Thodi
>> <shameerali.kolothum.thodi@huawei.com>; qemu-arm@nongnu.org;
>> qemu-devel@nongnu.org; eric.auger@redhat.com;
>> peter.maydell@linaro.org; jgg@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; shameerkolothum@gmail.com
>> Subject: RE: [RFC PATCH v3 05/15] hw/arm/smmuv3-accel: Introduce
>> smmuv3 accel device
>>
>>
>>
>>> -----Original Message-----
>>> From: Nicolin Chen <nicolinc@nvidia.com>
>>> Subject: Re: [RFC PATCH v3 05/15] hw/arm/smmuv3-accel: Introduce
>>> smmuv3 accel device
>>>
>>> On Tue, Jul 15, 2025 at 10:48:31AM +0000, Duan, Zhenzhong wrote:
>>>>> +static const TypeInfo types[] = {
>>>>> +    {
>>>>> +        .name = TYPE_ARM_SMMUV3_ACCEL,
>>>>> +        .parent = TYPE_ARM_SMMUV3,
>>>>> +        .class_init = smmuv3_accel_class_init,
>>>>> +    }
>>>> In cover-letter, I see "-device arm-smmuv3", so where is above accel
>>>> device created so we could use smmuv3_accel_ops?
>>> The smmu-common.c is the shared file between accel and non-accel
>>> instances. It has a module property:
>>>    DEFINE_PROP_BOOL("accel", SMMUState, accel, false),
>> It looks we expose a new TYPE_ARM_SMMUV3_ACCEL type device just for
>> exporting accel iommu_ops?
>> What about returning accel iommu_ops directly in
>> smmu_iommu_ops_by_type() and drop the new type?
> We are not creating any new device here. Its just a Class object of different type.
> I had a different approach previously and Eric suggested to try this as there
> are examples in VFIO/IOMMUFD for something like this.
>
> https://lore.kernel.org/qemu-devel/1105d100-dd1e-4aca-b518-50f903967416@redhat.com/
Actually I pointed out that usually we don't add methods in states as it
was done in v2 (in SMMUState) but rather in classes hence my suggestion
to use a class instead. Now what looks strange is your class does not
implement any method ;-)

docs/devel/qom.rst says "The #ObjectClass typically holds a table of function pointers
for the virtual methods implemented by this type."

Sorry if I was unclear.

Now if you don't need any "accel" specific methods besides the PCIIOMMUOps which have a specific struct, I am not sure we need a class anymore. or then you direct embed the PCIIOMMUOps Struct in the method?

Also it's true that's weird that the actual object is never instantiated and may also appear in qom object list. This is not the case for the example I gave, ie. the VFIOContainerBase.

I don't know if anyone has a better/more elegant idea?

Thanks

Eric

>
> Thanks,
> Shameer
>
>>> where it directs to different iommu_ops:
>>> 937 static const PCIIOMMUOps *smmu_iommu_ops_by_type(SMMUState
>> *s)
>>> 938 {
>>> 939     SMMUBaseClass *sbc;
>>> 940
>>> 941     if (s->accel) {
>>> 942         sbc =
>>> ARM_SMMU_CLASS(object_class_by_name(TYPE_ARM_SMMUV3_ACCEL));
>>> 943     } else {
>>> 944         sbc =
>>> ARM_SMMU_CLASS(object_class_by_name(TYPE_ARM_SMMU));
>>> 945     }
>>> 946     assert(sbc->iommu_ops);
>>> 947
>>> 948     return sbc->iommu_ops;
>>> 949 }
>>>
>>> Nicolin