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 16:53, Jan Beulich wrote:
> 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"'?
Yes, that was what I meant.
> 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.
I understand your point. I am assuming that as PHYS_ADDR_T_* is now 
introduced at xen/arch/Kconfig, so x86 or RISC-V should be able to 
select the option.
As I see today, for :-

RISCV defines PADDR_BITS to 56. Thus, it will select PHYS_ADDR_T_64 only.

X86 defines PADDR_BITS to 52. Thus, it will also select PHYS_ADDR_T_64 only.

For Arm, there will be at least two configurations 1. which selects 
PHYS_ADDR_T_64   2. not select PHYS_ADDR_T_64 (ie for 32 bit physical 
address).
> > > (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 don't have a strong objection except that this is similar to what 
linux is doing today.
https://elixir.bootlin.com/linux/latest/source/arch/arm/include/asm/page.h#L12

> > I remember some discussion (or comment) that the physical addresses
> > should be represented using 'unsigned long'.
> A reference would be helpful.
https://lists.xenproject.org/archives/html/xen-devel/2023-02/msg00305.html

- Ayan

> Jan