[PATCH v5 08/27] iommu/arm-smmu-v3: Move allocation of the cdtable into arm_smmu_get_cd_ptr()
Mostafa Saleh
smostafa at google.com
Fri Mar 22 12:07:10 PDT 2024
Hi Jason,
On Mon, Mar 04, 2024 at 07:43:56PM -0400, Jason Gunthorpe wrote:
> No reason to force callers to do two steps. Make arm_smmu_get_cd_ptr()
> able to return an entry in all cases except OOM
I believe the current code is more clear, as it is explicit about which path
is expected to allocate.
As there are many callers for arm_smmu_get_cd_ptr() directly and indirectly,
and it read-modify-writes the cdtable, it would be a pain to debug not
knowing which one could allocate, and this patch only abstracts one
allocating call, so it is not much code less.
For example, (again I don’t know much about SVA) I think there might be a
race condition as follows:
arm_smmu_attach_dev
arm_smmu_domain_finalise() => set domain stage
[....]
arm_smmu_get_cd_ptr() => RMW master->cd_table
arm_smmu_sva_set_dev_pasid
__arm_smmu_sva_bind
Check stage is valid
[...]
arm_smmu_write_ctx_desc
arm_smmu_get_cd_ptr => RMW master->cd_table
If this path is true though, I guess the in current code, we would need some
barriers in arm_smmu_get_cd_ptr(), arm_smmu_get_cd_ptr()
> Tested-by: Nicolin Chen <nicolinc at nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg at nvidia.com>
> ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index e25dbb982feeee..2dd6cb17112e98 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -106,6 +106,7 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
>
> static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
> struct arm_smmu_device *smmu);
> +static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
>
> static void parse_driver_options(struct arm_smmu_device *smmu)
> {
> @@ -1231,6 +1232,11 @@ struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master,
> struct arm_smmu_device *smmu = master->smmu;
> struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table;
>
> + if (!master->cd_table.cdtab) {
> + if (arm_smmu_alloc_cd_tables(master))
> + return NULL;
> + }
> +
> if (cd_table->s1fmt == STRTAB_STE_0_S1FMT_LINEAR)
> return (struct arm_smmu_cd *)(cd_table->cdtab +
> ssid * CTXDESC_CD_DWORDS);
> @@ -2719,12 +2725,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> struct arm_smmu_cd target_cd;
> struct arm_smmu_cd *cdptr;
>
> - if (!master->cd_table.cdtab) {
> - ret = arm_smmu_alloc_cd_tables(master);
> - if (ret)
> - goto out_list_del;
> - }
> -
> cdptr = arm_smmu_get_cd_ptr(master, IOMMU_NO_PASID);
> if (!cdptr) {
> ret = -ENOMEM;
> --
> 2.43.2
>
Thanks,
Mostafa
More information about the linux-arm-kernel
mailing list