VTCR.T0SZ should be set as 0x20 to support 32bit IPA.
Refer ARM DDI 0487I.a ID081822, G8-9824, G8.2.171, VTCR,
"Virtualization Translation Control Register" for the bit descriptions.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from -

v1 - New patch.

 xen/arch/arm/p2m.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 948f199d84..cfdea55e71 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -2266,13 +2266,17 @@ void __init setup_virt_paging(void)
     register_t val = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
 
 #ifdef CONFIG_ARM_32
-    if ( p2m_ipa_bits < 40 )
+    if ( p2m_ipa_bits < PADDR_BITS )
         panic("P2M: Not able to support %u-bit IPA at the moment\n",
               p2m_ipa_bits);
 
-    printk("P2M: 40-bit IPA\n");
-    p2m_ipa_bits = 40;
+    printk("P2M: %u-bit IPA\n",PADDR_BITS);
+    p2m_ipa_bits = PADDR_BITS;
+#ifdef CONFIG_ARM_PA_32
+    val |= VTCR_T0SZ(0x20); /* 32 bit IPA */
+#else
     val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
+#endif
     val |= VTCR_SL0(0x1); /* P2M starts at first level */
 #else /* CONFIG_ARM_64 */
     static const struct {
-- 
2.17.1

Hi Ayan,

On 17/01/2023 17:43, Ayan Kumar Halder wrote:
> VTCR.T0SZ should be set as 0x20 to support 32bit IPA.
> Refer ARM DDI 0487I.a ID081822, G8-9824, G8.2.171, VTCR,
> "Virtualization Translation Control Register" for the bit descriptions.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> Changes from -
> 
> v1 - New patch.
> 
>   xen/arch/arm/p2m.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 948f199d84..cfdea55e71 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -2266,13 +2266,17 @@ void __init setup_virt_paging(void)
>       register_t val = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
>   
>   #ifdef CONFIG_ARM_32
> -    if ( p2m_ipa_bits < 40 )
> +    if ( p2m_ipa_bits < PADDR_BITS )
>           panic("P2M: Not able to support %u-bit IPA at the moment\n",
>                 p2m_ipa_bits);
>   
> -    printk("P2M: 40-bit IPA\n");
> -    p2m_ipa_bits = 40;
> +    printk("P2M: %u-bit IPA\n",PADDR_BITS);
> +    p2m_ipa_bits = PADDR_BITS;
> +#ifdef CONFIG_ARM_PA_32
> +    val |= VTCR_T0SZ(0x20); /* 32 bit IPA */
> +#else
>       val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
> +#endif

I am wondering whether this is right time to switch to an array like the 
arm64 code? This would allow to use 32-bit IPA also when Xen support 
64-bit physical address.

Cheers,

-- 
Julien Grall

On 20/01/2023 11:06, Julien Grall wrote:
> Hi Ayan,
Hi Julien,
>
> On 17/01/2023 17:43, Ayan Kumar Halder wrote:
>> VTCR.T0SZ should be set as 0x20 to support 32bit IPA.
>> Refer ARM DDI 0487I.a ID081822, G8-9824, G8.2.171, VTCR,
>> "Virtualization Translation Control Register" for the bit descriptions.
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>> Changes from -
>>
>> v1 - New patch.
>>
>>   xen/arch/arm/p2m.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 948f199d84..cfdea55e71 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -2266,13 +2266,17 @@ void __init setup_virt_paging(void)
>>       register_t val = 
>> VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
>>     #ifdef CONFIG_ARM_32
>> -    if ( p2m_ipa_bits < 40 )
>> +    if ( p2m_ipa_bits < PADDR_BITS )
>>           panic("P2M: Not able to support %u-bit IPA at the moment\n",
>>                 p2m_ipa_bits);
>>   -    printk("P2M: 40-bit IPA\n");
>> -    p2m_ipa_bits = 40;
>> +    printk("P2M: %u-bit IPA\n",PADDR_BITS);
>> +    p2m_ipa_bits = PADDR_BITS;
>> +#ifdef CONFIG_ARM_PA_32
>> +    val |= VTCR_T0SZ(0x20); /* 32 bit IPA */
>> +#else
>>       val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
>> +#endif
>
> I am wondering whether this is right time to switch to an array like 
> the arm64 code? This would allow to use 32-bit IPA also when Xen 
> support 64-bit physical address.

In AArch64, we use ID_AA64MMFR0_EL1.PARange to determine the physical 
address range supported at runtime. This is then used as an index into 
pa_range_info[] to determine t0sz, root_order, etc.

However, for AArch32 I do not see an equivalent register (similar to 
ID_AA64MMFR0_EL1) or any register to determine the physical address 
range. Thus, I will prefer to keep the code as it is unless you suggest 
any alternative.

- Ayan

>
> Cheers,
>

Hi,

On 07/02/2023 15:34, Ayan Kumar Halder wrote:
> 
> On 20/01/2023 11:06, Julien Grall wrote:
>> Hi Ayan,
> Hi Julien,
>>
>> On 17/01/2023 17:43, Ayan Kumar Halder wrote:
>>> VTCR.T0SZ should be set as 0x20 to support 32bit IPA.
>>> Refer ARM DDI 0487I.a ID081822, G8-9824, G8.2.171, VTCR,
>>> "Virtualization Translation Control Register" for the bit descriptions.
>>>
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>> ---
>>> Changes from -
>>>
>>> v1 - New patch.
>>>
>>>   xen/arch/arm/p2m.c | 10 +++++++---
>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>> index 948f199d84..cfdea55e71 100644
>>> --- a/xen/arch/arm/p2m.c
>>> +++ b/xen/arch/arm/p2m.c
>>> @@ -2266,13 +2266,17 @@ void __init setup_virt_paging(void)
>>>       register_t val = 
>>> VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
>>>     #ifdef CONFIG_ARM_32
>>> -    if ( p2m_ipa_bits < 40 )
>>> +    if ( p2m_ipa_bits < PADDR_BITS )
>>>           panic("P2M: Not able to support %u-bit IPA at the moment\n",
>>>                 p2m_ipa_bits);
>>>   -    printk("P2M: 40-bit IPA\n");
>>> -    p2m_ipa_bits = 40;
>>> +    printk("P2M: %u-bit IPA\n",PADDR_BITS);
>>> +    p2m_ipa_bits = PADDR_BITS;
>>> +#ifdef CONFIG_ARM_PA_32
>>> +    val |= VTCR_T0SZ(0x20); /* 32 bit IPA */
>>> +#else
>>>       val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
>>> +#endif
>>
>> I am wondering whether this is right time to switch to an array like 
>> the arm64 code? This would allow to use 32-bit IPA also when Xen 
>> support 64-bit physical address.
> 
> In AArch64, we use ID_AA64MMFR0_EL1.PARange to determine the physical 
> address range supported at runtime. This is then used as an index into 
> pa_range_info[] to determine t0sz, root_order, etc.

It is using both the ID_AA64MMFR0_EL1 but also p2m_ipa_bits to decide 
the size.

> 
> However, for AArch32 I do not see an equivalent register (similar to 
> ID_AA64MMFR0_EL1) or any register to determine the physical address 
> range. Thus, I will prefer to keep the code as it is unless you suggest 
> any alternative.

I looked at the Arm Arm and indeed it doesn't look like there are 
equivalent for ID_AA64MMFR0_EL1.PARange.

However, my point was less about reading the system register but more 
about the fact we could have the code a bit more generic and avoid the 
assumption that PADDR_BITS is only modified when CONFIG_ARM_PA_32 is set.

Cheers,

-- 
Julien Grall

On 09/02/2023 11:45, Julien Grall wrote:
> Hi,
Hi Julien,
>
> On 07/02/2023 15:34, Ayan Kumar Halder wrote:
>>
>> On 20/01/2023 11:06, Julien Grall wrote:
>>> Hi Ayan,
>> Hi Julien,
>>>
>>> On 17/01/2023 17:43, Ayan Kumar Halder wrote:
>>>> VTCR.T0SZ should be set as 0x20 to support 32bit IPA.
>>>> Refer ARM DDI 0487I.a ID081822, G8-9824, G8.2.171, VTCR,
>>>> "Virtualization Translation Control Register" for the bit 
>>>> descriptions.
>>>>
>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>>> ---
>>>> Changes from -
>>>>
>>>> v1 - New patch.
>>>>
>>>>   xen/arch/arm/p2m.c | 10 +++++++---
>>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>> index 948f199d84..cfdea55e71 100644
>>>> --- a/xen/arch/arm/p2m.c
>>>> +++ b/xen/arch/arm/p2m.c
>>>> @@ -2266,13 +2266,17 @@ void __init setup_virt_paging(void)
>>>>       register_t val = 
>>>> VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
>>>>     #ifdef CONFIG_ARM_32
>>>> -    if ( p2m_ipa_bits < 40 )
>>>> +    if ( p2m_ipa_bits < PADDR_BITS )
>>>>           panic("P2M: Not able to support %u-bit IPA at the moment\n",
>>>>                 p2m_ipa_bits);
>>>>   -    printk("P2M: 40-bit IPA\n");
>>>> -    p2m_ipa_bits = 40;
>>>> +    printk("P2M: %u-bit IPA\n",PADDR_BITS);
>>>> +    p2m_ipa_bits = PADDR_BITS;
>>>> +#ifdef CONFIG_ARM_PA_32
>>>> +    val |= VTCR_T0SZ(0x20); /* 32 bit IPA */
>>>> +#else
>>>>       val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
>>>> +#endif
>>>
>>> I am wondering whether this is right time to switch to an array like 
>>> the arm64 code? This would allow to use 32-bit IPA also when Xen 
>>> support 64-bit physical address.
>>
>> In AArch64, we use ID_AA64MMFR0_EL1.PARange to determine the physical 
>> address range supported at runtime. This is then used as an index 
>> into pa_range_info[] to determine t0sz, root_order, etc.
>
> It is using both the ID_AA64MMFR0_EL1 but also p2m_ipa_bits to decide 
> the size.
Ack.
>
>>
>> However, for AArch32 I do not see an equivalent register (similar to 
>> ID_AA64MMFR0_EL1) or any register to determine the physical address 
>> range. Thus, I will prefer to keep the code as it is unless you 
>> suggest any alternative.
>
> I looked at the Arm Arm and indeed it doesn't look like there are 
> equivalent for ID_AA64MMFR0_EL1.PARange.
>
> However, my point was less about reading the system register but more 
> about the fact we could have the code a bit more generic and avoid the 
> assumption that PADDR_BITS is only modified when CONFIG_ARM_PA_32 is set.

I had a rework at the patch. Please let me know if the following looks 
better.

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 948f199d84..bc3bdf5f3e 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -2266,14 +2266,35 @@ void __init setup_virt_paging(void)
      register_t val = 
VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;

  #ifdef CONFIG_ARM_32
-    if ( p2m_ipa_bits < 40 )
+    static const struct {
+        unsigned int pabits; /* Physical Address Size */
+        unsigned int t0sz;   /* Desired T0SZ */
+        unsigned int sl0;    /* Desired SL0 */
+    } pa_range_info[] __initconst = {
+        [0] = { 32,     32,     1 },
+        [1] = { 40,     24,     1 },
+    };
+    int i = 0;
+
+    if ( p2m_ipa_bits < PADDR_BITS )
+        panic("P2M: Not able to support %u-bit IPA at the moment\n",
+              p2m_ipa_bits);
+
+    printk("P2M: %u-bit IPA\n",PADDR_BITS);
+    p2m_ipa_bits = PADDR_BITS;
+
+    for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
+        if ( p2m_ipa_bits == pa_range_info[i].pabits )
+            break;
+
+    if ( i == ARRAY_SIZE(pa_range_info) )
          panic("P2M: Not able to support %u-bit IPA at the moment\n",
                p2m_ipa_bits);

-    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 */
+    val |= VTCR_T0SZ(pa_range_info[i].t0sz);
+    val |= VTCR_SL0(pa_range_info[i].sl0);
+

- Ayan

>
> Cheers,
>

Hi,

On 10/02/2023 15:39, Ayan Kumar Halder wrote:
> 
> On 09/02/2023 11:45, Julien Grall wrote:
>> Hi,
> Hi Julien,
>>
>> On 07/02/2023 15:34, Ayan Kumar Halder wrote:
>>>
>>> On 20/01/2023 11:06, Julien Grall wrote:
>>>> Hi Ayan,
>>> Hi Julien,
>>>>
>>>> On 17/01/2023 17:43, Ayan Kumar Halder wrote:
>>>>> VTCR.T0SZ should be set as 0x20 to support 32bit IPA.
>>>>> Refer ARM DDI 0487I.a ID081822, G8-9824, G8.2.171, VTCR,
>>>>> "Virtualization Translation Control Register" for the bit 
>>>>> descriptions.
>>>>>
>>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>>>> ---
>>>>> Changes from -
>>>>>
>>>>> v1 - New patch.
>>>>>
>>>>>   xen/arch/arm/p2m.c | 10 +++++++---
>>>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>>> index 948f199d84..cfdea55e71 100644
>>>>> --- a/xen/arch/arm/p2m.c
>>>>> +++ b/xen/arch/arm/p2m.c
>>>>> @@ -2266,13 +2266,17 @@ void __init setup_virt_paging(void)
>>>>>       register_t val = 
>>>>> VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
>>>>>     #ifdef CONFIG_ARM_32
>>>>> -    if ( p2m_ipa_bits < 40 )
>>>>> +    if ( p2m_ipa_bits < PADDR_BITS )
>>>>>           panic("P2M: Not able to support %u-bit IPA at the moment\n",
>>>>>                 p2m_ipa_bits);
>>>>>   -    printk("P2M: 40-bit IPA\n");
>>>>> -    p2m_ipa_bits = 40;
>>>>> +    printk("P2M: %u-bit IPA\n",PADDR_BITS);
>>>>> +    p2m_ipa_bits = PADDR_BITS;
>>>>> +#ifdef CONFIG_ARM_PA_32
>>>>> +    val |= VTCR_T0SZ(0x20); /* 32 bit IPA */
>>>>> +#else
>>>>>       val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
>>>>> +#endif
>>>>
>>>> I am wondering whether this is right time to switch to an array like 
>>>> the arm64 code? This would allow to use 32-bit IPA also when Xen 
>>>> support 64-bit physical address.
>>>
>>> In AArch64, we use ID_AA64MMFR0_EL1.PARange to determine the physical 
>>> address range supported at runtime. This is then used as an index 
>>> into pa_range_info[] to determine t0sz, root_order, etc.
>>
>> It is using both the ID_AA64MMFR0_EL1 but also p2m_ipa_bits to decide 
>> the size.
> Ack.
>>
>>>
>>> However, for AArch32 I do not see an equivalent register (similar to 
>>> ID_AA64MMFR0_EL1) or any register to determine the physical address 
>>> range. Thus, I will prefer to keep the code as it is unless you 
>>> suggest any alternative.
>>
>> I looked at the Arm Arm and indeed it doesn't look like there are 
>> equivalent for ID_AA64MMFR0_EL1.PARange.
>>
>> However, my point was less about reading the system register but more 
>> about the fact we could have the code a bit more generic and avoid the 
>> assumption that PADDR_BITS is only modified when CONFIG_ARM_PA_32 is set.
> 
> I had a rework at the patch. Please let me know if the following looks 
> better.
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 948f199d84..bc3bdf5f3e 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -2266,14 +2266,35 @@ void __init setup_virt_paging(void)
>       register_t val = 
> VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
> 
>   #ifdef CONFIG_ARM_32
> -    if ( p2m_ipa_bits < 40 )
> +    static const struct {
> +        unsigned int pabits; /* Physical Address Size */
> +        unsigned int t0sz;   /* Desired T0SZ */
> +        unsigned int sl0;    /* Desired SL0 */

Hmmm... Why don't you include the root order? In fact...

> +    } pa_range_info[] __initconst = {
> +        [0] = { 32,     32,     1 },
> +        [1] = { 40,     24,     1 },

... looking at the ARMv7 specification (see B3-1345 in ARM DDI 0406C.d), 
the root page-table is only concatenated for 40-bits.

> +    };
> +    int i = 0;
> +
> +    if ( p2m_ipa_bits < PADDR_BITS )
> +        panic("P2M: Not able to support %u-bit IPA at the moment\n",
> +              p2m_ipa_bits);

This check seems unnecessary now.

> +
> +    printk("P2M: %u-bit IPA\n",PADDR_BITS);
> +    p2m_ipa_bits = PADDR_BITS;
> +
> +    for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
> +        if ( p2m_ipa_bits == pa_range_info[i].pabits )
> +            break;
> +
> +    if ( i == ARRAY_SIZE(pa_range_info) )
>           panic("P2M: Not able to support %u-bit IPA at the moment\n",
>                 p2m_ipa_bits);
> 
> -    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 */
> +    val |= VTCR_T0SZ(pa_range_info[i].t0sz);
> +    val |= VTCR_SL0(pa_range_info[i].sl0);
> +

I think you can share a lot of code between arm64 and arm32. A diff 
below (not tested):

diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
index 91df922e1c9f..05f26780a29d 100644
--- a/xen/arch/arm/include/asm/p2m.h
+++ b/xen/arch/arm/include/asm/p2m.h
@@ -14,16 +14,10 @@
  /* Holds the bit size of IPAs in p2m tables.  */
  extern unsigned int p2m_ipa_bits;

-#ifdef CONFIG_ARM_64
  extern unsigned int p2m_root_order;
  extern unsigned int p2m_root_level;
  #define P2M_ROOT_ORDER    p2m_root_order
  #define P2M_ROOT_LEVEL p2m_root_level
-#else
-/* First level P2M is always 2 consecutive pages */
-#define P2M_ROOT_ORDER    1
-#define P2M_ROOT_LEVEL 1
-#endif

  struct domain;

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 948f199d848f..6fda0b92230a 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -2264,17 +2264,6 @@ void __init setup_virt_paging(void)
  {
      /* Setup Stage 2 address translation */
      register_t val = 
VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
-
-#ifdef CONFIG_ARM_32
-    if ( p2m_ipa_bits < 40 )
-        panic("P2M: Not able to support %u-bit IPA at the moment\n",
-              p2m_ipa_bits);
-
-    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 */
      static const struct {
          unsigned int pabits; /* Physical Address Size */
          unsigned int t0sz;   /* Desired T0SZ, minimum in comment */
@@ -2286,29 +2275,26 @@ void __init setup_virt_paging(void)
          [0] = { 32,      32/*32*/,  0,          1 },
          [1] = { 36,      28/*28*/,  0,          1 },
          [2] = { 40,      24/*24*/,  1,          1 },
+#ifdef CONFIG_ARM_64
          [3] = { 42,      22/*22*/,  3,          1 },
          [4] = { 44,      20/*20*/,  0,          2 },
          [5] = { 48,      16/*16*/,  0,          2 },
          [6] = { 52,      12/*12*/,  4,          2 },
          [7] = { 0 }  /* Invalid */
+#endif
      };

      unsigned int i;
      unsigned int pa_range = 0x10; /* Larger than any possible value */

+#ifdef CONFIG_ARM_64
      /*
       * Restrict "p2m_ipa_bits" if needed. As P2M table is always 
configured
       * with IPA bits == PA bits, compare against "pabits".
       */
      if ( pa_range_info[system_cpuinfo.mm64.pa_range].pabits < 
p2m_ipa_bits )
          p2m_ipa_bits = pa_range_info[system_cpuinfo.mm64.pa_range].pabits;
-
-    /*
-     * cpu info sanitization made sure we support 16bits VMID only if all
-     * cores are supporting it.
-     */
-    if ( system_cpuinfo.mm64.vmid_bits == MM64_VMID_16_BITS_SUPPORT )
-        max_vmid = MAX_VMID_16_BIT;
+#endif

      /* Choose suitable "pa_range" according to the resulted 
"p2m_ipa_bits". */
      for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
@@ -2324,12 +2310,22 @@ void __init setup_virt_paging(void)
      if ( pa_range >= ARRAY_SIZE(pa_range_info) || 
!pa_range_info[pa_range].pabits )
          panic("Unknown encoding of ID_AA64MMFR0_EL1.PARange %x\n", 
pa_range);

-    val |= VTCR_PS(pa_range);
-    val |= VTCR_TG0_4K;
+#ifdef CONFIG_ARM_64
+    /*
+     * cpu info sanitization made sure we support 16bits VMID only if all
+     * cores are supporting it.
+     */
+    if ( system_cpuinfo.mm64.vmid_bits == MM64_VMID_16_BITS_SUPPORT )
+        max_vmid = MAX_VMID_16_BIT;

      /* Set the VS bit only if 16 bit VMID is supported. */
      if ( MAX_VMID == MAX_VMID_16_BIT )
          val |= VTCR_VS;
+#endif
+
+    val |= VTCR_PS(pa_range);
+    val |= VTCR_TG0_4K;
+
      val |= VTCR_SL0(pa_range_info[pa_range].sl0);
      val |= VTCR_T0SZ(pa_range_info[pa_range].t0sz);

@@ -2341,7 +2337,7 @@ void __init setup_virt_paging(void)
             p2m_ipa_bits,
             pa_range_info[pa_range].pabits,
             ( MAX_VMID == MAX_VMID_16_BIT ) ? 16 : 8);
-#endif
+
      printk("P2M: %d levels with order-%d root, VTCR 0x%"PRIregister"\n",
             4 - P2M_ROOT_LEVEL, P2M_ROOT_ORDER, val);

Cheers,

-- 
Julien Grall

On 10/02/2023 16:19, Julien Grall wrote:
> Hi,

Hi Julien,

I need some inputs.

>
> On 10/02/2023 15:39, Ayan Kumar Halder wrote:
>>
>> On 09/02/2023 11:45, Julien Grall wrote:
>>> Hi,
>> Hi Julien,
>>>
>>> On 07/02/2023 15:34, Ayan Kumar Halder wrote:
>>>>
>>>> On 20/01/2023 11:06, Julien Grall wrote:
>>>>> Hi Ayan,
>>>> Hi Julien,
>>>>>
>>>>> On 17/01/2023 17:43, Ayan Kumar Halder wrote:
>>>>>> VTCR.T0SZ should be set as 0x20 to support 32bit IPA.
>>>>>> Refer ARM DDI 0487I.a ID081822, G8-9824, G8.2.171, VTCR,
>>>>>> "Virtualization Translation Control Register" for the bit 
>>>>>> descriptions.
>>>>>>
>>>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>>>>> ---
>>>>>> Changes from -
>>>>>>
>>>>>> v1 - New patch.
>>>>>>
>>>>>>   xen/arch/arm/p2m.c | 10 +++++++---
>>>>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>>>> index 948f199d84..cfdea55e71 100644
>>>>>> --- a/xen/arch/arm/p2m.c
>>>>>> +++ b/xen/arch/arm/p2m.c
>>>>>> @@ -2266,13 +2266,17 @@ void __init setup_virt_paging(void)
>>>>>>       register_t val = 
>>>>>> VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
>>>>>>     #ifdef CONFIG_ARM_32
>>>>>> -    if ( p2m_ipa_bits < 40 )
>>>>>> +    if ( p2m_ipa_bits < PADDR_BITS )
>>>>>>           panic("P2M: Not able to support %u-bit IPA at the 
>>>>>> moment\n",
>>>>>>                 p2m_ipa_bits);
>>>>>>   -    printk("P2M: 40-bit IPA\n");
>>>>>> -    p2m_ipa_bits = 40;
>>>>>> +    printk("P2M: %u-bit IPA\n",PADDR_BITS);
>>>>>> +    p2m_ipa_bits = PADDR_BITS;
>>>>>> +#ifdef CONFIG_ARM_PA_32
>>>>>> +    val |= VTCR_T0SZ(0x20); /* 32 bit IPA */
>>>>>> +#else
>>>>>>       val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
>>>>>> +#endif
>>>>>
>>>>> I am wondering whether this is right time to switch to an array 
>>>>> like the arm64 code? This would allow to use 32-bit IPA also when 
>>>>> Xen support 64-bit physical address.
>>>>
>>>> In AArch64, we use ID_AA64MMFR0_EL1.PARange to determine the 
>>>> physical address range supported at runtime. This is then used as 
>>>> an index into pa_range_info[] to determine t0sz, root_order, etc.
>>>
>>> It is using both the ID_AA64MMFR0_EL1 but also p2m_ipa_bits to 
>>> decide the size.
>> Ack.
>>>
>>>>
>>>> However, for AArch32 I do not see an equivalent register (similar 
>>>> to ID_AA64MMFR0_EL1) or any register to determine the physical 
>>>> address range. Thus, I will prefer to keep the code as it is unless 
>>>> you suggest any alternative.
>>>
>>> I looked at the Arm Arm and indeed it doesn't look like there are 
>>> equivalent for ID_AA64MMFR0_EL1.PARange.
>>>
>>> However, my point was less about reading the system register but 
>>> more about the fact we could have the code a bit more generic and 
>>> avoid the assumption that PADDR_BITS is only modified when 
>>> CONFIG_ARM_PA_32 is set.
>>
>> I had a rework at the patch. Please let me know if the following 
>> looks better.
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 948f199d84..bc3bdf5f3e 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -2266,14 +2266,35 @@ void __init setup_virt_paging(void)
>>       register_t val = 
>> VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
>>
>>   #ifdef CONFIG_ARM_32
>> -    if ( p2m_ipa_bits < 40 )
>> +    static const struct {
>> +        unsigned int pabits; /* Physical Address Size */
>> +        unsigned int t0sz;   /* Desired T0SZ */
>> +        unsigned int sl0;    /* Desired SL0 */
>
> Hmmm... Why don't you include the root order? In fact...
>
>> +    } pa_range_info[] __initconst = {
>> +        [0] = { 32,     32,     1 },
>> +        [1] = { 40,     24,     1 },
>
> ... looking at the ARMv7 specification (see B3-1345 in ARM DDI 
> 0406C.d), the root page-table is only concatenated for 40-bits.
Sorry, I could not follow how you inferred this. Can you paste the 
relevant snippet ?
>
>> +    };
>> +    int i = 0;
>> +
>> +    if ( p2m_ipa_bits < PADDR_BITS )
>> +        panic("P2M: Not able to support %u-bit IPA at the moment\n",
>> +              p2m_ipa_bits);
>
> This check seems unnecessary now.

We still need this check as arm_smmu_device_cfg_probe() invokes 
p2m_restrict_ipa_bits(size).

And referARM IHI 0062D.cID070116 (SMMU arch version 2.0 Specification), 
the IPA address size can be

 0.

    0b0000 32-bit.

 1.

    0b0001 36-bit.

10.

    0b0010 40-bit.

11.

    0b0011 42-bit.

100.

    0b0100 44-bit.

101.

    0b0101 48-bit.

So if p2m_ipa_bits = 36 bits and PADDR_BITS = 40, then we want to panic.

- Ayan

>
>> +
>> +    printk("P2M: %u-bit IPA\n",PADDR_BITS);
>> +    p2m_ipa_bits = PADDR_BITS;
>> +
>> +    for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
>> +        if ( p2m_ipa_bits == pa_range_info[i].pabits )
>> +            break;
>> +
>> +    if ( i == ARRAY_SIZE(pa_range_info) )
>>           panic("P2M: Not able to support %u-bit IPA at the moment\n",
>>                 p2m_ipa_bits);
>>
>> -    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 */
>> +    val |= VTCR_T0SZ(pa_range_info[i].t0sz);
>> +    val |= VTCR_SL0(pa_range_info[i].sl0);
>> +
>
> I think you can share a lot of code between arm64 and arm32. A diff 
> below (not tested):
>
> diff --git a/xen/arch/arm/include/asm/p2m.h 
> b/xen/arch/arm/include/asm/p2m.h
> index 91df922e1c9f..05f26780a29d 100644
> --- a/xen/arch/arm/include/asm/p2m.h
> +++ b/xen/arch/arm/include/asm/p2m.h
> @@ -14,16 +14,10 @@
>  /* Holds the bit size of IPAs in p2m tables.  */
>  extern unsigned int p2m_ipa_bits;
>
> -#ifdef CONFIG_ARM_64
>  extern unsigned int p2m_root_order;
>  extern unsigned int p2m_root_level;
>  #define P2M_ROOT_ORDER    p2m_root_order
>  #define P2M_ROOT_LEVEL p2m_root_level
> -#else
> -/* First level P2M is always 2 consecutive pages */
> -#define P2M_ROOT_ORDER    1
> -#define P2M_ROOT_LEVEL 1
> -#endif
>
>  struct domain;
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 948f199d848f..6fda0b92230a 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -2264,17 +2264,6 @@ void __init setup_virt_paging(void)
>  {
>      /* Setup Stage 2 address translation */
>      register_t val = 
> VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
> -
> -#ifdef CONFIG_ARM_32
> -    if ( p2m_ipa_bits < 40 )
> -        panic("P2M: Not able to support %u-bit IPA at the moment\n",
> -              p2m_ipa_bits);
> -
> -    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 */
>      static const struct {
>          unsigned int pabits; /* Physical Address Size */
>          unsigned int t0sz;   /* Desired T0SZ, minimum in comment */
> @@ -2286,29 +2275,26 @@ void __init setup_virt_paging(void)
>          [0] = { 32,      32/*32*/,  0,          1 },
>          [1] = { 36,      28/*28*/,  0,          1 },
>          [2] = { 40,      24/*24*/,  1,          1 },
> +#ifdef CONFIG_ARM_64
>          [3] = { 42,      22/*22*/,  3,          1 },
>          [4] = { 44,      20/*20*/,  0,          2 },
>          [5] = { 48,      16/*16*/,  0,          2 },
>          [6] = { 52,      12/*12*/,  4,          2 },
>          [7] = { 0 }  /* Invalid */
> +#endif
>      };
>
>      unsigned int i;
>      unsigned int pa_range = 0x10; /* Larger than any possible value */
>
> +#ifdef CONFIG_ARM_64
>      /*
>       * Restrict "p2m_ipa_bits" if needed. As P2M table is always 
> configured
>       * with IPA bits == PA bits, compare against "pabits".
>       */
>      if ( pa_range_info[system_cpuinfo.mm64.pa_range].pabits < 
> p2m_ipa_bits )
>          p2m_ipa_bits = 
> pa_range_info[system_cpuinfo.mm64.pa_range].pabits;
> -
> -    /*
> -     * cpu info sanitization made sure we support 16bits VMID only if 
> all
> -     * cores are supporting it.
> -     */
> -    if ( system_cpuinfo.mm64.vmid_bits == MM64_VMID_16_BITS_SUPPORT )
> -        max_vmid = MAX_VMID_16_BIT;
> +#endif
>
>      /* Choose suitable "pa_range" according to the resulted 
> "p2m_ipa_bits". */
>      for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
> @@ -2324,12 +2310,22 @@ void __init setup_virt_paging(void)
>      if ( pa_range >= ARRAY_SIZE(pa_range_info) || 
> !pa_range_info[pa_range].pabits )
>          panic("Unknown encoding of ID_AA64MMFR0_EL1.PARange %x\n", 
> pa_range);
>
> -    val |= VTCR_PS(pa_range);
> -    val |= VTCR_TG0_4K;
> +#ifdef CONFIG_ARM_64
> +    /*
> +     * cpu info sanitization made sure we support 16bits VMID only if 
> all
> +     * cores are supporting it.
> +     */
> +    if ( system_cpuinfo.mm64.vmid_bits == MM64_VMID_16_BITS_SUPPORT )
> +        max_vmid = MAX_VMID_16_BIT;
>
>      /* Set the VS bit only if 16 bit VMID is supported. */
>      if ( MAX_VMID == MAX_VMID_16_BIT )
>          val |= VTCR_VS;
> +#endif
> +
> +    val |= VTCR_PS(pa_range);
> +    val |= VTCR_TG0_4K;
> +
>      val |= VTCR_SL0(pa_range_info[pa_range].sl0);
>      val |= VTCR_T0SZ(pa_range_info[pa_range].t0sz);
>
> @@ -2341,7 +2337,7 @@ void __init setup_virt_paging(void)
>             p2m_ipa_bits,
>             pa_range_info[pa_range].pabits,
>             ( MAX_VMID == MAX_VMID_16_BIT ) ? 16 : 8);
> -#endif
> +
>      printk("P2M: %d levels with order-%d root, VTCR 0x%"PRIregister"\n",
>             4 - P2M_ROOT_LEVEL, P2M_ROOT_ORDER, val);
>
> Cheers,
>

On 10/02/2023 17:51, Ayan Kumar Halder wrote:
> 
> On 10/02/2023 16:19, Julien Grall wrote:
>> Hi,
>> On 10/02/2023 15:39, Ayan Kumar Halder wrote:
>>>
>>> On 09/02/2023 11:45, Julien Grall wrote:
>>>> Hi,
>>> Hi Julien,
>>>>
>>>> On 07/02/2023 15:34, Ayan Kumar Halder wrote:
>>>>>
>>>>> On 20/01/2023 11:06, Julien Grall wrote:
>>>>>> Hi Ayan,
>>>>> Hi Julien,
>>>>>>
>>>>>> On 17/01/2023 17:43, Ayan Kumar Halder wrote:
>>>>>>> VTCR.T0SZ should be set as 0x20 to support 32bit IPA.
>>>>>>> Refer ARM DDI 0487I.a ID081822, G8-9824, G8.2.171, VTCR,
>>>>>>> "Virtualization Translation Control Register" for the bit 
>>>>>>> descriptions.
>>>>>>>
>>>>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>>>>>> ---
>>>>>>> Changes from -
>>>>>>>
>>>>>>> v1 - New patch.
>>>>>>>
>>>>>>>   xen/arch/arm/p2m.c | 10 +++++++---
>>>>>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>>>>> index 948f199d84..cfdea55e71 100644
>>>>>>> --- a/xen/arch/arm/p2m.c
>>>>>>> +++ b/xen/arch/arm/p2m.c
>>>>>>> @@ -2266,13 +2266,17 @@ void __init setup_virt_paging(void)
>>>>>>>       register_t val = 
>>>>>>> VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
>>>>>>>     #ifdef CONFIG_ARM_32
>>>>>>> -    if ( p2m_ipa_bits < 40 )
>>>>>>> +    if ( p2m_ipa_bits < PADDR_BITS )
>>>>>>>           panic("P2M: Not able to support %u-bit IPA at the 
>>>>>>> moment\n",
>>>>>>>                 p2m_ipa_bits);
>>>>>>>   -    printk("P2M: 40-bit IPA\n");
>>>>>>> -    p2m_ipa_bits = 40;
>>>>>>> +    printk("P2M: %u-bit IPA\n",PADDR_BITS);
>>>>>>> +    p2m_ipa_bits = PADDR_BITS;
>>>>>>> +#ifdef CONFIG_ARM_PA_32
>>>>>>> +    val |= VTCR_T0SZ(0x20); /* 32 bit IPA */
>>>>>>> +#else
>>>>>>>       val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
>>>>>>> +#endif
>>>>>>
>>>>>> I am wondering whether this is right time to switch to an array 
>>>>>> like the arm64 code? This would allow to use 32-bit IPA also when 
>>>>>> Xen support 64-bit physical address.
>>>>>
>>>>> In AArch64, we use ID_AA64MMFR0_EL1.PARange to determine the 
>>>>> physical address range supported at runtime. This is then used as 
>>>>> an index into pa_range_info[] to determine t0sz, root_order, etc.
>>>>
>>>> It is using both the ID_AA64MMFR0_EL1 but also p2m_ipa_bits to 
>>>> decide the size.
>>> Ack.
>>>>
>>>>>
>>>>> However, for AArch32 I do not see an equivalent register (similar 
>>>>> to ID_AA64MMFR0_EL1) or any register to determine the physical 
>>>>> address range. Thus, I will prefer to keep the code as it is unless 
>>>>> you suggest any alternative.
>>>>
>>>> I looked at the Arm Arm and indeed it doesn't look like there are 
>>>> equivalent for ID_AA64MMFR0_EL1.PARange.
>>>>
>>>> However, my point was less about reading the system register but 
>>>> more about the fact we could have the code a bit more generic and 
>>>> avoid the assumption that PADDR_BITS is only modified when 
>>>> CONFIG_ARM_PA_32 is set.
>>>
>>> I had a rework at the patch. Please let me know if the following 
>>> looks better.
>>>
>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>> index 948f199d84..bc3bdf5f3e 100644
>>> --- a/xen/arch/arm/p2m.c
>>> +++ b/xen/arch/arm/p2m.c
>>> @@ -2266,14 +2266,35 @@ void __init setup_virt_paging(void)
>>>       register_t val = 
>>> VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
>>>
>>>   #ifdef CONFIG_ARM_32
>>> -    if ( p2m_ipa_bits < 40 )
>>> +    static const struct {
>>> +        unsigned int pabits; /* Physical Address Size */
>>> +        unsigned int t0sz;   /* Desired T0SZ */
>>> +        unsigned int sl0;    /* Desired SL0 */
>>
>> Hmmm... Why don't you include the root order? In fact...
>>
>>> +    } pa_range_info[] __initconst = {
>>> +        [0] = { 32,     32,     1 },
>>> +        [1] = { 40,     24,     1 },
>>
>> ... looking at the ARMv7 specification (see B3-1345 in ARM DDI 
>> 0406C.d), the root page-table is only concatenated for 40-bits.
> Sorry, I could not follow how you inferred this. Can you paste the 
> relevant snippet ?

Use of concatenated second-level translation tables
A stage 2 translation with an input address range of 31-34 bits can 
start the translation either:
• With a first-level lookup, accessing a first-level translation table 
with 2-16 entries.
• With a second-level lookup, accessing a set of concatenated 
second-level translation tables.

>>
>>> +    };
>>> +    int i = 0;
>>> +
>>> +    if ( p2m_ipa_bits < PADDR_BITS )
>>> +        panic("P2M: Not able to support %u-bit IPA at the moment\n",
>>> +              p2m_ipa_bits);
>>
>> This check seems unnecessary now.
> 
> We still need this check as arm_smmu_device_cfg_probe() invokes 
> p2m_restrict_ipa_bits(size).
> 
> And referARM IHI 0062D.cID070116 (SMMU arch version 2.0 Specification), 
> the IPA address size can be
> 
> 0.
> 
>     0b0000 32-bit.
> 
> 1.
> 
>     0b0001 36-bit.
> 
> 10.
> 
>     0b0010 40-bit.
> 
> 11.
> 
>     0b0011 42-bit.
> 
> 100.
> 
>     0b0100 44-bit.
> 
> 101.
> 
>     0b0101 48-bit.
> 
> So if p2m_ipa_bits = 36 bits and PADDR_BITS = 40, then we want to panic.

We can have the same situation on Arm64 (PADDR_BITS = 48), yet we don't 
panic(). So I don't quite understand why we would need to differ...

Or are you saying that for 64-bit we also need such check? If so, then I 
am still not sure why because p2m_ipa_bits represent the guest physical 
address space and PADDR_BITS the Xen physical address space. It is valid 
to have both of them differing.

Cheers,

-- 
Julien Grall

On Tue, 17 Jan 2023, Ayan Kumar Halder wrote:
> VTCR.T0SZ should be set as 0x20 to support 32bit IPA.
> Refer ARM DDI 0487I.a ID081822, G8-9824, G8.2.171, VTCR,
> "Virtualization Translation Control Register" for the bit descriptions.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> Changes from -
> 
> v1 - New patch.
> 
>  xen/arch/arm/p2m.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 948f199d84..cfdea55e71 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -2266,13 +2266,17 @@ void __init setup_virt_paging(void)
>      register_t val = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
>  
>  #ifdef CONFIG_ARM_32
> -    if ( p2m_ipa_bits < 40 )
> +    if ( p2m_ipa_bits < PADDR_BITS )
>          panic("P2M: Not able to support %u-bit IPA at the moment\n",
>                p2m_ipa_bits);
>  
> -    printk("P2M: 40-bit IPA\n");
> -    p2m_ipa_bits = 40;
> +    printk("P2M: %u-bit IPA\n",PADDR_BITS);
> +    p2m_ipa_bits = PADDR_BITS;
> +#ifdef CONFIG_ARM_PA_32
> +    val |= VTCR_T0SZ(0x20); /* 32 bit IPA */
> +#else
>      val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
> +#endif
>      val |= VTCR_SL0(0x1); /* P2M starts at first level */
>  #else /* CONFIG_ARM_64 */
>      static const struct {
> -- 
> 2.17.1
>