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

Ayan Kumar Halder posted 10 patches 1 year, 2 months ago
There is a newer version of this series
[XEN v5 05/10] xen/arm: Introduce choice to enable 64/32 bit physical addressing
Posted by Ayan Kumar Halder 1 year, 2 months ago
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@amd.com>
---
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
+	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"
+	depends on ARM_48
+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
+
 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);
     BUILD_BUG_ON(sizeof(struct page_info) != PAGE_INFO_SIZE);
 
     if ( frametable_size > FRAMETABLE_SIZE )
-- 
2.17.1
Re: [XEN v5 05/10] xen/arm: Introduce choice to enable 64/32 bit physical addressing
Posted by Michal Orzel 1 year, 2 months ago
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@amd.com>
> ---
> 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
Re: [XEN v5 05/10] xen/arm: Introduce choice to enable 64/32 bit physical addressing
Posted by Ayan Kumar Halder 1 year, 2 months ago
On 24/04/2023 13:08, Michal Orzel wrote:
> Hi Ayan,

Hi Michal,

A clarification.

>
> 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@amd.com>
>> ---
>> 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.

Currently, I intentend to support 32-bit physical address configuration 
for ARM_32 only. So in case of ARM_32, user will have a choice between 
32-bit and 40-bit PA.

So, if it is ok with you, can I remove the below line and "config 
ARM_PA_BITS_48 ..." ?

Then ...

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

default 48 if ARM_64

- Ayan

> 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
Re: [XEN v5 05/10] xen/arm: Introduce choice to enable 64/32 bit physical addressing
Posted by Michal Orzel 1 year, 2 months ago
Hi Ayan,

On 25/04/2023 17:16, Ayan Kumar Halder wrote:
> 
> On 24/04/2023 13:08, Michal Orzel wrote:
>> Hi Ayan,
> 
> Hi Michal,
> 
> A clarification.
> 
>>
>> 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@amd.com>
>>> ---
>>> 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.
> 
> Currently, I intentend to support 32-bit physical address configuration 
> for ARM_32 only. So in case of ARM_32, user will have a choice between 
> 32-bit and 40-bit PA.
> 
> So, if it is ok with you, can I remove the below line and "config 
> ARM_PA_BITS_48 ..." ?
I'm ok with that. I'm also ok to keep ARM_PA_BITS_48 but with ARM_32 protection removed to that the option makes sense.
However, since there is no real choice for arm64, I think the former is better.

~Michal
Re: [XEN v5 05/10] xen/arm: Introduce choice to enable 64/32 bit physical addressing
Posted by Jan Beulich 1 year, 2 months ago
On 13.04.2023 19:37, Ayan Kumar Halder wrote:
> --- 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
> +	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

May I suggest that "help" come generally last, and preferably "depends on"
before "select"?

Jan