[PATCH v4 0/3] Set SMMU s2 input-size based on p2m tables

[PATCH v4 0/3] Set SMMU s2 input-size based on p2m tables

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Hi,

This is a fix for the issue I'm seeing on ZynqMP with missmatched
setup of the SMMU and the shared p2m page-tables with the CPU.

This implementes a global p2m_ipa_bits cap for S2 input-size as
discussed in the previous RFC.

Best regards,
Edgar

Changelog:
v3 -> v4:
* Replace ASSERT on supported IPA sizes with returnig error.
* Remove redundant 'addresses' after IPA.

v2 -> v3:
* pfn -> ipa.
* Fix typos in commit msg for 3/3.

v1 -> v2:
* Use a global pfn bitsize instead of a per-domain one.

Edgar E. Iglesias (3):
  xen/arm: Re-order iommu_setup to after setup_virt_paging
  xen/arm: Add p2m_ipa_bits
  xen/iommu: arm: Use p2m_ipa_bits as stage2 input size

 xen/arch/arm/p2m.c                 |  5 +++++
 xen/arch/arm/setup.c               |  4 ++--
 xen/drivers/passthrough/arm/smmu.c | 10 ++++++++--
 xen/include/asm-arm/p2m.h          |  3 +++
 4 files changed, 18 insertions(+), 4 deletions(-)

-- 
1.9.1

[PATCH v4 1/3] xen/arm: Re-order iommu_setup to after setup_virt_paging

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

This is needed to allow the paging setup to probe for
IPA bit sizes to be used in p2m tables prior to iommu setup.

Reviewed-by: Julien Grall <julien.grall@citrix.com>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 xen/arch/arm/setup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 4ec7c13..797673b 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -800,8 +800,6 @@ void __init start_xen(unsigned long boot_phys_offset,
     local_irq_enable();
     local_abort_enable();
 
-    iommu_setup();
-
     smp_prepare_cpus(cpus);
 
     initialize_keytable();
@@ -825,6 +823,8 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     setup_virt_paging();
 
+    iommu_setup();
+
     do_initcalls();
 
     /* Create initial domain 0. */
-- 
1.9.1

Re: [PATCH v4 1/3] xen/arm: Re-order iommu_setup to after setup_virt_paging

[Ian Campbell]

On Fri, 2015-05-01 at 11:40 +1000, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> This is needed to allow the paging setup to probe for
> IPA bit sizes to be used in p2m tables prior to iommu setup.
> 
> Reviewed-by: Julien Grall <julien.grall@citrix.com>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

This seems like a more logical ordering in any case:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH v4 2/3] xen/arm: Add p2m_ipa_bits

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Export p2m_ipa_bits holding the bit size of IPAs used in p2m tables.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 xen/arch/arm/p2m.c        | 5 +++++
 xen/include/asm-arm/p2m.h | 3 +++
 2 files changed, 8 insertions(+)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 8dfee24..c6507ae 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -27,6 +27,9 @@ static unsigned int __read_mostly p2m_root_level;
 
 #define P2M_ROOT_PAGES    (1<<P2M_ROOT_ORDER)
 
+/* Size of IPAs in bits.  */
+unsigned int p2m_ipa_bits;
+
 static bool_t p2m_valid(lpae_t pte)
 {
     return pte.p2m.valid;
@@ -1515,6 +1518,7 @@ void __init setup_virt_paging(void)
 
 #ifdef CONFIG_ARM_32
     printk("P2M: 40-bit IPA\n");
+    p2m_ipa_bits = 40;
     val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
     val |= VTCR_SL0(0x1); /* P2M starts at first level */
 #else /* CONFIG_ARM_64 */
@@ -1557,6 +1561,7 @@ void __init setup_virt_paging(void)
 
     p2m_root_order = pa_range_info[pa_range].root_order;
     p2m_root_level = 2 - pa_range_info[pa_range].sl0;
+    p2m_ipa_bits = 64 - pa_range_info[pa_range].t0sz;
 
     printk("P2M: %d-bit IPA with %d-bit PA\n",
            64 - pa_range_info[pa_range].t0sz,
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 341df55..63748ef 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -10,6 +10,9 @@
 
 #define paddr_bits PADDR_BITS
 
+/* Holds the bit size of IPAs in p2m tables.  */
+extern unsigned int p2m_ipa_bits;
+
 struct domain;
 
 extern void memory_type_changed(struct domain *);
-- 
1.9.1

Re: [PATCH v4 2/3] xen/arm: Add p2m_ipa_bits

[Ian Campbell]

On Fri, 2015-05-01 at 11:40 +1000, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> Export p2m_ipa_bits holding the bit size of IPAs used in p2m tables.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  xen/arch/arm/p2m.c        | 5 +++++
>  xen/include/asm-arm/p2m.h | 3 +++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 8dfee24..c6507ae 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -27,6 +27,9 @@ static unsigned int __read_mostly p2m_root_level;
>  
>  #define P2M_ROOT_PAGES    (1<<P2M_ROOT_ORDER)
>  
> +/* Size of IPAs in bits.  */
> +unsigned int p2m_ipa_bits;

I think this should be __read_mostly.

I'm also wondering about suggesting to put it in the existing #ifdef
with P2M_ROOT_LEVEL etc and have it be:
        const unsigned int __read_mostly = 40
on 32-bit and only vary on 64-bit.

Ian.

Re: [PATCH v4 2/3] xen/arm: Add p2m_ipa_bits

[Edgar E. Iglesias]

On Tue, May 05, 2015 at 02:23:23PM +0100, Ian Campbell wrote:
> On Fri, 2015-05-01 at 11:40 +1000, Edgar E. Iglesias wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> > 
> > Export p2m_ipa_bits holding the bit size of IPAs used in p2m tables.
> > 
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  xen/arch/arm/p2m.c        | 5 +++++
> >  xen/include/asm-arm/p2m.h | 3 +++
> >  2 files changed, 8 insertions(+)
> > 
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index 8dfee24..c6507ae 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -27,6 +27,9 @@ static unsigned int __read_mostly p2m_root_level;
> >  
> >  #define P2M_ROOT_PAGES    (1<<P2M_ROOT_ORDER)
> >  
> > +/* Size of IPAs in bits.  */
> > +unsigned int p2m_ipa_bits;
> 
> I think this should be __read_mostly.
> 
> I'm also wondering about suggesting to put it in the existing #ifdef
> with P2M_ROOT_LEVEL etc and have it be:
>         const unsigned int __read_mostly = 40
> on 32-bit and only vary on 64-bit.
>

Sounds good, I've changed it accordingly for v5.

Thanks,
Edgar 

[PATCH v4 3/3] xen/iommu: arm: Use p2m_ipa_bits as stage2 input size

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

The Stage2 input-size must match what the CPU uses because
the SMMU and the CPU share page-tables.

Test that the SMMU supports the P2M IPA bit size, use it if
supported or bail out if not.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 xen/drivers/passthrough/arm/smmu.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 8a9b58b..d9f3931 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2230,8 +2230,14 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 	size = arm_smmu_id_size_to_bits((id >> ID2_IAS_SHIFT) & ID2_IAS_MASK);
 	smmu->s1_output_size = min_t(unsigned long, PHYS_MASK_SHIFT, size);
 
-	/* Xen: Stage-2 input size is not restricted */
-	smmu->s2_input_size = size;
+	/* Xen: Stage-2 input size has to match p2m_ipa_bits.  */
+	if (size < p2m_ipa_bits) {
+		dev_err(smmu->dev,
+			"P2M IPA size not supported (P2M=%u SMMU=%lu)!\n",
+			p2m_ipa_bits, size);
+		return -ENODEV;
+	}
+	smmu->s2_input_size = p2m_ipa_bits;
 #if 0
 	/* Stage-2 input size limited due to pgd allocation (PTRS_PER_PGD) */
 #ifdef CONFIG_64BIT
-- 
1.9.1

Re: [PATCH v4 3/3] xen/iommu: arm: Use p2m_ipa_bits as stage2 input size

[Julien Grall]

Hi Edgar,

On 01/05/15 02:40, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> The Stage2 input-size must match what the CPU uses because
> the SMMU and the CPU share page-tables.
> 
> Test that the SMMU supports the P2M IPA bit size, use it if
> supported or bail out if not.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Reviewed-by: Julien Grall <julien.grall@citrix.com>

Regards,

-- 
Julien Grall

Re: [PATCH v4 3/3] xen/iommu: arm: Use p2m_ipa_bits as stage2 input size

[Ian Campbell]

On Fri, 2015-05-01 at 11:40 +1000, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> The Stage2 input-size must match what the CPU uses because
> the SMMU and the CPU share page-tables.
> 
> Test that the SMMU supports the P2M IPA bit size, use it if
> supported or bail out if not.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  xen/drivers/passthrough/arm/smmu.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 8a9b58b..d9f3931 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -2230,8 +2230,14 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>  	size = arm_smmu_id_size_to_bits((id >> ID2_IAS_SHIFT) & ID2_IAS_MASK);
>  	smmu->s1_output_size = min_t(unsigned long, PHYS_MASK_SHIFT, size);
>  
> -	/* Xen: Stage-2 input size is not restricted */
> -	smmu->s2_input_size = size;
> +	/* Xen: Stage-2 input size has to match p2m_ipa_bits.  */
> +	if (size < p2m_ipa_bits) {

Referencing my question on 0/3, what if size > p2m_ipa_bits?

Do we need to also check that we are configuring the same number of
levels of PT etc, or is that already handled?

> +		dev_err(smmu->dev,
> +			"P2M IPA size not supported (P2M=%u SMMU=%lu)!\n",
> +			p2m_ipa_bits, size);
> +		return -ENODEV;
> +	}
> +	smmu->s2_input_size = p2m_ipa_bits;
>  #if 0
>  	/* Stage-2 input size limited due to pgd allocation (PTRS_PER_PGD) */
>  #ifdef CONFIG_64BIT

Re: [PATCH v4 3/3] xen/iommu: arm: Use p2m_ipa_bits as stage2 input size

[Julien Grall]

Hi Ian,

On 05/05/15 14:24, Ian Campbell wrote:
> On Fri, 2015-05-01 at 11:40 +1000, Edgar E. Iglesias wrote:
>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>
>> The Stage2 input-size must match what the CPU uses because
>> the SMMU and the CPU share page-tables.
>>
>> Test that the SMMU supports the P2M IPA bit size, use it if
>> supported or bail out if not.
>>
>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>> ---
>>  xen/drivers/passthrough/arm/smmu.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
>> index 8a9b58b..d9f3931 100644
>> --- a/xen/drivers/passthrough/arm/smmu.c
>> +++ b/xen/drivers/passthrough/arm/smmu.c
>> @@ -2230,8 +2230,14 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>>  	size = arm_smmu_id_size_to_bits((id >> ID2_IAS_SHIFT) & ID2_IAS_MASK);
>>  	smmu->s1_output_size = min_t(unsigned long, PHYS_MASK_SHIFT, size);
>>  
>> -	/* Xen: Stage-2 input size is not restricted */
>> -	smmu->s2_input_size = size;
>> +	/* Xen: Stage-2 input size has to match p2m_ipa_bits.  */
>> +	if (size < p2m_ipa_bits) {
> 
> Referencing my question on 0/3, what if size > p2m_ipa_bits?

The would require more work because we would need to restrict the P2M
which would cause other trouble (see my answer on 0/3) such as the 1:1
mapping in dom0.

> Do we need to also check that we are configuring the same number of
> levels of PT etc, or is that already handled?

The SMMU only care about the number of IPA bits.

Regards,

-- 
Julien Grall

Re: [PATCH v4 3/3] xen/iommu: arm: Use p2m_ipa_bits as stage2 input size

[Ian Campbell]

On Tue, 2015-05-05 at 14:48 +0100, Julien Grall wrote:
> > Do we need to also check that we are configuring the same number of
> > levels of PT etc, or is that already handled?
> 
> The SMMU only care about the number of IPA bits.

What ensures that the starting level of the SMMU matches the starting
level of the MMU-s2?

Feeding a 3-level table to an MMU which is configured to expect 4 won't
end well.

Ian.

Re: [PATCH v4 3/3] xen/iommu: arm: Use p2m_ipa_bits as stage2 input size

[Julien Grall]

On 05/05/15 14:59, Ian Campbell wrote:
> On Tue, 2015-05-05 at 14:48 +0100, Julien Grall wrote:
>>> Do we need to also check that we are configuring the same number of
>>> levels of PT etc, or is that already handled?
>>
>> The SMMU only care about the number of IPA bits.
> 
> What ensures that the starting level of the SMMU matches the starting
> level of the MMU-s2?

Nothing, it's hardcoded in the SMMU driver for now :/.

It's assuming SL0 = 1 which works fine for ARM32 but would be an issue
on platform with IPA >= 44 bits.

S2 output size may need to be restrict too depending of the PA bits.

Regards,

-- 
Julien Grall

Re: [PATCH v4 3/3] xen/iommu: arm: Use p2m_ipa_bits as stage2 input size

[Edgar E. Iglesias]

On Tue, May 05, 2015 at 03:30:10PM +0100, Julien Grall wrote:
> On 05/05/15 14:59, Ian Campbell wrote:
> > On Tue, 2015-05-05 at 14:48 +0100, Julien Grall wrote:
> >>> Do we need to also check that we are configuring the same number of
> >>> levels of PT etc, or is that already handled?
> >>
> >> The SMMU only care about the number of IPA bits.
> > 
> > What ensures that the starting level of the SMMU matches the starting
> > level of the MMU-s2?
> 
> Nothing, it's hardcoded in the SMMU driver for now :/.
> 
> It's assuming SL0 = 1 which works fine for ARM32 but would be an issue
> on platform with IPA >= 44 bits.
> 
> S2 output size may need to be restrict too depending of the PA bits.

Right, these could cause problems.

I've sent out a v5 of the ipa size series.
I can have a look at these other MMU settings and send follow-up patches if
there is interest?

Cheers,
Edgar

Re: [PATCH v4 0/3] Set SMMU s2 input-size based on p2m tables

[Ian Campbell]

On Fri, 2015-05-01 at 11:40 +1000, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> Hi,
> 
> This is a fix for the issue I'm seeing on ZynqMP with missmatched
> setup of the SMMU and the shared p2m page-tables with the CPU.

Looking back at previous conversations it seems like your SMMU handles
fewer input bits than the second stage of the regular MMU, is that
right?

Is there an architectural constraint that bits(SMMU) <= bits(MMU-s2)?

> 
> This implementes a global p2m_ipa_bits cap for S2 input-size as
> discussed in the previous RFC.
> 
> Best regards,
> Edgar
> 
> Changelog:
> v3 -> v4:
> * Replace ASSERT on supported IPA sizes with returnig error.
> * Remove redundant 'addresses' after IPA.
> 
> v2 -> v3:
> * pfn -> ipa.
> * Fix typos in commit msg for 3/3.
> 
> v1 -> v2:
> * Use a global pfn bitsize instead of a per-domain one.
> 
> Edgar E. Iglesias (3):
>   xen/arm: Re-order iommu_setup to after setup_virt_paging
>   xen/arm: Add p2m_ipa_bits
>   xen/iommu: arm: Use p2m_ipa_bits as stage2 input size
> 
>  xen/arch/arm/p2m.c                 |  5 +++++
>  xen/arch/arm/setup.c               |  4 ++--
>  xen/drivers/passthrough/arm/smmu.c | 10 ++++++++--
>  xen/include/asm-arm/p2m.h          |  3 +++
>  4 files changed, 18 insertions(+), 4 deletions(-)
> 

Re: [PATCH v4 0/3] Set SMMU s2 input-size based on p2m tables

[Julien Grall]

Hi Ian,

On 05/05/15 14:17, Ian Campbell wrote:
> On Fri, 2015-05-01 at 11:40 +1000, Edgar E. Iglesias wrote:
>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>
>> Hi,
>>
>> This is a fix for the issue I'm seeing on ZynqMP with missmatched
>> setup of the SMMU and the shared p2m page-tables with the CPU.
> 
> Looking back at previous conversations it seems like your SMMU handles
> fewer input bits than the second stage of the regular MMU, is that
> right?
> 
> Is there an architectural constraint that bits(SMMU) <= bits(MMU-s2)?

His problem is bits(MMU-s2) <= bits(SMMU).

Although, we were talking about hardware where the bits(SMMU) <=
bits(MMU-s2).

I have Seattle in mind but I haven't yet feedback from Suravee (in CC).

Suravee, do you have any input? Having the bits(SMMU) <= bits(MMU-s2)
would restrict the P2M to use 40 bits. Anything above won't be
accessible to DOM0 because of the 1:1 mapping.

Regards,

-- 
Julien Grall

Re: [PATCH v4 0/3] Set SMMU s2 input-size based on p2m tables

[Edgar E. Iglesias]

On Tue, May 05, 2015 at 02:17:47PM +0100, Ian Campbell wrote:
> On Fri, 2015-05-01 at 11:40 +1000, Edgar E. Iglesias wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> > 
> > Hi,
> > 
> > This is a fix for the issue I'm seeing on ZynqMP with missmatched
> > setup of the SMMU and the shared p2m page-tables with the CPU.
> 
> Looking back at previous conversations it seems like your SMMU handles
> fewer input bits than the second stage of the regular MMU, is that
> right?

It's the other way around. The SMMU handles up-to 48 bits S2 input addrs
but the Cortex-A53 only does 40bit IPAs.

> Is there an architectural constraint that bits(SMMU) <= bits(MMU-s2)?

I'm not aware of any such constraints. In XEN, because we share page-tables
between CPU and SMMUs, we need to make sure the SMMUs support the
page-tables format and get configured accordingly.

Cheers,
Edgar


> 
> > 
> > This implementes a global p2m_ipa_bits cap for S2 input-size as
> > discussed in the previous RFC.
> > 
> > Best regards,
> > Edgar
> > 
> > Changelog:
> > v3 -> v4:
> > * Replace ASSERT on supported IPA sizes with returnig error.
> > * Remove redundant 'addresses' after IPA.
> > 
> > v2 -> v3:
> > * pfn -> ipa.
> > * Fix typos in commit msg for 3/3.
> > 
> > v1 -> v2:
> > * Use a global pfn bitsize instead of a per-domain one.
> > 
> > Edgar E. Iglesias (3):
> >   xen/arm: Re-order iommu_setup to after setup_virt_paging
> >   xen/arm: Add p2m_ipa_bits
> >   xen/iommu: arm: Use p2m_ipa_bits as stage2 input size
> > 
> >  xen/arch/arm/p2m.c                 |  5 +++++
> >  xen/arch/arm/setup.c               |  4 ++--
> >  xen/drivers/passthrough/arm/smmu.c | 10 ++++++++--
> >  xen/include/asm-arm/p2m.h          |  3 +++
> >  4 files changed, 18 insertions(+), 4 deletions(-)
> > 
> 
> 

Re: [PATCH v4 0/3] Set SMMU s2 input-size based on p2m tables

[Ian Campbell]

On Wed, 2015-05-06 at 13:26 +1000, Edgar E. Iglesias wrote:
> On Tue, May 05, 2015 at 02:17:47PM +0100, Ian Campbell wrote:
> > On Fri, 2015-05-01 at 11:40 +1000, Edgar E. Iglesias wrote:
> > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> > > 
> > > Hi,
> > > 
> > > This is a fix for the issue I'm seeing on ZynqMP with missmatched
> > > setup of the SMMU and the shared p2m page-tables with the CPU.
> > 
> > Looking back at previous conversations it seems like your SMMU handles
> > fewer input bits than the second stage of the regular MMU, is that
> > right?
> 
> It's the other way around. The SMMU handles up-to 48 bits S2 input addrs
> but the Cortex-A53 only does 40bit IPAs.

Oops, I read backwards, thanks.

Thinking about it some more I think more (at at least as) important as
the input IPA size is the page table format, specifically the starting
level (TCR.SL0) and the amount of concatenation at the root level (I
forget how that is specified I think it is implicit in the TCR.SL0 and
TCR.T0SZ?). If we are sharing page tables then they really ought to
match.

That said I think your patch is a good start and rules out issues on at
least one axis, so it's worth doing.

> > Is there an architectural constraint that bits(SMMU) <= bits(MMU-s2)?
> 
> I'm not aware of any such constraints. In XEN, because we share page-tables
> between CPU and SMMUs, we need to make sure the SMMUs support the
> page-tables format and get configured accordingly.

Right, it seems like we may eventually need to introduce the possibility
of not sharing the p2m depending on the circumstances as is done on x86.

Ian.

Re: [PATCH v4 0/3] Set SMMU s2 input-size based on p2m tables

[Edgar E. Iglesias]

On Wed, May 06, 2015 at 09:54:57AM +0100, Ian Campbell wrote:
> On Wed, 2015-05-06 at 13:26 +1000, Edgar E. Iglesias wrote:
> > On Tue, May 05, 2015 at 02:17:47PM +0100, Ian Campbell wrote:
> > > On Fri, 2015-05-01 at 11:40 +1000, Edgar E. Iglesias wrote:
> > > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> > > > 
> > > > Hi,
> > > > 
> > > > This is a fix for the issue I'm seeing on ZynqMP with missmatched
> > > > setup of the SMMU and the shared p2m page-tables with the CPU.
> > > 
> > > Looking back at previous conversations it seems like your SMMU handles
> > > fewer input bits than the second stage of the regular MMU, is that
> > > right?
> > 
> > It's the other way around. The SMMU handles up-to 48 bits S2 input addrs
> > but the Cortex-A53 only does 40bit IPAs.
> 
> Oops, I read backwards, thanks.
> 
> Thinking about it some more I think more (at at least as) important as
> the input IPA size is the page table format, specifically the starting
> level (TCR.SL0) and the amount of concatenation at the root level (I
> forget how that is specified I think it is implicit in the TCR.SL0 and
> TCR.T0SZ?). If we are sharing page tables then they really ought to
> match.

Yes these are just as important. It just happens to get configured
correctly for the HW we are using.
BTW, I think it is indirectly computed from the startlevel, page-size
and S2 input size.

> That said I think your patch is a good start and rules out issues on at
> least one axis, so it's worth doing.
> 
> > > Is there an architectural constraint that bits(SMMU) <= bits(MMU-s2)?
> > 
> > I'm not aware of any such constraints. In XEN, because we share page-tables
> > between CPU and SMMUs, we need to make sure the SMMUs support the
> > page-tables format and get configured accordingly.
> 
> Right, it seems like we may eventually need to introduce the possibility
> of not sharing the p2m depending on the circumstances as is done on x86.

Yes. How would that work in practice? I guess some of the guests memory space
would not be DMA:able? or would we allow some kind of dynamic mapping
driven from the guest?

Cheers,
Edgar

Re: [PATCH v4 0/3] Set SMMU s2 input-size based on p2m tables

[Ian Campbell]

On Wed, 2015-05-06 at 19:17 +1000, Edgar E. Iglesias wrote:
> > Right, it seems like we may eventually need to introduce the possibility
> > of not sharing the p2m depending on the circumstances as is done on x86.
> 
> Yes. How would that work in practice? I guess some of the guests memory space
> would not be DMA:able? or would we allow some kind of dynamic mapping
> driven from the guest?

For domU with passthrough enabled there would be a limitation on the
maximum usable IPA I suppose.

For dom0 it's a bit trickier, but I think the answer is basically that
on systems with insufficiently large SMMU support and peripherals or RAM
above the SMMU's limit we wouldn't be able to take advantage of the SMMU
protections and would be stuck with e.g. 1:1 mode. If it was only RAM
and not peripherals up high then perhaps we could trade off use of SMMU
vs dom0 RAM size.

Ian

Re: [PATCH v4 0/3] Set SMMU s2 input-size based on p2m tables

[Julien Grall]

Hi Ian,

On 06/05/15 10:39, Ian Campbell wrote:
> On Wed, 2015-05-06 at 19:17 +1000, Edgar E. Iglesias wrote:
>>> Right, it seems like we may eventually need to introduce the possibility
>>> of not sharing the p2m depending on the circumstances as is done on x86.

I'd like to avoid non-share P2M as much as possible. It would also not
help in the situation where bits(SMMU) < bits(MMU-s2), at least in case
of DOM0.

For DOM0 with direct memory mapping (currently the default), every grant
table page are also mapped 1:1 in order to use them in DMA requests.
This is because dev_bus_addr return by the hypercall is the MFN (not the
IPA).

The direct memory mapping could only be dropped if all the devices using
DMA are protected by an SMMU.

>> Yes. How would that work in practice? I guess some of the guests memory space
>> would not be DMA:able? or would we allow some kind of dynamic mapping
>> driven from the guest?
> 
> For domU with passthrough enabled there would be a limitation on the
> maximum usable IPA I suppose.

Right.

> For dom0 it's a bit trickier, but I think the answer is basically that
> on systems with insufficiently large SMMU support and peripherals or RAM
> above the SMMU's limit we wouldn't be able to take advantage of the SMMU
> protections and would be stuck with e.g. 1:1 mode. If it was only RAM
> and not peripherals up high then perhaps we could trade off use of SMMU
> vs dom0 RAM size.

See a possible problem above. I think we would have to boot DOM0 without
SMMU protection.

Although, given the complexity of the implementation, I would wait any
feedback from AMD before considering to add SMMU support for platform
where the SMMU handle less address bits than the MMU.

Regards,

-- 
Julien Grall

Re: [PATCH v4 0/3] Set SMMU s2 input-size based on p2m tables

[Ian Campbell]

On Wed, 2015-05-06 at 12:06 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 06/05/15 10:39, Ian Campbell wrote:
> > On Wed, 2015-05-06 at 19:17 +1000, Edgar E. Iglesias wrote:
> >>> Right, it seems like we may eventually need to introduce the possibility
> >>> of not sharing the p2m depending on the circumstances as is done on x86.
> 
> I'd like to avoid non-share P2M as much as possible. It would also not
> help in the situation where bits(SMMU) < bits(MMU-s2), at least in case
> of DOM0.
> 
> For DOM0 with direct memory mapping (currently the default), every grant
> table page are also mapped 1:1 in order to use them in DMA requests.
> This is because dev_bus_addr return by the hypercall is the MFN (not the
> IPA).
> 
> The direct memory mapping could only be dropped if all the devices using
> DMA are protected by an SMMU.

Correct, I was talking mainly about the case where we would otherwise be
able to drop the mapping, i.e. everything protected (IOW "RAM completely
covered" is a special case of "protected device").

> > For dom0 it's a bit trickier, but I think the answer is basically that
> > on systems with insufficiently large SMMU support and peripherals or RAM
> > above the SMMU's limit we wouldn't be able to take advantage of the SMMU
> > protections and would be stuck with e.g. 1:1 mode. If it was only RAM
> > and not peripherals up high then perhaps we could trade off use of SMMU
> > vs dom0 RAM size.
> 
> See a possible problem above. I think we would have to boot DOM0 without
> SMMU protection.

Right, I was thinking mainly of the non-1:1 SMMU is completely used
case.

> Although, given the complexity of the implementation, I would wait any
> feedback from AMD before considering to add SMMU support for platform
> where the SMMU handle less address bits than the MMU.

Agreed.

Ian.