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

Wed Apr 24 01:28:46 PDT 2024

On 24/04/2024 09:01, Shameerali Kolothum Thodi wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Ryan Roberts <ryan.roberts at arm.com>
>> Sent: Tuesday, April 23, 2024 3:42 PM
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi at huawei.com>;
>> iommu at lists.linux.dev; linux-arm-kernel at lists.infradead.org
>> Cc: joro at 8bytes.org; jgg at nvidia.com; kevin.tian at intel.com;
>> nicolinc at nvidia.com; mshavit at google.com; robin.murphy at arm.com;
>> will at kernel.org; joao.m.martins at oracle.com; jiangkunkun
>> <jiangkunkun at huawei.com>; zhukeqian <zhukeqian1 at huawei.com>;
>> Linuxarm <linuxarm at 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 at 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 at linaro.org>
>>> Signed-off-by: Joao Martins <joao.m.martins at oracle.com>
>>> Reviewed-by: Jason Gunthorpe <jgg at nvidia.com>
>>> Signed-off-by: Shameer Kolothum
>> <shameerali.kolothum.thodi at huawei.com>
>>
>> Except for the nit (feel free to ignore it), LGTM!
>>
>> Reviewed-by: Ryan Roberts <ryan.roberts at 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. 

Ahh yes, I see that now.

> How about we rename to 
> ARM_SMMU_FEAT_HTTU_HA and ARM_SMMU_FEAT_HTTU_HA?

Yes, that's much more obvious from where I'm standing. But given HA and HD are
fields in the spec, I'm also fine with leaving as is. Up to you.

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