Re: [XEN v5 05/10] xen/arm: Introduce choice to enable 64/32 bit physical addressing

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 24/04/2023 13:08, Michal Orzel wrote:
> Hi Ayan,
Hi Michal,

A clarification.

> On 13/04/2023 19:37, Ayan Kumar Halder wrote:
> > Some Arm based hardware platforms which does not support LPAE
> > (eg Cortex-R52), uses 32 bit physical addresses.
> > Also, users may choose to use 32 bits to represent physical addresses
> > for optimization.
> >
> > To support the above use cases, we have introduced arch independent
> > configs to choose if the physical address can be represented using
> > 32 bits (PHYS_ADDR_T_32) or 64 bits (!PHYS_ADDR_T_32).
> > For now only ARM_32 provides support to enable 32 bit physical
> > addressing.
> >
> > When PHYS_ADDR_T_32 is defined, PADDR_BITS is set to 32.
> > When PHYS_ADDR_T_32 is not defined for ARM_32, PADDR_BITS is set to 40.
> > When PHYS_ADDR_T_32 is not defined for ARM_64, PADDR_BITS is set to 48.
> > The last two are same as the current configuration used today on Xen.
> >
> > PADDR_BITS is also set to 48 when ARM_64 is defined. The reason being
> > the choice to select ARM_PA_BITS_32/ARM_PA_BITS_40/ARM_PA_BITS_48 is
> > currently allowed when ARM_32 is defined.
> >
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > ---
> > Changes from -
> > v1 - 1. Extracted from "[XEN v1 8/9] xen/arm: Other adaptations required to support 
> > 32bit paddr".
> >
> > v2 - 1. Introduced Kconfig choice. ARM_64 can select PHYS_ADDR_64 only whereas
> > ARM_32 can select PHYS_ADDR_32 or PHYS_ADDR_64.
> > 2. For CONFIG_ARM_PA_32, paddr_t is defined as 'unsigned long'.
> >
> > v3 - 1. Allow user to define PADDR_BITS by selecting different config options
> > ARM_PA_BITS_32, ARM_PA_BITS_40 and ARM_PA_BITS_48.
> > 2. Add the choice under "Architecture Features".
> >
> > v4 - 1. Removed PHYS_ADDR_T_64 as !PHYS_ADDR_T_32 means PHYS_ADDR_T_32.
> >
> >   xen/arch/Kconfig                     |  3 +++
> >   xen/arch/arm/Kconfig                 | 37 ++++++++++++++++++++++++++--
> >   xen/arch/arm/include/asm/page-bits.h |  6 +----
> >   xen/arch/arm/include/asm/types.h     |  6 +++++
> >   xen/arch/arm/mm.c                    |  5 ++++
> >   5 files changed, 50 insertions(+), 7 deletions(-)
> >
> > diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig
> > index 7028f7b74f..67ba38f32f 100644
> > --- a/xen/arch/Kconfig
> > +++ b/xen/arch/Kconfig
> > @@ -1,6 +1,9 @@
> >   config 64BIT
> >          bool
> >
> > +config PHYS_ADDR_T_32
> > +       bool
> > +
> >   config NR_CPUS
> >          int "Maximum number of CPUs"
> >          range 1 4095
> > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > index 239d3aed3c..3f6e13e475 100644
> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -19,13 +19,46 @@ config ARM
> >          select HAS_PMAP
> >          select IOMMU_FORCE_PT_SHARE
> >
> > +menu "Architecture Features"
> > +
> > +choice
> > +       prompt "Physical address space size" if ARM_32
> Why is it protected by ARM_32 but in the next line you add something protected 
> by ARM_64?
> This basically means that for arm64, ARM_PA_BITS_XXX is never defined.
Currently, I intentend to support 32-bit physical address configuration 
for ARM_32 only. So in case of ARM_32, user will have a choice between 
32-bit and 40-bit PA.
So, if it is ok with you, can I remove the below line and "config 
ARM_PA_BITS_48 ..." ?
Then ...

> > +       default ARM_PA_BITS_48 if ARM_64
> > +       default ARM_PA_BITS_40 if ARM_32
> > +       help
> > +         User can choose to represent the width of physical address. This can
> > +         sometimes help in optimizing the size of image when user chooses a
> > +         smaller size to represent physical address.
> > +
> > +config ARM_PA_BITS_32
> > +       bool "32-bit"
> > +       help
> > +         On platforms where any physical address can be represented within 32 
> > bits,
> > +         user should choose this option. This will help is reduced size of the
> > +         binary.
> > +       select PHYS_ADDR_T_32
> > +       depends on ARM_32
> > +
> > +config ARM_PA_BITS_40
> > +       bool "40-bit"
> > +       depends on ARM_32
> > +
> > +config ARM_PA_BITS_48
> > +       bool "40-bit"
> 40-bit? I think this should be 48-bit.
>
> > +       depends on ARM_48
> What is ARM_48? Shouldn't it be ARM_64?
> And if so, why bother defining it given everything here is protected by ARM_32.
>
> > +endchoice
> > +
> > +config PADDR_BITS
> > +       int
> > +       default 32 if ARM_PA_BITS_32
> > +       default 40 if ARM_PA_BITS_40
> > +       default 48 if ARM_PA_BITS_48 || ARM_64
default 48 if ARM_64

- Ayan

> This reads as if on arm32 we could have 48-bit PA space which is not true (LPAE 
> is 40 bit unless I missed something).
> You could get rid of || ARM_64 if the choice wasn't protected by ARM_32 and 
> fixing ARM_48 to ARM_64.
>
> > +
> >   config ARCH_DEFCONFIG
> >          string
> >          default "arch/arm/configs/arm32_defconfig" if ARM_32
> >          default "arch/arm/configs/arm64_defconfig" if ARM_64
> >
> > -menu "Architecture Features"
> > -
> >   source "arch/Kconfig"
> >
> >   config ACPI
> > diff --git a/xen/arch/arm/include/asm/page-bits.h 
> > b/xen/arch/arm/include/asm/page-bits.h
> > index 5d6477e599..deb381ceeb 100644
> > --- a/xen/arch/arm/include/asm/page-bits.h
> > +++ b/xen/arch/arm/include/asm/page-bits.h
> > @@ -3,10 +3,6 @@
> >
> >   #define PAGE_SHIFT              12
> >
> > -#ifdef CONFIG_ARM_64
> > -#define PADDR_BITS              48
> > -#else
> > -#define PADDR_BITS              40
> > -#endif
> > +#define PADDR_BITS              CONFIG_PADDR_BITS
> >
> >   #endif /* __ARM_PAGE_SHIFT_H__ */
> > diff --git a/xen/arch/arm/include/asm/types.h b/xen/arch/arm/include/asm/types.h
> > index e218ed77bd..e3cfbbb060 100644
> > --- a/xen/arch/arm/include/asm/types.h
> > +++ b/xen/arch/arm/include/asm/types.h
> > @@ -34,9 +34,15 @@ typedef signed long long s64;
> >   typedef unsigned long long u64;
> >   typedef u32 vaddr_t;
> >   #define PRIvaddr PRIx32
> > +#if defined(CONFIG_PHYS_ADDR_T_32)
> > +typedef unsigned long paddr_t;
> > +#define INVALID_PADDR (~0UL)
> > +#define PRIpaddr "08lx"
> > +#else
> >   typedef u64 paddr_t;
> >   #define INVALID_PADDR (~0ULL)
> >   #define PRIpaddr "016llx"
> > +#endif
> >   typedef u32 register_t;
> >   #define PRIregister "08x"
> >   #elif defined (CONFIG_ARM_64)
> > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > index b99806af99..6dc37be97e 100644
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -690,6 +690,11 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t 
> > pe)
> >       const unsigned long mapping_size = frametable_size < MB(32) ? MB(2) : 
> > MB(32);
> >       int rc;
> >
> > +    /*
> > +     * The size of paddr_t should be sufficient for the complete range of
> > +     * physical address.
> > +     */
> > +    BUILD_BUG_ON((sizeof(paddr_t) * 8) < PADDR_BITS);
> Just FYI, there is a macro BITS_PER_BYTE defined in bitops.h that you could use 
> instead of 8.
> Although I'm not sure if this would be better :)
>
> >       BUILD_BUG_ON(sizeof(struct page_info) != PAGE_INFO_SIZE);
> >
> >       if ( frametable_size > FRAMETABLE_SIZE )
> > --
> > 2.17.1
> ~Michal