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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Jan,

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, PHYS_ADDR_T_128. So, I am hoping that defining them explicitly 
might help.
Also, the user cannot select these configs directly.

However, I am open to defining only one of them if it makes sense.

> If so, what guards against both being selected
> at the same time?
At present, we rely on "select".
> 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.

> (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.
I remember some discussion (or comment) that the physical addresses 
should be represented using 'unsigned long'.
- Ayan


> Jan