RE: [PATCH v2 1/4] iommu/arm-smmu-v3: Add feature detection for HTTU

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

Hi,

> -----Original Message-----
> From: Ryan Roberts <ryan.roberts@arm.com>
> Sent: Tuesday, April 23, 2024 3:42 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> iommu@lists.linux.dev; linux-arm-kernel@lists.infradead.org
> Cc: joro@8bytes.org; jgg@nvidia.com; kevin.tian@intel.com;
> nicolinc@nvidia.com; mshavit@google.com; robin.murphy@arm.com;
> will@kernel.org; joao.m.martins@oracle.com; jiangkunkun
> <jiangkunkun@huawei.com>; zhukeqian <zhukeqian1@huawei.com>;
> Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH v2 1/4] iommu/arm-smmu-v3: Add feature detection for
> HTTU
> 
> Hi,
> 
> I'm aiming to (slowly) get more involved with SMMU activities, although I'm
> sure
> it will take a while to get up to speed and provide useful input. It was
> suggested that this series would be a useful starting point to dip my toe in.
> Please bear with me while I ask stupid questions...

Thanks for going through the series. I am planning to respin this one soon, once
Jason's SMMUv3 refactor series in some good form.

> 
> 
> On 22/02/2024 09:49, Shameer Kolothum wrote:
> > From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> >
> > If the SMMU supports it and the kernel was built with HTTU support,
> > Probe support for Hardware Translation Table Update (HTTU) which is
> > essentially to enable hardware update of access and dirty flags.
> >
> > Probe and set the smmu::features for Hardware Dirty and Hardware Access
> > bits. This is in preparation, to enable it on the context descriptors of
> > stage 1 format.
> >
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> 
> Except for the nit (feel free to ignore it), LGTM!
> 
> Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>
> 
> 
> > ---
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 32
> +++++++++++++++++++++
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  5 ++++
> >  2 files changed, 37 insertions(+)
> >
> > 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 0606166a8781..bd30739e3588 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -4196,6 +4196,28 @@ static void arm_smmu_device_iidr_probe(struct
> arm_smmu_device *smmu)
> >  	}
> >  }
> >
> > +static void arm_smmu_get_httu(struct arm_smmu_device *smmu, u32
> reg)
> > +{
> > +	u32 fw_features = smmu->features & (ARM_SMMU_FEAT_HA |
> ARM_SMMU_FEAT_HD);
> > +	u32 features = 0;
> > +
> > +	switch (FIELD_GET(IDR0_HTTU, reg)) {
> > +	case IDR0_HTTU_ACCESS_DIRTY:
> > +		features |= ARM_SMMU_FEAT_HD;
> > +		fallthrough;
> > +	case IDR0_HTTU_ACCESS:
> > +		features |= ARM_SMMU_FEAT_HA;
> > +	}
> > +
> > +	if (smmu->dev->of_node)
> > +		smmu->features |= features;
> > +	else if (features != fw_features)
> > +		/* ACPI IORT sets the HTTU bits */
> > +		dev_warn(smmu->dev,
> > +			 "IDR0.HTTU overridden by FW configuration
> (0x%x)\n",
> > +			 fw_features);
> > +}
> > +
> >  static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
> >  {
> >  	u32 reg;
> > @@ -4256,6 +4278,8 @@ static int arm_smmu_device_hw_probe(struct
> arm_smmu_device *smmu)
> >  			smmu->features |= ARM_SMMU_FEAT_E2H;
> >  	}
> >
> > +	arm_smmu_get_httu(smmu, reg);
> > +
> >  	/*
> >  	 * The coherency feature as set by FW is used in preference to the ID
> >  	 * register, but warn on mismatch.
> > @@ -4448,6 +4472,14 @@ static int arm_smmu_device_acpi_probe(struct
> platform_device *pdev,
> >  	if (iort_smmu->flags & ACPI_IORT_SMMU_V3_COHACC_OVERRIDE)
> >  		smmu->features |= ARM_SMMU_FEAT_COHERENCY;
> >
> > +	switch (FIELD_GET(ACPI_IORT_SMMU_V3_HTTU_OVERRIDE,
> iort_smmu->flags)) {
> > +	case IDR0_HTTU_ACCESS_DIRTY:
> > +		smmu->features |= ARM_SMMU_FEAT_HD;
> > +		fallthrough;
> > +	case IDR0_HTTU_ACCESS:
> > +		smmu->features |= ARM_SMMU_FEAT_HA;
> > +	}
> > +
> >  	return 0;
> >  }
> >  #else
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > index 45bcd72fcda4..5e51a6c1d55f 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > @@ -33,6 +33,9 @@
> >  #define IDR0_ASID16			(1 << 12)
> >  #define IDR0_ATS			(1 << 10)
> >  #define IDR0_HYP			(1 << 9)
> > +#define IDR0_HTTU			GENMASK(7, 6)
> > +#define IDR0_HTTU_ACCESS		1
> > +#define IDR0_HTTU_ACCESS_DIRTY		2
> >  #define IDR0_COHACC			(1 << 4)
> >  #define IDR0_TTF			GENMASK(3, 2)
> >  #define IDR0_TTF_AARCH64		2
> > @@ -668,6 +671,8 @@ struct arm_smmu_device {
> >  #define ARM_SMMU_FEAT_SVA		(1 << 17)
> >  #define ARM_SMMU_FEAT_E2H		(1 << 18)
> >  #define ARM_SMMU_FEAT_NESTING		(1 << 19)
> > +#define ARM_SMMU_FEAT_HA		(1 << 20)
> > +#define ARM_SMMU_FEAT_HD		(1 << 21)
> 
> nit: HA and HD are a bit opaque, at least to me. I guess they are HW Access
> and
> HW Dirty? Perhaps ARM_SMMU_FEAT_HW_ACCESS and
> ARM_SMMU_FEAT_HW_DIRTY are more
> expressive?

Nothing against it. Just that _HW_ is not used in driver anywhere(AFAICS)
and HA/HD are used in the specification. How about we rename to 
ARM_SMMU_FEAT_HTTU_HA and ARM_SMMU_FEAT_HTTU_HA?

Thanks,
Shameer

> 
> >  	u32				features;
> >
> >  #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
>