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

[Michal Orzel <michal.orzel@xxxxxxx>]

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@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'.
>>>
>>> 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