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.th...@huawei.com>
> Overall the patch looks good to me,
> Reviewed-by: Nicolin Chen <nicol...@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
>
