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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 29/11/2022 15:52, Julien Grall wrote:
> On 29/11/2022 16:23, Ayan Kumar Halder wrote:
> > On 29/11/2022 14:52, Julien Grall wrote:
> > > On 29/11/2022 14:57, Ayan Kumar Halder wrote:
> > > > Hi All,
> > > Hi,
Hi Julien,
> > 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 ?
> There should not be any 32-bit address but you don't know what the 
> device-tree is containing because this is user input.
> This is not the business of the Device-Tree parser to decide whether 
> the value should be downcasted or rejected. That's the goal of the 
> callers.
> > I think then this approach (ie "typedef u32 paddr_t" for 32 bit 
> > system) is incorrect.
> I am a bit surprised you came to this conclusion just based on the 
> above. As I said before, there are benefits to allow Xen to be built 
> with 32-bit (e.g. smaller code size and better use of the register).
> > 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 ?
> Some of the changes you propose are questionable (see below).
>
> > Or any better suggestions ?
> Rework you previous approach by not touching the Device-Tree code.
>
> > 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();
> I don't particularly like the runtime check when you should be able to 
> sanitize the values before hand.
I think we can replace BUG() with ASSERT(). This is similar to what is 
being done today.
> > +
> > +    return (void *) ((uint32_t)(ma & GENMASK(31,0)));
> & GENMASK (...) is a bit pointless here given that you above confirmed 
> the top 32-bit are zeroed.
> > +#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
> I don't understand why the probably needs to be changed here...
>
> >   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;
> ... because the code should compile without any issue. If you were 
> really concern about ignore the top 32-bit, then you could add a 
> BUG_ON() (This is OK because this is init code).
Yes, the code compiles fine without the change. I can put a BUG_ON() here.
> > 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();
> Why not returning NULL?
Ack
> > +
> > +    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();
> Same here question here.
>
> > +
> > +    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
> BUILD_BUG_ON() are used to indicate that the code would fall over the 
> check pass. I can't find the justification for this change in the 
> commit message.
I had a look at the following commit which introduced this, but I 
couldn't get the explaination for this.
commit 88e3ed61642bb393458acc7a9bd2f96edc337190
Author: Jan Beulich <jbeulich@xxxxxxxx>
Date:   Tue Sep 1 14:02:57 2015 +0200

@Jan :- Do you know why BUILD_BUG_ON() was introduced ?

> It is also not clear why you are modifying this path because so far on 
> Arm32 the xenheap and domheap are separated for good reason (i.e. lack 
> of address space). Is this going to change with Armv8-R?
See this comment

 * CONFIG_SEPARATE_XENHEAP=y
 *
 *   The xen heap is maintained as an entirely separate heap.
 *
 *   Arch code arranges for some (perhaps small) amount of physical
 *   memory to be covered by a direct mapping and registers that
 *   memory as the Xen heap (via init_xenheap_pages()) and the
 *   remainder as the dom heap.
 *
 *   This mode of operation is most commonly used by 32-bit arches
 *   where the virtual address space is insufficient to map all RAM.

This is not true for R52 as the memory is mapped 1-1. Thus "VA == PA".

I don't think the lack of virtual address space applies to R52. Thus, 
CONFIG_SEPARATE_XENHEAP=N
- Ayan

> Cheers,