Re: [XEN v4 07/11] xen/arm: Introduce choice to enable 64/32 bit physical addressing

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.03.2023 15:03, Ayan Kumar Halder wrote:
> --- a/xen/arch/Kconfig
> +++ b/xen/arch/Kconfig
> @@ -1,6 +1,12 @@
>  config 64BIT
>       bool
>  
> +config PHYS_ADDR_T_32
> +     bool
> +
> +config PHYS_ADDR_T_64
> +     bool

Do we really need both? If so, what guards against both being selected
at the same time?

Them being put in common code I consider it an at least latent issue
that you add "select"s ...

> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -9,6 +9,7 @@ config ARM_64
>       select 64BIT
>       select ARM_EFI
>       select HAS_FAST_MULTIPLY
> +     select PHYS_ADDR_T_64
>  
>  config ARM
>       def_bool y
> @@ -19,13 +20,48 @@ config ARM
>       select HAS_PMAP
>       select IOMMU_FORCE_PT_SHARE
>  
> +menu "Architecture Features"
> +
> +choice
> +     prompt "Physical address space size" if ARM_32
> +     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"
> +     select PHYS_ADDR_T_64
> +     depends on ARM_32
> +
> +config ARM_PA_BITS_48
> +     bool "40-bit"
> +     select PHYS_ADDR_T_64
> +     depends on ARM_48
> +endchoice

... only for Arm. You get away with this only because ...

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

... you tweak things here, when we're in the process of moving stuff
out of asm/types.h. (Using "unsigned long" for a 32-bit paddr_t is of
course suspicious as well - this ought to be uint32_t.)

Jan