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
