Re: [XEN v3 6/9] xen/arm: Introduce choice to enable 64/32 bit physical addressing

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 8 Feb 2023, 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_32) or 64 bits (PHYS_ADDR_64).
> For now only ARM_32 provides support to enable 32 bit physical
> addressing.
> 
> 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'.
> 
> (Jan,Julien please let me know if I understood your suggestion about Kconfig 
> correctly).
> 
>  xen/arch/Kconfig                     | 12 +++++++++++
>  xen/arch/arm/Kconfig                 | 31 ++++++++++++++++++++++++++++
>  xen/arch/arm/include/asm/page-bits.h |  2 ++
>  xen/arch/arm/include/asm/types.h     |  6 ++++++
>  4 files changed, 51 insertions(+)
> 
> diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig
> index 7028f7b74f..1eff312b51 100644
> --- a/xen/arch/Kconfig
> +++ b/xen/arch/Kconfig
> @@ -1,6 +1,18 @@
>  config 64BIT
>       bool
>  
> +config PHYS_ADDR_32
> +     bool
> +     help
> +       Select this option if the physical addresses can be represented by
> +       32 bits.
> +
> +config PHYS_ADDR_64
> +     bool
> +     help
> +       Select this option if the physical addresses can be represented
> +       64 bits.

These two config symbols should be defined in xen/arch/arm/Kconfig
(unless you plan to also define them for x86).


>  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..0955891e86 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -18,6 +18,37 @@ config ARM
>       select HAS_PDX
>       select HAS_PMAP
>       select IOMMU_FORCE_PT_SHARE
> +     choice

This shows up at the very beginning of the global menu. I think it would
be better to move it under "Architecture Features"

Also given that there is not choice on arm64, I don't think the choice
option should be visible to the user (because the user cannot change
selection for arm64).

> +             bool "Representative width for any physical address (default 
> 64bit)"
> +             optional

should not be "optional", optional means that neither ARM_PA_64 nor
ARM_PA_32 could be selected. I think we want one of the two to always be
selected, ARM_PA_64 being the default option, and the only option on
arm64.

> +             ---help---
> +               You may want to specify the width to represent the physical
> +               address space.
> +               By default, the physical addresses are represented using
> +               64 bits.
> +
> +               However in certain platforms, the physical addresses may be
> +               represented using 32 bits.
> +               Also, the user may decide that the physical addresses can be
> +               represented using 32 bits for a given SoC. In those cases,
> +               user may want to enable 32 bit physical address for
> +               optimization.
> +               For now, we have enabled this choice for ARM_32 only.
> +
> +             config ARM_PA_64
> +                     select PHYS_ADDR_64
> +                     bool "Select 64 bits to represent physical address"
> +                     ---help---
> +                       Use 64 bits to represent physical address.
> +
> +             config ARM_PA_32
> +                     select PHYS_ADDR_32
> +                     depends on ARM_32
> +                     bool "Select 32 bits to represent physical address"
> +                     ---help---
> +                       Use 32 bits to represent physical address.
> +
> +    endchoice
>  
>  config ARCH_DEFCONFIG
>       string
> diff --git a/xen/arch/arm/include/asm/page-bits.h 
> b/xen/arch/arm/include/asm/page-bits.h
> index 5d6477e599..8f4dcebcfd 100644
> --- a/xen/arch/arm/include/asm/page-bits.h
> +++ b/xen/arch/arm/include/asm/page-bits.h
> @@ -5,6 +5,8 @@
>  
>  #ifdef CONFIG_ARM_64
>  #define PADDR_BITS              48
> +#elif CONFIG_ARM_PA_32
> +#define PADDR_BITS              32
>  #else
>  #define PADDR_BITS              40
>  #endif
> diff --git a/xen/arch/arm/include/asm/types.h 
> b/xen/arch/arm/include/asm/types.h
> index e218ed77bd..26144bc87e 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_ARM_PA_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)
> -- 
> 2.17.1
> 
>