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

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

On 7/14/25 7:23 PM, Nicolin Chen wrote:
> On Mon, Jul 14, 2025 at 04:59:31PM +0100, Shameer Kolothum wrote:
>> Also setup specific PCIIOMMUOps for accel SMMUv3 as accel
>> SMMUv3 will have different handling for those ops callbacks
>> in subsequent patches.
>>
>> The "accel" property is not yet added, so users cannot set it at this
>> point. It will be introduced in a subsequent patch once the necessary
>> support is in place.
>>
>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Overall the patch looks good to me,
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
>
> with some nits:
>
>> @@ -61,7 +61,8 @@ arm_common_ss.add(when: 'CONFIG_ARMSSE', if_true: files('armsse.c'))
>>  arm_common_ss.add(when: 'CONFIG_FSL_IMX7', if_true: files('fsl-imx7.c', 'mcimx7d-sabre.c'))
>>  arm_common_ss.add(when: 'CONFIG_FSL_IMX8MP', if_true: files('fsl-imx8mp.c'))
>>  arm_common_ss.add(when: 'CONFIG_FSL_IMX8MP_EVK', if_true: files('imx8mp-evk.c'))
>> -arm_common_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true: files('smmuv3.c'))
>> +arm_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true: files('smmuv3.c'))
>> +arm_ss.add(when: ['CONFIG_ARM_SMMUV3', 'CONFIG_IOMMUFD'], if_true: files('smmuv3-accel.c'))
> Wondering why "arm_common_ss" is changed to "arm_ss"?
Indeed why did you need to change that?

Thanks

Eric
>
>> +static SMMUv3AccelDevice *smmuv3_accel_get_dev(SMMUState *bs, SMMUPciBus *sbus,
>> +                                                PCIBus *bus, int devfn)
> There seems to be an extra space in the 2nd line.
>
>> +{
>> +    SMMUDevice *sdev = sbus->pbdev[devfn];
>> +    SMMUv3AccelDevice *accel_dev;
>> +
>> +    if (sdev) {
>> +        accel_dev = container_of(sdev, SMMUv3AccelDevice, sdev);
>> +    } else {
>> +        accel_dev = g_new0(SMMUv3AccelDevice, 1);
>> +        sdev = &accel_dev->sdev;
>> +
>> +        sbus->pbdev[devfn] = sdev;
>> +        smmu_init_sdev(bs, sdev, bus, devfn);
>> +    }
> Could just:
>     if (sdev) {
>         return container_of(sdev, SMMUv3AccelDevice, sdev);
>     }
>
> Then, no extra indentations for the rest of the code.
>
>> +
>> +    return accel_dev;
>> +}
>> +
>> +static AddressSpace *smmuv3_accel_find_add_as(PCIBus *bus, void *opaque,
>> +                                              int devfn)
>> +{
>> +    SMMUState *bs = opaque;
>> +    SMMUPciBus *sbus;
>> +    SMMUv3AccelDevice *accel_dev;
>> +    SMMUDevice *sdev;
>> +
>> +    sbus = smmu_get_sbus(bs, bus);
>> +    accel_dev = smmuv3_accel_get_dev(bs, sbus, bus, devfn);
>> +    sdev = &accel_dev->sdev;
> Maybe just:
>
> +    SMMUPciBus *sbus = smmu_get_sbus(bs, bus);
> +    SMMUv3AccelDevice *accel_dev = smmuv3_accel_get_dev(bs, sbus, bus, devfn);
> +    SMMUDevice *sdev = &accel_dev->sdev;
>
> ?
>
>> +typedef struct SMMUv3AccelDevice {
>> +    SMMUDevice  sdev;
> Let's drop the extra space in between.
>
>> +} SMMUv3AccelDevice;
>> +
>> +#endif /* HW_ARM_SMMUV3_ACCEL_H */
>> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
>> index eb94623555..c459d24427 100644
>> --- a/include/hw/arm/smmu-common.h
>> +++ b/include/hw/arm/smmu-common.h
>> @@ -162,6 +162,7 @@ struct SMMUState {
>>      uint8_t bus_num;
>>      PCIBus *primary_bus;
>>      bool smmu_per_bus; /* SMMU is specific to the primary_bus */
>> +    bool accel; /* SMMU has accelerator support */
> How about:
> "SMMU is in the HW-accelerated mode for stage-1 translation"
> ?
>
> Thanks
> Nicolin
>