Re: [XEN v2 11/11] xen/arm: p2m: Enable support for 32bit IPA

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

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@xxxxxxx>
> > > > ---
> > > > 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,