Re: [XEN v2 09/11] xen/arm: Introduce ARM_PA_32 to support 32 bit physical address

[Julien Grall <julien@xxxxxxx>]

Hi Ayan,

On 17/01/2023 17:43, Ayan Kumar Halder wrote:
> We have introduced a new config option to support 32 bit physical address.
> By default, it is disabled.
> ARM_PA_32 cannot be enabled on ARM_64 as the memory management unit works
> on 48bit physical addresses.
I don't understand the "cannot" here. It is possible to have a 64-bit HW 
that support only 32-bit physical address.
After your series, I also don't see any restriction in Xen to enable 
ARM_PA_32.
Whether we want to do it is a different discussion. I don't have any 
strong opinion. But the wording should be clarified.
> On ARM_32, it can be used on systems where large page address extension is
> not supported.
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> ---
> Changes from -
>
> v1 - 1. No changes.
>
>   xen/arch/arm/Kconfig                 | 9 +++++++++
>   xen/arch/arm/include/asm/page-bits.h | 2 ++
>   xen/arch/arm/include/asm/types.h     | 7 +++++++
>   3 files changed, 18 insertions(+)
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 239d3aed3c..aeb0f7252e 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -39,6 +39,15 @@ config ACPI
>   config ARM_EFI
>         bool
>   
> +config ARM_PA_32
> +       bool "32 bit Physical Address"
> +       depends on ARM_32
> +       default n
> +       ---help---
> +
> +         Support 32 bit physical addresses.
The description is a bit misleading. If you select N, then you can still 
still boot on HW supporting only 32-bit physical address.
It is only not clear from the description why a user may want to select it.

From an external interface PoV, I think it would be better if we let 
the user decide how much physical address bits they want Xen to support.
In the Kconfig, this would translate as a "choice". For Arm64, there 
will only be one (48 bits) where-as Arm32 there would be two (32, 40).

For an internal interface PoV, this could still translate to select 
ARM_PA_32 (or whichever name we decide) to indicate the type of paddr_t.
> +         If unsure, say N
> +
>   config GICV3
>         bool "GICv3 driver"
>         depends on !NEW_VGIC
> 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 083acbd151..f9595b9098 100644
> --- a/xen/arch/arm/include/asm/types.h
> +++ b/xen/arch/arm/include/asm/types.h
> @@ -37,9 +37,16 @@ typedef signed long long s64;
>   typedef unsigned long long u64;
>   typedef u32 vaddr_t;
>   #define PRIvaddr PRIx32
> +#if defined(CONFIG_ARM_PA_32)
> +typedef u32 paddr_t;
> +#define INVALID_PADDR (~0UL)
> +#define PADDR_SHIFT BITS_PER_LONG
> +#define PRIpaddr PRIx32
> +#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)
Cheers,

--
Julien Grall