| Message ID | 20230321140357.24094-8-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for 32 bit physical address | expand |
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? If so, what guards against both being selected at the same time? 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 PRIx32 > +#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. (Using "unsigned long" for a 32-bit paddr_t is of course suspicious as well - this ought to be uint32_t.) Jan
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
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
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
On 21.03.2023 19:33, Ayan Kumar Halder wrote: > 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: >>>> (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 I see. I guess this will be okay as long as only 32-bit arches elect to use 32-bit physical addresses. Maybe there should be a BUILD_BUG_ON() somewhere, accompanied by a suitable comment? Jan
Hi Jan, On 22/03/2023 06:59, Jan Beulich wrote: > On 21.03.2023 19:33, Ayan Kumar Halder wrote: >> 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: >>>>> (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 > > I see. I guess this will be okay as long as only 32-bit arches elect to > use 32-bit physical addresses. Maybe there should be a BUILD_BUG_ON() > somewhere, accompanied by a suitable comment? Hmmm... We definitely have 40-bits physical address space on Arm32. In fact, my suggestion was not to define paddr_t as unsigned long for everyone but only when PHYS_ADDR_T_32 is selected (AFAICT this is what is done in this patch). This is to avoid having to add cast everywhere we are using PAGE_* on paddr_t and print it. That said, I realize this means that for 64-bit, we would still use 64-bit integer. I view it as a less a problem (at least on Arm) because registers are always 64-bit. I am open to other suggestion. Cheers,
On 22.03.2023 14:29, Julien Grall wrote: > On 22/03/2023 06:59, Jan Beulich wrote: >> On 21.03.2023 19:33, Ayan Kumar Halder wrote: >>> 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: >>>>>> (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 >> >> I see. I guess this will be okay as long as only 32-bit arches elect to >> use 32-bit physical addresses. Maybe there should be a BUILD_BUG_ON() >> somewhere, accompanied by a suitable comment? > > Hmmm... We definitely have 40-bits physical address space on Arm32. In > fact, my suggestion was not to define paddr_t as unsigned long for > everyone but only when PHYS_ADDR_T_32 is selected (AFAICT this is what > is done in this patch). > > This is to avoid having to add cast everywhere we are using PAGE_* on > paddr_t and print it. > > That said, I realize this means that for 64-bit, we would still use > 64-bit integer. I view it as a less a problem (at least on Arm) because > registers are always 64-bit. I am open to other suggestion. It simply struck me as odd to use a 64-bit type for something that was explicitly said is only going to be 32 bits wide. I would therefore prefer if we could limit 32-bit paddr_t to 32-bit architectures for now, as expressed before when asking for a respective BUILD_BUG_ON(). Especially if, as intended, the type definition moves to xen/types.h (and hence isn't Arm-specific anymore). Jan
Hi Jan, Julien, On 22/03/2023 13:53, Jan Beulich wrote: > On 22.03.2023 14:29, Julien Grall wrote: >> On 22/03/2023 06:59, Jan Beulich wrote: >>> On 21.03.2023 19:33, Ayan Kumar Halder wrote: >>>> 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: >>>>>>> (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 >>> I see. I guess this will be okay as long as only 32-bit arches elect to >>> use 32-bit physical addresses. Maybe there should be a BUILD_BUG_ON() >>> somewhere, accompanied by a suitable comment? >> Hmmm... We definitely have 40-bits physical address space on Arm32. In >> fact, my suggestion was not to define paddr_t as unsigned long for >> everyone but only when PHYS_ADDR_T_32 is selected (AFAICT this is what >> is done in this patch). >> >> This is to avoid having to add cast everywhere we are using PAGE_* on >> paddr_t and print it. >> >> That said, I realize this means that for 64-bit, we would still use >> 64-bit integer. I view it as a less a problem (at least on Arm) because >> registers are always 64-bit. I am open to other suggestion. > It simply struck me as odd to use a 64-bit type for something that was > explicitly said is only going to be 32 bits wide. I would therefore > prefer if we could limit 32-bit paddr_t to 32-bit architectures for > now, as expressed before when asking for a respective BUILD_BUG_ON(). > Especially if, as intended, the type definition moves to xen/types.h > (and hence isn't Arm-specific anymore). > > Jan Please have a look at the below patch and let me know your thoughts. This patch now :- 1. Removes the config "PHYS_ADDR_T_64". So when PHYS_ADDR_T_32 is not selected, it means that physical addresses are to be denoted by 64 bits. 2. Added a BUILD_BUG_ON() to check that paddr_t is exactly 32-bit wide when CONFIG_PHYS_ADDR_T_32 is selected. As 32-bit Arm architecture (Arm_32) can support 40 bits PA with LPAE, thus we cannot always use 32-bit paddr_t. 3. For Jan's concern that the changes to xen/arch/arm/include/asm/types.h will complicate movement to common header, I think we will need to use CONFIG_PHYS_ADDR_T_32 to define types for 32-bit physical addresses. I am open to any alternative suggestions that you propose. commit 3a61721a5169072b4aa3bbd0df38de5e69a5abc1 Author: Ayan Kumar Halder <ayan.kumar.halder@amd.com> Date: Wed Feb 8 12:05:26 2023 +0000 xen/arm: Introduce choice to enable 64/32 bit physical addressing 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@amd.com> 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..e6dadeb8b1 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 + 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" + depends on ARM_32 + +config ARM_PA_BITS_48 + bool "40-bit" + depends on ARM_48 +endchoice + +config PADDR_BITS + int + default 32 if ARM_PA_BITS_32 + default 40 if ARM_PA_BITS_40 + default 48 if ARM_PA_BITS_48 || ARM_64 + config ARCH_DEFCONFIG string default "arch/arm/configs/arm32_defconfig" if ARM_32 default "arch/arm/configs/arm64_defconfig" if ARM_64 -menu "Architecture Features" - source "arch/Kconfig" config ACPI diff --git a/xen/arch/arm/include/asm/page-bits.h b/xen/arch/arm/include/asm/page-bits.h index 5d6477e599..deb381ceeb 100644 --- a/xen/arch/arm/include/asm/page-bits.h +++ b/xen/arch/arm/include/asm/page-bits.h @@ -3,10 +3,6 @@ #define PAGE_SHIFT 12 -#ifdef CONFIG_ARM_64 -#define PADDR_BITS 48 -#else -#define PADDR_BITS 40 -#endif +#define PADDR_BITS CONFIG_PADDR_BITS #endif /* __ARM_PAGE_SHIFT_H__ */ diff --git a/xen/arch/arm/include/asm/types.h b/xen/arch/arm/include/asm/types.h index e218ed77bd..e3cfbbb060 100644 --- 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 PRIx32 +#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) diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index b99806af99..6dc37be97e 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -690,6 +690,11 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe) const unsigned long mapping_size = frametable_size < MB(32) ? MB(2) : MB(32); int rc; + /* Check that paddr_t is exactly 32 bits when CONFIG_PHYS_ADDR_T_32 is defined */ + #ifdef CONFIG_PHYS_ADDR_T_32 + BUILD_BUG_ON((sizeof(paddr_t) * 8) != 32); + #endif + /* + * The size of paddr_t should be sufficient for the complete range of + * physical address. + */ + BUILD_BUG_ON((sizeof(paddr_t) * 8) < PADDR_BITS); BUILD_BUG_ON(sizeof(struct page_info) != PAGE_INFO_SIZE); if ( frametable_size > FRAMETABLE_SIZE ) - Ayan
Hi, On 27/03/2023 12:46, Ayan Kumar Halder wrote: > On 22/03/2023 13:53, Jan Beulich wrote: >> On 22.03.2023 14:29, Julien Grall wrote: >>> On 22/03/2023 06:59, Jan Beulich wrote: >>>> On 21.03.2023 19:33, Ayan Kumar Halder wrote: >>>>> 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: >>>>>>>> (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 >>>> I see. I guess this will be okay as long as only 32-bit arches elect to >>>> use 32-bit physical addresses. Maybe there should be a BUILD_BUG_ON() >>>> somewhere, accompanied by a suitable comment? >>> Hmmm... We definitely have 40-bits physical address space on Arm32. In >>> fact, my suggestion was not to define paddr_t as unsigned long for >>> everyone but only when PHYS_ADDR_T_32 is selected (AFAICT this is what >>> is done in this patch). >>> >>> This is to avoid having to add cast everywhere we are using PAGE_* on >>> paddr_t and print it. >>> >>> That said, I realize this means that for 64-bit, we would still use >>> 64-bit integer. I view it as a less a problem (at least on Arm) because >>> registers are always 64-bit. I am open to other suggestion. >> It simply struck me as odd to use a 64-bit type for something that was >> explicitly said is only going to be 32 bits wide. I would therefore >> prefer if we could limit 32-bit paddr_t to 32-bit architectures for >> now, as expressed before when asking for a respective BUILD_BUG_ON(). >> Especially if, as intended, the type definition moves to xen/types.h >> (and hence isn't Arm-specific anymore). >> >> Jan > > Please have a look at the below patch and let me know your thoughts. > This patch now :- > > 1. Removes the config "PHYS_ADDR_T_64". So when PHYS_ADDR_T_32 is not > selected, it means that physical addresses are to be denoted by 64 bits. > > 2. Added a BUILD_BUG_ON() to check that paddr_t is exactly 32-bit wide > when CONFIG_PHYS_ADDR_T_32 is selected. As 32-bit Arm architecture > (Arm_32) can support 40 bits PA with LPAE, thus we cannot always use > 32-bit paddr_t. > > 3. For Jan's concern that the changes to > xen/arch/arm/include/asm/types.h will complicate movement to common > header, I think we will need to use CONFIG_PHYS_ADDR_T_32 to define > types for 32-bit physical addresses. > > I am open to any alternative suggestions that you propose. > > > commit 3a61721a5169072b4aa3bbd0df38de5e69a5abc1 > Author: Ayan Kumar Halder <ayan.kumar.halder@amd.com> > Date: Wed Feb 8 12:05:26 2023 +0000 > > xen/arm: Introduce choice to enable 64/32 bit physical addressing > > 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@amd.com> > > 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..e6dadeb8b1 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 > + 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 Typo: I think it is more common to have the ',' at the end of the line rather than a the beginning followed by a space. > + binary. > + select PHYS_ADDR_T_32 > + depends on ARM_32 > + > +config ARM_PA_BITS_40 > + bool "40-bit" > + depends on ARM_32 > + > +config ARM_PA_BITS_48 > + bool "40-bit" > + depends on ARM_48 > +endchoice > + > +config PADDR_BITS > + int > + default 32 if ARM_PA_BITS_32 > + default 40 if ARM_PA_BITS_40 > + default 48 if ARM_PA_BITS_48 || ARM_64 > + > config ARCH_DEFCONFIG > string > default "arch/arm/configs/arm32_defconfig" if ARM_32 > default "arch/arm/configs/arm64_defconfig" if ARM_64 > > -menu "Architecture Features" > - > source "arch/Kconfig" > > config ACPI > diff --git a/xen/arch/arm/include/asm/page-bits.h > b/xen/arch/arm/include/asm/page-bits.h > index 5d6477e599..deb381ceeb 100644 > --- a/xen/arch/arm/include/asm/page-bits.h > +++ b/xen/arch/arm/include/asm/page-bits.h > @@ -3,10 +3,6 @@ > > #define PAGE_SHIFT 12 > > -#ifdef CONFIG_ARM_64 > -#define PADDR_BITS 48 > -#else > -#define PADDR_BITS 40 > -#endif > +#define PADDR_BITS CONFIG_PADDR_BITS > > #endif /* __ARM_PAGE_SHIFT_H__ */ > diff --git a/xen/arch/arm/include/asm/types.h > b/xen/arch/arm/include/asm/types.h > index e218ed77bd..e3cfbbb060 100644 > --- 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 PRIx32 > +#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) > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index b99806af99..6dc37be97e 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -690,6 +690,11 @@ void __init setup_frametable_mappings(paddr_t ps, > paddr_t pe) > const unsigned long mapping_size = frametable_size < MB(32) ? > MB(2) : MB(32); > int rc; > > + /* Check that paddr_t is exactly 32 bits when CONFIG_PHYS_ADDR_T_32 > is defined */ This is only describing the BUILD_BUG_ON() in words but don't really say why. In fact... > > + #ifdef CONFIG_PHYS_ADDR_T_32 > > + BUILD_BUG_ON((sizeof(paddr_t) * 8) != 32); > > + #endif ... nothing really wrong will happen if paddr_t is bigger. The code will just be less optimized. So I would drop it. If there is a desire to keep it then it should be moved in build_assertions() with a suitable comment (e.g. what could go wrong if the build assertion fail). Cheers,
diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig index 7028f7b74f..89096c77a4 100644 --- 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 + 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..13e3a23911 100644 --- 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 + +config PADDR_BITS + int + default 32 if ARM_PA_BITS_32 + default 40 if ARM_PA_BITS_40 + default 48 if ARM_PA_BITS_48 || ARM_64 + config ARCH_DEFCONFIG string default "arch/arm/configs/arm32_defconfig" if ARM_32 default "arch/arm/configs/arm64_defconfig" if ARM_64 -menu "Architecture Features" - source "arch/Kconfig" config ACPI diff --git a/xen/arch/arm/include/asm/page-bits.h b/xen/arch/arm/include/asm/page-bits.h index 5d6477e599..deb381ceeb 100644 --- a/xen/arch/arm/include/asm/page-bits.h +++ b/xen/arch/arm/include/asm/page-bits.h @@ -3,10 +3,6 @@ #define PAGE_SHIFT 12 -#ifdef CONFIG_ARM_64 -#define PADDR_BITS 48 -#else -#define PADDR_BITS 40 -#endif +#define PADDR_BITS CONFIG_PADDR_BITS #endif /* __ARM_PAGE_SHIFT_H__ */ diff --git a/xen/arch/arm/include/asm/types.h b/xen/arch/arm/include/asm/types.h index e218ed77bd..e3cfbbb060 100644 --- 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 PRIx32 +#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) diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index b99806af99..d8b43ef38c 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -690,6 +690,7 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe) const unsigned long mapping_size = frametable_size < MB(32) ? MB(2) : MB(32); int rc; + BUILD_BUG_ON((sizeof(paddr_t) * 8) < PADDR_BITS); BUILD_BUG_ON(sizeof(struct page_info) != PAGE_INFO_SIZE); if ( frametable_size > FRAMETABLE_SIZE )
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_64). 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_64 is defined with ARM_32, PADDR_BITS is set to 40. When PHYS_ADDR_T_64 is defined with 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@amd.com> --- 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". xen/arch/Kconfig | 6 +++++ xen/arch/arm/Kconfig | 40 ++++++++++++++++++++++++++-- xen/arch/arm/include/asm/page-bits.h | 6 +---- xen/arch/arm/include/asm/types.h | 6 +++++ xen/arch/arm/mm.c | 1 + 5 files changed, 52 insertions(+), 7 deletions(-)