diff mbox series

[v2,4/4] iommu/arm-smmu-v3: Enable HTTU for stage1 with io-pgtable mapping

Message ID 20240222094923.33104-5-shameerali.kolothum.thodi@huawei.com (mailing list archive)
State New, archived
Headers show
Series iommu/smmuv3: Add IOMMUFD dirty tracking support for SMMUv3 | expand

Commit Message

Shameer Kolothum Feb. 22, 2024, 9:49 a.m. UTC 
From: Kunkun Jiang <jiangkunkun@huawei.com>

If io-pgtable quirk flag indicates support for hardware update of
dirty state, enable HA/HD bits in the SMMU CD and also set the DBM
bit in the page descriptor.

And now report the dirty page tracking capability of SMMUv3.

Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 +++++++++++++++
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  3 +++
 drivers/iommu/io-pgtable-arm.c              |  5 ++++-
 3 files changed, 22 insertions(+), 1 deletion(-)

Comments

Jason Gunthorpe March 8, 2024, 2:32 p.m. UTC | #1 
On Thu, Feb 22, 2024 at 09:49:23AM +0000, Shameer Kolothum wrote:
> From: Kunkun Jiang <jiangkunkun@huawei.com>
> 
> If io-pgtable quirk flag indicates support for hardware update of
> dirty state, enable HA/HD bits in the SMMU CD and also set the DBM
> bit in the page descriptor.
> 
> And now report the dirty page tracking capability of SMMUv3.
> 
> Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 +++++++++++++++
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  3 +++
>  drivers/iommu/io-pgtable-arm.c              |  5 ++++-
>  3 files changed, 22 insertions(+), 1 deletion(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason
Ryan Roberts April 23, 2024, 4:45 p.m. UTC | #2 
On 22/02/2024 09:49, Shameer Kolothum wrote:
> From: Kunkun Jiang <jiangkunkun@huawei.com>
> 
> If io-pgtable quirk flag indicates support for hardware update of
> dirty state, enable HA/HD bits in the SMMU CD and also set the DBM
> bit in the page descriptor.
> 
> And now report the dirty page tracking capability of SMMUv3.
> 
> Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

Section 3.13 of the spec states: "Where translation tables are shared between
CDs that contain the same ASID (within a translation regime), the CD HA and HD
fields must be identical."

I don't think the way that smmu domains work, its possible to end up with a
single pgtable shared between multiple CDs? So the driver should be able to
guarantee this constraint is met?

Assuming yes:

Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>


> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 +++++++++++++++
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  3 +++
>  drivers/iommu/io-pgtable-arm.c              |  5 ++++-
>  3 files changed, 22 insertions(+), 1 deletion(-)
> 
> 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 058bbb0dbe2e..4423cc7e48cf 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1271,6 +1271,12 @@ void arm_smmu_make_s1_cd(struct arm_smmu_cd *target,
>  		CTXDESC_CD_0_ASET |
>  		FIELD_PREP(CTXDESC_CD_0_ASID, smmu_domain->asid)
>  		);
> +
> +	/* To enable dirty flag update, set both Access flag and dirty state update */
> +	if (pgtbl_cfg->quirks & IO_PGTABLE_QUIRK_ARM_HD)
> +		target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_HA |
> +					       CTXDESC_CD_0_TCR_HD);
> +
>  	target->data[1] = cpu_to_le64(pgtbl_cfg->arm_lpae_s1_cfg.ttbr &
>  				      CTXDESC_CD_1_TTB0_MASK);
>  	target->data[3] = cpu_to_le64(pgtbl_cfg->arm_lpae_s1_cfg.mair);
> @@ -2307,6 +2313,13 @@ static const struct iommu_flush_ops arm_smmu_flush_ops = {
>  	.tlb_add_page	= arm_smmu_tlb_inv_page_nosync,
>  };
>  
> +static bool arm_smmu_dbm_capable(struct arm_smmu_device *smmu)
> +{
> +	u32 features = (ARM_SMMU_FEAT_HD | ARM_SMMU_FEAT_COHERENCY);
> +
> +	return (smmu->features & features) == features;
> +}
> +
>  /* IOMMU API */
>  static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap)
>  {
> @@ -2319,6 +2332,8 @@ static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap)
>  	case IOMMU_CAP_NOEXEC:
>  	case IOMMU_CAP_DEFERRED_FLUSH:
>  		return true;
> +	case IOMMU_CAP_DIRTY_TRACKING:
> +		return arm_smmu_dbm_capable(master->smmu);
>  	default:
>  		return false;
>  	}
> 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 5e51a6c1d55f..a9cd805e5a1b 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -313,6 +313,9 @@ struct arm_smmu_cd {
>  #define CTXDESC_CD_0_TCR_IPS		GENMASK_ULL(34, 32)
>  #define CTXDESC_CD_0_TCR_TBI0		(1ULL << 38)
>  
> +#define CTXDESC_CD_0_TCR_HA            (1UL << 43)
> +#define CTXDESC_CD_0_TCR_HD            (1UL << 42)
> +
>  #define CTXDESC_CD_0_AA64		(1UL << 41)
>  #define CTXDESC_CD_0_S			(1UL << 44)
>  #define CTXDESC_CD_0_R			(1UL << 45)
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 1ce7b7a3c1e8..56e88e555fc7 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -429,6 +429,8 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
>  		pte = ARM_LPAE_PTE_nG;
>  		if (!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
>  			pte |= ARM_LPAE_PTE_AP_RDONLY;
> +		else if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_ARM_HD)
> +			pte |= ARM_LPAE_PTE_AP_WRITABLE;
>  		if (!(prot & IOMMU_PRIV))
>  			pte |= ARM_LPAE_PTE_AP_UNPRIV;
>  	} else {
> @@ -948,7 +950,8 @@ arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg *cfg, void *cookie)
>  
>  	if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS |
>  			    IO_PGTABLE_QUIRK_ARM_TTBR1 |
> -			    IO_PGTABLE_QUIRK_ARM_OUTER_WBWA))
> +			    IO_PGTABLE_QUIRK_ARM_OUTER_WBWA |
> +			    IO_PGTABLE_QUIRK_ARM_HD))
>  		return NULL;
>  
>  	data = arm_lpae_alloc_pgtable(cfg);
Jason Gunthorpe April 23, 2024, 5:32 p.m. UTC | #3 
On Tue, Apr 23, 2024 at 05:45:16PM +0100, Ryan Roberts wrote:
> On 22/02/2024 09:49, Shameer Kolothum wrote:
> > From: Kunkun Jiang <jiangkunkun@huawei.com>
> > 
> > If io-pgtable quirk flag indicates support for hardware update of
> > dirty state, enable HA/HD bits in the SMMU CD and also set the DBM
> > bit in the page descriptor.
> > 
> > And now report the dirty page tracking capability of SMMUv3.
> > 
> > Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
> > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> > Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
> > Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> 
> Section 3.13 of the spec states: "Where translation tables are shared between
> CDs that contain the same ASID (within a translation regime), the CD HA and HD
> fields must be identical."
> 
> I don't think the way that smmu domains work, its possible to end up with a
> single pgtable shared between multiple CDs? 

It is possible. iommufd can link a single hwpt -> iommu_domain ->
smmu_domain to many different RIDs and to different PASIDs each with
their own CD.

> So the driver should be able to guarantee this constraint is met?

It is expected to be done by this:

> > @@ -1271,6 +1271,12 @@ void arm_smmu_make_s1_cd(struct arm_smmu_cd *target,
> >  		CTXDESC_CD_0_ASET |
> >  		FIELD_PREP(CTXDESC_CD_0_ASID, smmu_domain->asid)
> >  		);
> > +
> > +	/* To enable dirty flag update, set both Access flag and dirty state update */
> > +	if (pgtbl_cfg->quirks & IO_PGTABLE_QUIRK_ARM_HD)
> > +		target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_HA |
> > +					       CTXDESC_CD_0_TCR_HD);
> > +

This function is the only place that programs the ASID into a CD entry
for the domain, and it always derives the HA/HD bits in the same way
from some immutable value stored in the iommu_domain structure.

Jason
Ryan Roberts April 24, 2024, 7:58 a.m. UTC | #4 
On 23/04/2024 18:32, Jason Gunthorpe wrote:
> On Tue, Apr 23, 2024 at 05:45:16PM +0100, Ryan Roberts wrote:
>> On 22/02/2024 09:49, Shameer Kolothum wrote:
>>> From: Kunkun Jiang <jiangkunkun@huawei.com>
>>>
>>> If io-pgtable quirk flag indicates support for hardware update of
>>> dirty state, enable HA/HD bits in the SMMU CD and also set the DBM
>>> bit in the page descriptor.
>>>
>>> And now report the dirty page tracking capability of SMMUv3.
>>>
>>> Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
>>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>>> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>>
>> Section 3.13 of the spec states: "Where translation tables are shared between
>> CDs that contain the same ASID (within a translation regime), the CD HA and HD
>> fields must be identical."
>>
>> I don't think the way that smmu domains work, its possible to end up with a
>> single pgtable shared between multiple CDs? 
> 
> It is possible. iommufd can link a single hwpt -> iommu_domain ->
> smmu_domain to many different RIDs and to different PASIDs each with
> their own CD.

Ahh ok.

> 
>> So the driver should be able to guarantee this constraint is met?
> 
> It is expected to be done by this:
> 
>>> @@ -1271,6 +1271,12 @@ void arm_smmu_make_s1_cd(struct arm_smmu_cd *target,
>>>  		CTXDESC_CD_0_ASET |
>>>  		FIELD_PREP(CTXDESC_CD_0_ASID, smmu_domain->asid)
>>>  		);
>>> +
>>> +	/* To enable dirty flag update, set both Access flag and dirty state update */
>>> +	if (pgtbl_cfg->quirks & IO_PGTABLE_QUIRK_ARM_HD)
>>> +		target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_HA |
>>> +					       CTXDESC_CD_0_TCR_HD);
>>> +
> 
> This function is the only place that programs the ASID into a CD entry
> for the domain, and it always derives the HA/HD bits in the same way
> from some immutable value stored in the iommu_domain structure.

But then I don't understand how this code can make the guarrantee? Whether or
not dirty tracking is enabled seems to be a property of the domain, not the page
table. (See arm_smmu_domain_finalise()). So if multiple domains can share a page
table surely some of those domains could end up with dirty tracking on and
others with it off. So there would be multiple CDs, all pointing to the same
pgtable but with different HA/HD values?

Or perhaps I've misunderstood your comment and there is actually always a 1:1
relationship between domain and pgtable? But a 1:* relationship between domain
and CD?

> 
> Jason
Jason Gunthorpe April 24, 2024, 12:15 p.m. UTC | #5 
On Wed, Apr 24, 2024 at 08:58:41AM +0100, Ryan Roberts wrote:

> Or perhaps I've misunderstood your comment and there is actually always a 1:1
> relationship between domain and pgtable? But a 1:* relationship between domain
> and CD?

Yes, this is the case

Jason
Ryan Roberts April 24, 2024, 12:45 p.m. UTC | #6 
On 24/04/2024 13:15, Jason Gunthorpe wrote:
> On Wed, Apr 24, 2024 at 08:58:41AM +0100, Ryan Roberts wrote:
> 
>> Or perhaps I've misunderstood your comment and there is actually always a 1:1
>> relationship between domain and pgtable? But a 1:* relationship between domain
>> and CD?
> 
> Yes, this is the case

Great thanks! See - I said I'd be asking stupid questions...

> 
> Jason
diff mbox series

Patch

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 058bbb0dbe2e..4423cc7e48cf 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1271,6 +1271,12 @@  void arm_smmu_make_s1_cd(struct arm_smmu_cd *target,
 		CTXDESC_CD_0_ASET |
 		FIELD_PREP(CTXDESC_CD_0_ASID, smmu_domain->asid)
 		);
+
+	/* To enable dirty flag update, set both Access flag and dirty state update */
+	if (pgtbl_cfg->quirks & IO_PGTABLE_QUIRK_ARM_HD)
+		target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_HA |
+					       CTXDESC_CD_0_TCR_HD);
+
 	target->data[1] = cpu_to_le64(pgtbl_cfg->arm_lpae_s1_cfg.ttbr &
 				      CTXDESC_CD_1_TTB0_MASK);
 	target->data[3] = cpu_to_le64(pgtbl_cfg->arm_lpae_s1_cfg.mair);
@@ -2307,6 +2313,13 @@  static const struct iommu_flush_ops arm_smmu_flush_ops = {
 	.tlb_add_page	= arm_smmu_tlb_inv_page_nosync,
 };
 
+static bool arm_smmu_dbm_capable(struct arm_smmu_device *smmu)
+{
+	u32 features = (ARM_SMMU_FEAT_HD | ARM_SMMU_FEAT_COHERENCY);
+
+	return (smmu->features & features) == features;
+}
+
 /* IOMMU API */
 static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap)
 {
@@ -2319,6 +2332,8 @@  static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap)
 	case IOMMU_CAP_NOEXEC:
 	case IOMMU_CAP_DEFERRED_FLUSH:
 		return true;
+	case IOMMU_CAP_DIRTY_TRACKING:
+		return arm_smmu_dbm_capable(master->smmu);
 	default:
 		return false;
 	}
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 5e51a6c1d55f..a9cd805e5a1b 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -313,6 +313,9 @@  struct arm_smmu_cd {
 #define CTXDESC_CD_0_TCR_IPS		GENMASK_ULL(34, 32)
 #define CTXDESC_CD_0_TCR_TBI0		(1ULL << 38)
 
+#define CTXDESC_CD_0_TCR_HA            (1UL << 43)
+#define CTXDESC_CD_0_TCR_HD            (1UL << 42)
+
 #define CTXDESC_CD_0_AA64		(1UL << 41)
 #define CTXDESC_CD_0_S			(1UL << 44)
 #define CTXDESC_CD_0_R			(1UL << 45)
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 1ce7b7a3c1e8..56e88e555fc7 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -429,6 +429,8 @@  static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
 		pte = ARM_LPAE_PTE_nG;
 		if (!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
 			pte |= ARM_LPAE_PTE_AP_RDONLY;
+		else if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_ARM_HD)
+			pte |= ARM_LPAE_PTE_AP_WRITABLE;
 		if (!(prot & IOMMU_PRIV))
 			pte |= ARM_LPAE_PTE_AP_UNPRIV;
 	} else {
@@ -948,7 +950,8 @@  arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg *cfg, void *cookie)
 
 	if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS |
 			    IO_PGTABLE_QUIRK_ARM_TTBR1 |
-			    IO_PGTABLE_QUIRK_ARM_OUTER_WBWA))
+			    IO_PGTABLE_QUIRK_ARM_OUTER_WBWA |
+			    IO_PGTABLE_QUIRK_ARM_HD))
 		return NULL;
 
 	data = arm_lpae_alloc_pgtable(cfg);