| Message ID | 20230117174358.15344-10-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for 32 bit physical address | expand |
On 17.01.2023 18:43, Ayan Kumar Halder wrote: > --- a/xen/arch/arm/include/asm/types.h > +++ b/xen/arch/arm/include/asm/types.h > @@ -37,9 +37,16 @@ typedef signed long long s64; > typedef unsigned long long u64; > typedef u32 vaddr_t; > #define PRIvaddr PRIx32 > +#if defined(CONFIG_ARM_PA_32) > +typedef u32 paddr_t; > +#define INVALID_PADDR (~0UL) > +#define PADDR_SHIFT BITS_PER_LONG > +#define PRIpaddr PRIx32 > +#else With our plan to consolidate basic type definitions into xen/types.h the use of ARM_PA_32 is problematic: Preferably we'd have an arch- independent Kconfig setting, much like Linux'es PHYS_ADDR_T_64BIT (I don't think we should re-use the name directly, as we have no phys_addr_t type), which each arch selects (or not) accordingly. Perhaps architectures already selecting 64BIT wouldn't need to do this explicitly, and instead 64BIT could select the new setting (or the new setting could default to Y when 64BIT=y). Jan
Hi Ayan, On 17/01/2023 17:43, Ayan Kumar Halder wrote: > We have introduced a new config option to support 32 bit physical address. > By default, it is disabled. > ARM_PA_32 cannot be enabled on ARM_64 as the memory management unit works > on 48bit physical addresses. I don't understand the "cannot" here. It is possible to have a 64-bit HW that support only 32-bit physical address. After your series, I also don't see any restriction in Xen to enable ARM_PA_32. Whether we want to do it is a different discussion. I don't have any strong opinion. But the wording should be clarified. > On ARM_32, it can be used on systems where large page address extension is > not supported. > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> > --- > Changes from - > > v1 - 1. No changes. > > xen/arch/arm/Kconfig | 9 +++++++++ > xen/arch/arm/include/asm/page-bits.h | 2 ++ > xen/arch/arm/include/asm/types.h | 7 +++++++ > 3 files changed, 18 insertions(+) > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig > index 239d3aed3c..aeb0f7252e 100644 > --- a/xen/arch/arm/Kconfig > +++ b/xen/arch/arm/Kconfig > @@ -39,6 +39,15 @@ config ACPI > config ARM_EFI > bool > > +config ARM_PA_32 > + bool "32 bit Physical Address" > + depends on ARM_32 > + default n > + ---help--- > + > + Support 32 bit physical addresses. The description is a bit misleading. If you select N, then you can still still boot on HW supporting only 32-bit physical address. It is only not clear from the description why a user may want to select it. From an external interface PoV, I think it would be better if we let the user decide how much physical address bits they want Xen to support. In the Kconfig, this would translate as a "choice". For Arm64, there will only be one (48 bits) where-as Arm32 there would be two (32, 40). For an internal interface PoV, this could still translate to select ARM_PA_32 (or whichever name we decide) to indicate the type of paddr_t. > + If unsure, say N > + > config GICV3 > bool "GICv3 driver" > depends on !NEW_VGIC > diff --git a/xen/arch/arm/include/asm/page-bits.h b/xen/arch/arm/include/asm/page-bits.h > index 5d6477e599..8f4dcebcfd 100644 > --- a/xen/arch/arm/include/asm/page-bits.h > +++ b/xen/arch/arm/include/asm/page-bits.h > @@ -5,6 +5,8 @@ > > #ifdef CONFIG_ARM_64 > #define PADDR_BITS 48 > +#elif CONFIG_ARM_PA_32 > +#define PADDR_BITS 32 > #else > #define PADDR_BITS 40 > #endif > diff --git a/xen/arch/arm/include/asm/types.h b/xen/arch/arm/include/asm/types.h > index 083acbd151..f9595b9098 100644 > --- a/xen/arch/arm/include/asm/types.h > +++ b/xen/arch/arm/include/asm/types.h > @@ -37,9 +37,16 @@ typedef signed long long s64; > typedef unsigned long long u64; > typedef u32 vaddr_t; > #define PRIvaddr PRIx32 > +#if defined(CONFIG_ARM_PA_32) > +typedef u32 paddr_t; > +#define INVALID_PADDR (~0UL) > +#define PADDR_SHIFT BITS_PER_LONG > +#define PRIpaddr PRIx32 > +#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) Cheers,
Hi Jan, On 18/01/2023 08:50, Jan Beulich wrote: > On 17.01.2023 18:43, Ayan Kumar Halder wrote: >> --- a/xen/arch/arm/include/asm/types.h >> +++ b/xen/arch/arm/include/asm/types.h >> @@ -37,9 +37,16 @@ typedef signed long long s64; >> typedef unsigned long long u64; >> typedef u32 vaddr_t; >> #define PRIvaddr PRIx32 >> +#if defined(CONFIG_ARM_PA_32) >> +typedef u32 paddr_t; >> +#define INVALID_PADDR (~0UL) >> +#define PADDR_SHIFT BITS_PER_LONG >> +#define PRIpaddr PRIx32 >> +#else > With our plan to consolidate basic type definitions into xen/types.h > the use of ARM_PA_32 is problematic: Preferably we'd have an arch- > independent Kconfig setting, much like Linux'es PHYS_ADDR_T_64BIT > (I don't think we should re-use the name directly, as we have no > phys_addr_t type), which each arch selects (or not) accordingly. > Perhaps architectures already selecting 64BIT wouldn't need to do > this explicitly, and instead 64BIT could select the new setting > (or the new setting could default to Y when 64BIT=y). Looking at some of the instances where 64BIT is currently used (xen/drivers/passthrough/arm/smmu.c), I understand that it is used to define the width of **virtual** address. Thus, it would not conflict with ARM_PA_32 (which is for physical address). Later on if we wish to introduce something similar to Linux's PHYS_ADDR_T_64BIT (which is arch agnostic), then ARM_PA_32 can be selected by "!PHYS_ADDR_T_64BIT" && "CONFIG_ARM_32". (We may decide to drop ARM_PA_32 config option, but we can still reuse ARM_PA_32 specific changes). Also, then we will need to support 32 bit physical address (ie !PHYS_ADDR_T_64BIT) with 32 bit virtual address (ie !64BIT) and 64 bit virtual address (ie 64BIT). Let's confine the current changes to Arm architecture only (as I don't have knowledge about x86 or RISCV). When similar changes will be done for other architectures, then we can think about introducing an architecture agnostic option. - Ayan > > Jan
On 18.01.2023 12:57, Ayan Kumar Halder wrote: > Hi Jan, > > On 18/01/2023 08:50, Jan Beulich wrote: >> On 17.01.2023 18:43, Ayan Kumar Halder wrote: >>> --- a/xen/arch/arm/include/asm/types.h >>> +++ b/xen/arch/arm/include/asm/types.h >>> @@ -37,9 +37,16 @@ typedef signed long long s64; >>> typedef unsigned long long u64; >>> typedef u32 vaddr_t; >>> #define PRIvaddr PRIx32 >>> +#if defined(CONFIG_ARM_PA_32) >>> +typedef u32 paddr_t; >>> +#define INVALID_PADDR (~0UL) >>> +#define PADDR_SHIFT BITS_PER_LONG >>> +#define PRIpaddr PRIx32 >>> +#else >> With our plan to consolidate basic type definitions into xen/types.h >> the use of ARM_PA_32 is problematic: Preferably we'd have an arch- >> independent Kconfig setting, much like Linux'es PHYS_ADDR_T_64BIT >> (I don't think we should re-use the name directly, as we have no >> phys_addr_t type), which each arch selects (or not) accordingly. >> Perhaps architectures already selecting 64BIT wouldn't need to do >> this explicitly, and instead 64BIT could select the new setting >> (or the new setting could default to Y when 64BIT=y). > > Looking at some of the instances where 64BIT is currently used > (xen/drivers/passthrough/arm/smmu.c), I understand that it is used to > define the width of **virtual** address. > > Thus, it would not conflict with ARM_PA_32 (which is for physical address). > > Later on if we wish to introduce something similar to Linux's > PHYS_ADDR_T_64BIT (which is arch agnostic), then ARM_PA_32 can be > selected by "!PHYS_ADDR_T_64BIT" && "CONFIG_ARM_32". (We may decide to > drop ARM_PA_32 config option, but we can still reuse ARM_PA_32 specific > changes). > > Also, then we will need to support 32 bit physical address (ie > !PHYS_ADDR_T_64BIT) with 32 bit virtual address (ie !64BIT) and 64 bit > virtual address (ie 64BIT). > > Let's confine the current changes to Arm architecture only (as I don't > have knowledge about x86 or RISCV). When similar changes will be done > for other architectures, then we can think about introducing an > architecture agnostic option. I disagree, not the least with the present goal of reworking xen/types.h vs asm/types.h. By having an arch-independent Kconfig symbol, paddr_t could also be manufactured uniformly in xen/types.h. Jan
On Wed, 18 Jan 2023, Jan Beulich wrote: > On 18.01.2023 12:57, Ayan Kumar Halder wrote: > > Hi Jan, > > > > On 18/01/2023 08:50, Jan Beulich wrote: > >> On 17.01.2023 18:43, Ayan Kumar Halder wrote: > >>> --- a/xen/arch/arm/include/asm/types.h > >>> +++ b/xen/arch/arm/include/asm/types.h > >>> @@ -37,9 +37,16 @@ typedef signed long long s64; > >>> typedef unsigned long long u64; > >>> typedef u32 vaddr_t; > >>> #define PRIvaddr PRIx32 > >>> +#if defined(CONFIG_ARM_PA_32) > >>> +typedef u32 paddr_t; > >>> +#define INVALID_PADDR (~0UL) > >>> +#define PADDR_SHIFT BITS_PER_LONG > >>> +#define PRIpaddr PRIx32 > >>> +#else > >> With our plan to consolidate basic type definitions into xen/types.h > >> the use of ARM_PA_32 is problematic: Preferably we'd have an arch- > >> independent Kconfig setting, much like Linux'es PHYS_ADDR_T_64BIT > >> (I don't think we should re-use the name directly, as we have no > >> phys_addr_t type), which each arch selects (or not) accordingly. > >> Perhaps architectures already selecting 64BIT wouldn't need to do > >> this explicitly, and instead 64BIT could select the new setting > >> (or the new setting could default to Y when 64BIT=y). > > > > Looking at some of the instances where 64BIT is currently used > > (xen/drivers/passthrough/arm/smmu.c), I understand that it is used to > > define the width of **virtual** address. > > > > Thus, it would not conflict with ARM_PA_32 (which is for physical address). > > > > Later on if we wish to introduce something similar to Linux's > > PHYS_ADDR_T_64BIT (which is arch agnostic), then ARM_PA_32 can be > > selected by "!PHYS_ADDR_T_64BIT" && "CONFIG_ARM_32". (We may decide to > > drop ARM_PA_32 config option, but we can still reuse ARM_PA_32 specific > > changes). > > > > Also, then we will need to support 32 bit physical address (ie > > !PHYS_ADDR_T_64BIT) with 32 bit virtual address (ie !64BIT) and 64 bit > > virtual address (ie 64BIT). > > > > Let's confine the current changes to Arm architecture only (as I don't > > have knowledge about x86 or RISCV). When similar changes will be done > > for other architectures, then we can think about introducing an > > architecture agnostic option. > > I disagree, not the least with the present goal of reworking xen/types.h > vs asm/types.h. By having an arch-independent Kconfig symbol, paddr_t > could also be manufactured uniformly in xen/types.h. Jan is only asking to introduce the new Kconfig symbol as an arch-independent symbol. In other words, rename ARM_PA_32 to PADDR_32 (or something like that) and move the symbol to xen/arch/Kconfig. I think that's reasonable. And PADDR_32 could be forced to always be unselected on x86: I don't think Jan is asking you to rework the whole of xen/arch/x86 and xen/commons to build on x86 with paddr_t set to uint32_t.
Hi, On 17/01/2023 17:43, Ayan Kumar Halder wrote: > diff --git a/xen/arch/arm/include/asm/types.h b/xen/arch/arm/include/asm/types.h > index 083acbd151..f9595b9098 100644 > --- a/xen/arch/arm/include/asm/types.h > +++ b/xen/arch/arm/include/asm/types.h > @@ -37,9 +37,16 @@ typedef signed long long s64; > typedef unsigned long long u64; > typedef u32 vaddr_t; > #define PRIvaddr PRIx32 > +#if defined(CONFIG_ARM_PA_32) > +typedef u32 paddr_t; > +#define INVALID_PADDR (~0UL) > +#define PADDR_SHIFT BITS_PER_LONG You define PADDR_SHIFT but don't seem to use it. Is it intended? > +#define PRIpaddr PRIx32 > +#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) Cheers,
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index 239d3aed3c..aeb0f7252e 100644 --- a/xen/arch/arm/Kconfig +++ b/xen/arch/arm/Kconfig @@ -39,6 +39,15 @@ config ACPI config ARM_EFI bool +config ARM_PA_32 + bool "32 bit Physical Address" + depends on ARM_32 + default n + ---help--- + + Support 32 bit physical addresses. + If unsure, say N + config GICV3 bool "GICv3 driver" depends on !NEW_VGIC diff --git a/xen/arch/arm/include/asm/page-bits.h b/xen/arch/arm/include/asm/page-bits.h index 5d6477e599..8f4dcebcfd 100644 --- a/xen/arch/arm/include/asm/page-bits.h +++ b/xen/arch/arm/include/asm/page-bits.h @@ -5,6 +5,8 @@ #ifdef CONFIG_ARM_64 #define PADDR_BITS 48 +#elif CONFIG_ARM_PA_32 +#define PADDR_BITS 32 #else #define PADDR_BITS 40 #endif diff --git a/xen/arch/arm/include/asm/types.h b/xen/arch/arm/include/asm/types.h index 083acbd151..f9595b9098 100644 --- a/xen/arch/arm/include/asm/types.h +++ b/xen/arch/arm/include/asm/types.h @@ -37,9 +37,16 @@ typedef signed long long s64; typedef unsigned long long u64; typedef u32 vaddr_t; #define PRIvaddr PRIx32 +#if defined(CONFIG_ARM_PA_32) +typedef u32 paddr_t; +#define INVALID_PADDR (~0UL) +#define PADDR_SHIFT BITS_PER_LONG +#define PRIpaddr PRIx32 +#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)
We have introduced a new config option to support 32 bit physical address. By default, it is disabled. ARM_PA_32 cannot be enabled on ARM_64 as the memory management unit works on 48bit physical addresses. On ARM_32, it can be used on systems where large page address extension is not supported. Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> --- Changes from - v1 - 1. No changes. xen/arch/arm/Kconfig | 9 +++++++++ xen/arch/arm/include/asm/page-bits.h | 2 ++ xen/arch/arm/include/asm/types.h | 7 +++++++ 3 files changed, 18 insertions(+)