[PATCH 0/2] hw/arm/smmu: Fixes for TTB1

[PATCH 0/2] hw/arm/smmu: Fixes for TTB1

[Jean-Philippe Brucker]

Two small changes to support TTB1. Note that I had to modify the Linux
driver in order to test this (see below), but other OSes might use TTB1.

Jean-Philippe Brucker (2):
  hw/arm/smmu-common: Support 64-bit addresses
  hw/arm/smmu-common: Fix TTB1 handling

 hw/arm/smmu-common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


--- 8< --- Linux hacks:
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 8d772ea8a583..bf0ff699b64b 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -276,6 +276,11 @@
 #define CTXDESC_CD_0_TCR_IRGN0		GENMASK_ULL(9, 8)
 #define CTXDESC_CD_0_TCR_ORGN0		GENMASK_ULL(11, 10)
 #define CTXDESC_CD_0_TCR_SH0		GENMASK_ULL(13, 12)
+#define CTXDESC_CD_0_TCR_T1SZ		GENMASK_ULL(21, 16)
+#define CTXDESC_CD_0_TCR_TG1		GENMASK_ULL(23, 22)
+#define CTXDESC_CD_0_TCR_IRGN1		GENMASK_ULL(25, 24)
+#define CTXDESC_CD_0_TCR_ORGN1		GENMASK_ULL(27, 26)
+#define CTXDESC_CD_0_TCR_SH1		GENMASK_ULL(29, 28)
 #define CTXDESC_CD_0_TCR_EPD0		(1ULL << 14)
 #define CTXDESC_CD_0_TCR_EPD1		(1ULL << 30)

@@ -293,6 +298,7 @@
 #define CTXDESC_CD_0_ASID		GENMASK_ULL(63, 48)

 #define CTXDESC_CD_1_TTB0_MASK		GENMASK_ULL(51, 4)
+#define CTXDESC_CD_2_TTB1_MASK		GENMASK_ULL(51, 4)

 /*
  * When the SMMU only supports linear context descriptor tables, pick a
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 f2425b0f0cd6..3a4343e60a54 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1075,8 +1075,8 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_domain *smmu_domain, int ssid,
 		 * this substream's traffic
 		 */
 	} else { /* (1) and (2) */
-		cdptr[1] = cpu_to_le64(cd->ttbr & CTXDESC_CD_1_TTB0_MASK);
-		cdptr[2] = 0;
+		cdptr[1] = 0;
+		cdptr[2] = cpu_to_le64(cd->ttbr & CTXDESC_CD_2_TTB1_MASK);
 		cdptr[3] = cpu_to_le64(cd->mair);

 		/*
@@ -2108,13 +2108,13 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,

 	cfg->cd.asid	= (u16)asid;
 	cfg->cd.ttbr	= pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
-	cfg->cd.tcr	= FIELD_PREP(CTXDESC_CD_0_TCR_T0SZ, tcr->tsz) |
-			  FIELD_PREP(CTXDESC_CD_0_TCR_TG0, tcr->tg) |
-			  FIELD_PREP(CTXDESC_CD_0_TCR_IRGN0, tcr->irgn) |
-			  FIELD_PREP(CTXDESC_CD_0_TCR_ORGN0, tcr->orgn) |
-			  FIELD_PREP(CTXDESC_CD_0_TCR_SH0, tcr->sh) |
+	cfg->cd.tcr	= FIELD_PREP(CTXDESC_CD_0_TCR_T1SZ, tcr->tsz) |
+			  FIELD_PREP(CTXDESC_CD_0_TCR_TG1, tcr->tg) |
+			  FIELD_PREP(CTXDESC_CD_0_TCR_IRGN1, tcr->irgn) |
+			  FIELD_PREP(CTXDESC_CD_0_TCR_ORGN1, tcr->orgn) |
+			  FIELD_PREP(CTXDESC_CD_0_TCR_SH1, tcr->sh) |
 			  FIELD_PREP(CTXDESC_CD_0_TCR_IPS, tcr->ips) |
-			  CTXDESC_CD_0_TCR_EPD1 | CTXDESC_CD_0_AA64;
+			  CTXDESC_CD_0_TCR_EPD0 | CTXDESC_CD_0_AA64;
 	cfg->cd.mair	= pgtbl_cfg->arm_lpae_s1_cfg.mair;

 	/*
@@ -2212,6 +2212,7 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain,
 		.pgsize_bitmap	= smmu->pgsize_bitmap,
 		.ias		= ias,
 		.oas		= oas,
+		.quirks		= IO_PGTABLE_QUIRK_ARM_TTBR1,
 		.coherent_walk	= smmu->features & ARM_SMMU_FEAT_COHERENCY,
 		.tlb		= &arm_smmu_flush_ops,
 		.iommu_dev	= smmu->dev,
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 38434203bf04..3fe154c9782d 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -677,6 +677,10 @@ static int dma_info_to_prot(enum dma_data_direction dir, bool coherent,
 	}
 }

+/* HACK */
+#define VA_SIZE		39
+#define VA_MASK		(~0ULL << VA_SIZE)
+
 static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
 		size_t size, u64 dma_limit, struct device *dev)
 {
@@ -706,7 +710,7 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
 		iova = alloc_iova_fast(iovad, iova_len, dma_limit >> shift,
 				       true);

-	return (dma_addr_t)iova << shift;
+	return (dma_addr_t)iova << shift | VA_MASK;
 }

 static void iommu_dma_free_iova(struct iommu_dma_cookie *cookie,
@@ -714,6 +718,7 @@ static void iommu_dma_free_iova(struct iommu_dma_cookie *cookie,
 {
 	struct iova_domain *iovad = &cookie->iovad;

+	iova &= ~VA_MASK;
 	/* The MSI case is only ever cleaning up its most recent allocation */
 	if (cookie->type == IOMMU_DMA_MSI_COOKIE)
 		cookie->msi_iova -= size;

[PATCH 1/2] hw/arm/smmu-common: Support 64-bit addresses

[Jean-Philippe Brucker]

Addresses targeting the second translation table (TTB1) in the SMMU have
all upper bits set. Ensure the IOMMU region covers all 64 bits.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/smmu-common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 733c964778..2b8c67b9a1 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -439,7 +439,7 @@ static AddressSpace *smmu_find_add_as(PCIBus *bus, void *opaque, int devfn)
 
         memory_region_init_iommu(&sdev->iommu, sizeof(sdev->iommu),
                                  s->mrtypename,
-                                 OBJECT(s), name, 1ULL << SMMU_MAX_VA_BITS);
+                                 OBJECT(s), name, UINT64_MAX);
         address_space_init(&sdev->as,
                            MEMORY_REGION(&sdev->iommu), name);
         trace_smmu_add_mr(name);
-- 
2.39.0

[PATCH 2/2] hw/arm/smmu-common: Fix TTB1 handling

[Jean-Philippe Brucker]

Addresses targeting the second translation table (TTB1) in the SMMU have
all upper bits set (except for the top byte when TBI is enabled). Fix
the TTB1 check.

Reported-by: Ola Hugosson <ola.hugosson@arm.com>
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/smmu-common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 2b8c67b9a1..0a5a60ca1e 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -249,7 +249,7 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
         /* there is a ttbr0 region and we are in it (high bits all zero) */
         return &cfg->tt[0];
     } else if (cfg->tt[1].tsz &&
-           !extract64(iova, 64 - cfg->tt[1].tsz, cfg->tt[1].tsz - tbi_byte)) {
+        sextract64(iova, 64 - cfg->tt[1].tsz, cfg->tt[1].tsz - tbi_byte) == -1) {
         /* there is a ttbr1 region and we are in it (high bits all one) */
         return &cfg->tt[1];
     } else if (!cfg->tt[0].tsz) {
-- 
2.39.0

Re: [PATCH 1/2] hw/arm/smmu-common: Support 64-bit addresses

[Richard Henderson]

On 2/10/23 06:37, Jean-Philippe Brucker wrote:
> Addresses targeting the second translation table (TTB1) in the SMMU have
> all upper bits set. Ensure the IOMMU region covers all 64 bits.
> 
> Signed-off-by: Jean-Philippe Brucker<jean-philippe@linaro.org>
> ---
>   hw/arm/smmu-common.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 2/2] hw/arm/smmu-common: Fix TTB1 handling

[Richard Henderson]

On 2/10/23 06:37, Jean-Philippe Brucker wrote:
> Addresses targeting the second translation table (TTB1) in the SMMU have
> all upper bits set (except for the top byte when TBI is enabled). Fix
> the TTB1 check.
> 
> Reported-by: Ola Hugosson<ola.hugosson@arm.com>
> Signed-off-by: Jean-Philippe Brucker<jean-philippe@linaro.org>
> ---
>   hw/arm/smmu-common.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 1/2] hw/arm/smmu-common: Support 64-bit addresses

[Eric Auger]

Hi Jean,
On 2/10/23 17:37, Jean-Philippe Brucker wrote:
> Addresses targeting the second translation table (TTB1) in the SMMU have
> all upper bits set. Ensure the IOMMU region covers all 64 bits.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  hw/arm/smmu-common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 733c964778..2b8c67b9a1 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -439,7 +439,7 @@ static AddressSpace *smmu_find_add_as(PCIBus *bus, void *opaque, int devfn)
>  
>          memory_region_init_iommu(&sdev->iommu, sizeof(sdev->iommu),
>                                   s->mrtypename,
> -                                 OBJECT(s), name, 1ULL << SMMU_MAX_VA_BITS);
> +                                 OBJECT(s), name, UINT64_MAX);
as SMMU_MAX_VA_BITS is not used anymore, please remove it.
>          address_space_init(&sdev->as,
>                             MEMORY_REGION(&sdev->iommu), name);
>          trace_smmu_add_mr(name);
Thanks

Eric

Re: [PATCH 2/2] hw/arm/smmu-common: Fix TTB1 handling

[Eric Auger]

Hi Jean,

On 2/10/23 17:37, Jean-Philippe Brucker wrote:
> Addresses targeting the second translation table (TTB1) in the SMMU have
> all upper bits set (except for the top byte when TBI is enabled). Fix
> the TTB1 check.
>
> Reported-by: Ola Hugosson <ola.hugosson@arm.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  hw/arm/smmu-common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 2b8c67b9a1..0a5a60ca1e 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -249,7 +249,7 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
>          /* there is a ttbr0 region and we are in it (high bits all zero) */
>          return &cfg->tt[0];
>      } else if (cfg->tt[1].tsz &&
> -           !extract64(iova, 64 - cfg->tt[1].tsz, cfg->tt[1].tsz - tbi_byte)) {
> +        sextract64(iova, 64 - cfg->tt[1].tsz, cfg->tt[1].tsz - tbi_byte) == -1) {
>          /* there is a ttbr1 region and we are in it (high bits all one) */
>          return &cfg->tt[1];
>      } else if (!cfg->tt[0].tsz) {

Reviewed-by: Eric Auger <eric.auger@redhat.com>

While reading the spec again, I noticed we do not support VAX. Is it
something that we would need to support?

Thanks!

Eric

Re: [PATCH 2/2] hw/arm/smmu-common: Fix TTB1 handling

[Jean-Philippe Brucker]

On Mon, Feb 13, 2023 at 05:30:03PM +0100, Eric Auger wrote:
> Hi Jean,
> 
> On 2/10/23 17:37, Jean-Philippe Brucker wrote:
> > Addresses targeting the second translation table (TTB1) in the SMMU have
> > all upper bits set (except for the top byte when TBI is enabled). Fix
> > the TTB1 check.
> >
> > Reported-by: Ola Hugosson <ola.hugosson@arm.com>
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> >  hw/arm/smmu-common.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> > index 2b8c67b9a1..0a5a60ca1e 100644
> > --- a/hw/arm/smmu-common.c
> > +++ b/hw/arm/smmu-common.c
> > @@ -249,7 +249,7 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
> >          /* there is a ttbr0 region and we are in it (high bits all zero) */
> >          return &cfg->tt[0];
> >      } else if (cfg->tt[1].tsz &&
> > -           !extract64(iova, 64 - cfg->tt[1].tsz, cfg->tt[1].tsz - tbi_byte)) {
> > +        sextract64(iova, 64 - cfg->tt[1].tsz, cfg->tt[1].tsz - tbi_byte) == -1) {
> >          /* there is a ttbr1 region and we are in it (high bits all one) */
> >          return &cfg->tt[1];
> >      } else if (!cfg->tt[0].tsz) {
> 
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> 
> While reading the spec again, I noticed we do not support VAX. Is it
> something that we would need to support?

I guess it would be needed to support sharing page tables with the CPU, if
the CPU supports and the OS uses FEAT_LVA. But in order to share the
stage-1, Linux would need more complex features as well (ATS+PRI/Stall,
PASID).

For a private DMA address space, I think 48 bits of VA is already plenty.

Thanks,
Jean

Re: [PATCH 2/2] hw/arm/smmu-common: Fix TTB1 handling

[Eric Auger]

On 2/14/23 17:46, Jean-Philippe Brucker wrote:
> On Mon, Feb 13, 2023 at 05:30:03PM +0100, Eric Auger wrote:
>> Hi Jean,
>>
>> On 2/10/23 17:37, Jean-Philippe Brucker wrote:
>>> Addresses targeting the second translation table (TTB1) in the SMMU have
>>> all upper bits set (except for the top byte when TBI is enabled). Fix
>>> the TTB1 check.
>>>
>>> Reported-by: Ola Hugosson <ola.hugosson@arm.com>
>>> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
>>> ---
>>>  hw/arm/smmu-common.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
>>> index 2b8c67b9a1..0a5a60ca1e 100644
>>> --- a/hw/arm/smmu-common.c
>>> +++ b/hw/arm/smmu-common.c
>>> @@ -249,7 +249,7 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
>>>          /* there is a ttbr0 region and we are in it (high bits all zero) */
>>>          return &cfg->tt[0];
>>>      } else if (cfg->tt[1].tsz &&
>>> -           !extract64(iova, 64 - cfg->tt[1].tsz, cfg->tt[1].tsz - tbi_byte)) {
>>> +        sextract64(iova, 64 - cfg->tt[1].tsz, cfg->tt[1].tsz - tbi_byte) == -1) {
>>>          /* there is a ttbr1 region and we are in it (high bits all one) */
>>>          return &cfg->tt[1];
>>>      } else if (!cfg->tt[0].tsz) {
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>>
>> While reading the spec again, I noticed we do not support VAX. Is it
>> something that we would need to support?
> I guess it would be needed to support sharing page tables with the CPU, if
> the CPU supports and the OS uses FEAT_LVA. But in order to share the
> stage-1, Linux would need more complex features as well (ATS+PRI/Stall,
> PASID).
>
> For a private DMA address space, I think 48 bits of VA is already plenty.

OK thanks!

Eric
>
> Thanks,
> Jean
>