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"?
> +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
