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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.03.2023 17:15, Ayan Kumar Halder wrote:
> On 21/03/2023 14:22, Jan Beulich wrote:
>> 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?
> I was thinking the same. I am assuming that in future we may have
> 
> PHYS_ADDR_T_16,

Really? What contemporary system would be able to run in just 64k?
Certainly not Xen, let alone any Dom0 on top of it.

> PHYS_ADDR_T_128. So, I am hoping that defining them explicitly might help.

Yes, 128-bit addresses may appear at some point. Then (and only then)
we'll need two controls, and we can then think about how to represent
them properly without risking issues.

> Also, the user cannot select these configs directly.

Sure, but at some point some strange combination of options might that
randconfig manages to construct.

>> If so, what guards against both being selected
>> at the same time?
> At present, we rely on "select".

You mean 'we rely on there being only one "select"'? Else I'm afraid I
don't understand your reply.

>> 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 PRIx32in
>>> +#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.
> 
> Are you suggesting not to add anything to asm/types.h ? IOW, the above 
> snippet should
> 
> be added to xen/include/xen/types.h.

It's not your snippet alone, but the definition of paddr_t in general
which should imo be moved (as a follow-on to "common: move standard C
fixed width type declarations to common header"). If your patch in its
present shape landed first, that movement would become more
complicated - first and foremost "select"ing the appropriate
PHYS_ADDR_T_* on x86 and RISC-V would then need to be done there, when
it really doesn't belong there.

>> (Using "unsigned long" for a 32-bit paddr_t is of
>> course suspicious as well - this ought to be uint32_t.)
> 
> The problem with using uint32_t for paddr_t is that there are instances 
> where the paddr_t is modified with PAGE_MASK or PAGE_ALIGN.
> 
> For eg , handle_passthrough_prop()
> 
>              printk(XENLOG_ERR "Unable to permit to dom%d access to"
>                     " 0x%"PRIpaddr" - 0x%"PRIpaddr"\n",
>                     kinfo->d->domain_id,
>                     mstart & PAGE_MASK, PAGE_ALIGN(mstart + size) - 1);
> 
> And in xen/include/xen/page-size.h,
> 
> #define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
> #define PAGE_MASK           (~(PAGE_SIZE-1))
> 
> Thus, the resulting types are unsigned long. This cannot be printed 
> using %u for PRIpaddr.

Is there anything wrong with making PAGE_SIZE expand to (1 << PAGE_SHIFT)
when physical addresses are only 32 bits wide?

> I remember some discussion (or comment) that the physical addresses 
> should be represented using 'unsigned long'.

A reference would be helpful.

Jan