Hi Ayan, 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.hal...@amd.com> > --- > 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.
> + 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 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
