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

[Julien Grall <julien@xxxxxxx>]

Hi Ayan,

On 28/04/2023 18:55, Ayan Kumar Halder wrote:
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 239d3aed3c..192582b61d 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -19,13 +19,41 @@ 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_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"
> +       depends on ARM_32
> +       select PHYS_ADDR_T_32
> +       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.
> +
> +config ARM_PA_BITS_40
> +       bool "40-bit"
> +       depends on 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_64
> +
>   config ARCH_DEFCONFIG
Any particular reason to move this config under "Architectures 
features"? IOW... Why didn't you add...
>         string
>         default "arch/arm/configs/arm32_defconfig" if ARM_32
>         default "arch/arm/configs/arm64_defconfig" if ARM_64
>   
> -menu "Architecture Features"
> -
... your new config here rather than moving "menu"?

>   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;
Looking at this again, I think this needs an explanation in the commit 
message and Kconfig at least and possibly in the code why we are not 
using uint32_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 74f6ff2c6f..5ef5fd8c49 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -703,6 +703,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) * BITS_PER_BYTE) < PADDR_BITS);
>       BUILD_BUG_ON(sizeof(struct page_info) != PAGE_INFO_SIZE);
>   
>       if ( frametable_size > FRAMETABLE_SIZE )
Cheers,

--
Julien Grall