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

[Michal Orzel <michal.orzel@xxxxxxx>]

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.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.

> +       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