Re: Arm: AArch32: Need suggestions to support 32 bit physical addresses

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 29/11/2022 14:52, Julien Grall wrote:
> On 29/11/2022 14:57, Ayan Kumar Halder wrote:
> > Hi All,
> Hi,
Hi Julien,

Many thanks for your inputs.

> > I am trying to gather opinions on how to support 32 bit physical 
> > addresses to enable Xen running on R52.
> > Refer Cortex R52 TRM, Section 2.2.12 "Memory Model"
> >
> > "...This is because the physical address is always the same as the 
> > virtual address...The virtual and physical address can be treated as 
> > synonyms for Cortex-R52."
> > Thus, I understand that R52 supports 32 bit physical address only. 
> > This is a bit different from Armv7 systems which supports Large 
> > Physical Address Extension (LPAE) ie 40 bit physical addresses. >
> > Please correct me if I misunderstand something. >
> > So currently, Xen supports 64 bit physical address for Arm_32 (ie 
> > Armv7) based system.
> Xen supports *up to* 64-bit physical address. This may be lower in the 
> HW (not all the Armv7 HW supports 40-bit address).
> > My aim is to enable support for 32 bit physical address.
> Technically this is already supported because this is a subset of 
> 64-bit. I can see a use case (even on non R* HW) where you may want to 
> use 32-bit paddr_t to reduce the code size (and registers used).
> But I think that's more an optimization that rather than been necessary.
>
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index 6014c0f852..4f8b5fc4be 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -56,10 +56,10 @@ static bool __init 
> > device_tree_node_compatible(const void *fdt, int node,
> >   }
> >
> >   void __init device_tree_get_reg(const __be32 **cell, u32 
> > address_cells,
> > -                                u32 size_cells, u64 *start, u64 *size)
> > +                                u32 size_cells, paddr_t *start, 
> > paddr_t *size)
> This needs to stay uint64_t because the Device-Tree may contain 64-bit 
> values and you...
Are you saying that the device tree may contain 64 bit addresses even 
though the platform is 32 bit ?
I think then this approach (ie "typedef u32 paddr_t" for 32 bit system) 
is incorrect.
Then, the other option would be to downcast 64 bit physical addresses to 
32 bits, when we need to translate pa to va.
Do you think this approach looks better ? Or any better suggestions ?

    xen/Arm: AArch32_V8R: Use 32 bits for the physical address

    In the case of MPU (unlike MMU), there is a 1-1 mapping of virtual 
address
    to physical address (ie VA = PA).

    However, the physical addresses defined for aarch32 is u64. This is 
a problem
    for aarch32 mpu systems as the physical addresses = virtual 
addresses = 32 bits.
    Thus, when the memory region for FDT is mapped, it returns the 
virtual address
    (which is the same as physical address but truncated upto the lower 
32 bits).
    Similar logic has been used to convert machine address to virtual 
address and
    for ioremap as well.

    We do not support physical addresses beyond 32 bits.
    As this logic will fail when FDT physical address is more than 32 
bits, we have
    added a BUG() to catch this.

    Thus, the following functions has been adapted :-
    early_fdt_map()
    copy_from_paddr()
    maddr_to_virt()
    ioremap_attr()

    Also disable "BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG)" as 
PADDR_BITS = 40
    and BITS_PER_LONG = 32.

    Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>

diff --git a/xen/arch/arm/include/asm/mm_mpu.h 
b/xen/arch/arm/include/asm/mm_mpu.h
index 306a4c497c..f4f5ae1488 100644
--- a/xen/arch/arm/include/asm/mm_mpu.h
+++ b/xen/arch/arm/include/asm/mm_mpu.h
@@ -89,7 +89,18 @@ static inline paddr_t __virt_to_maddr(vaddr_t va)
 static inline void *maddr_to_virt(paddr_t ma)
 {
     /* In MPU system, VA == PA. */
+#ifdef CONFIG_AARCH32_V8R
+    /*
+     * 64 bit physical addresses are not supported.
+     * Raise a bug if one encounters 64 bit address.
+     */
+    if (ma >> BITOP_BITS_PER_WORD)
+        BUG();
+
+    return (void *) ((uint32_t)(ma & GENMASK(31,0)));
+#else
     return (void *)ma;
+#endif
 }

 /*
diff --git a/xen/arch/arm/include/asm/setup.h 
b/xen/arch/arm/include/asm/setup.h
index b3330cd584..3f4ac7f475 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -119,7 +119,11 @@ extern struct bootinfo bootinfo;

 extern domid_t max_init_domid;

+#ifdef CONFIG_AARCH32_V8R
+void copy_from_paddr(void *dst, uint32_t paddr, unsigned long len);
+#else
 void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
+#endif

 size_t estimate_efi_size(unsigned int mem_nr_banks);

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 04c05d7a05..7a7386f33a 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -46,7 +46,11 @@ struct minimal_dtb_header {
  * @paddr: source physical address
  * @len: length to copy
  */
+#ifdef CONFIG_AARCH32_V8R
+void __init copy_from_paddr(void *dst, uint32_t paddr, unsigned long len)
+#else
 void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
+#endif
 {
     void *src = (void *)(unsigned long)paddr;

diff --git a/xen/arch/arm/mm_mpu.c b/xen/arch/arm/mm_mpu.c
index df43621ee7..62774aebc6 100644
--- a/xen/arch/arm/mm_mpu.c
+++ b/xen/arch/arm/mm_mpu.c
@@ -29,7 +29,7 @@
 #include <asm/arm64/fw_shareinfo.h>
 #endif

-#ifdef CONFIG_AARCH32_ARMV8_R
+#ifdef CONFIG_AARCH32_V8R
 #include <asm/arm32/armv8r/sysregs.h>
 #endif

@@ -414,7 +414,18 @@ void *ioremap_attr(paddr_t pa, size_t len, unsigned 
int attributes)
         return NULL;
     }

+#ifdef CONFIG_AARCH32_V8R
+    /*
+     * 64 bit physical addresses are not supported.
+     * Raise a bug if one encounters 64 bit address.
+     */
+    if (pa >> BITOP_BITS_PER_WORD)
+        BUG();
+
+    return (void *) ((uint32_t)(pa & GENMASK(31,0)));
+#else
     return (void *)pa;
+#endif
 }

 static void clear_boot_mpumap(void)
@@ -1007,7 +1018,19 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
     nr_xen_mpumap++;

     /* VA == PA */
+#ifdef CONFIG_AARCH32_V8R
+
+    /*
+     * 64 bit physical addresses are not supported.
+     * Raise a bug if one encounters 64 bit address.
+     */
+    if (fdt_paddr >> BITOP_BITS_PER_WORD)
+        BUG();
+
+    fdt_virt = (void *) ((uint32_t)(fdt_paddr & GENMASK(31,0)));
+#else
     fdt_virt = (void *)fdt_paddr;
+#endif

     if ( fdt_magic(fdt_virt) != FDT_MAGIC )
         return NULL;
@@ -1165,13 +1188,13 @@ void __init setup_protection_regions()
         {
             pr_t region;
             access_protection_region(true, &region, NULL, i);
-#ifdef CONFIG_AARCH32_ARMV8_R
+#ifdef CONFIG_AARCH32_V8R
             printk("Boot-time Xen MPU memory configuration. #%u : 
0x%"PRIx32" - 0x%"PRIx32".\n",
                    i, pr_get_base(&region), pr_get_limit(&region));
-#else /* CONFIG_AARCH32_ARMV8_R */
+#else
             printk("Boot-time Xen MPU memory configuration. #%u : 
0x%"PRIx64" - 0x%"PRIx64".\n",
                    i, pr_get_base(&region), pr_get_limit(&region));
-#endif /* CONFIG_AARCH32_ARMV8_R */
+#endif
         }
 }

@@ -1262,8 +1285,13 @@ static int __init relocate_xen_mpumap(void)
     if ( !xen_mpumap )
         return -EINVAL;

+#ifdef CONFIG_AARCH32_V8R
+    copy_from_paddr(xen_mpumap, (uint32_t)(pr_t *)boot_mpumap,
+                    sizeof(pr_t) * next_xen_mpumap_index);
+#else
     copy_from_paddr(xen_mpumap, (paddr_t)(pr_t *)boot_mpumap,
                     sizeof(pr_t) * next_xen_mpumap_index);
+#endif

     clear_boot_mpumap();

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 62afb07bc6..a73bf7de01 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2245,7 +2245,9 @@ void __init xenheap_max_mfn(unsigned long mfn)
 {
     ASSERT(!first_node_initialised);
     ASSERT(!xenheap_bits);
+#ifndef CONFIG_AARCH32_V8R
     BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
+#endif
     xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);
     printk(XENLOG_INFO "Xen heap: %u bits\n", xenheap_bits);
 }

- Ayan