| Message ID | 20240918175102.223076-3-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable early bootup of AArch64 MPU systems | expand |
Hi Ayan, On 18/09/2024 19:51, Ayan Kumar Halder wrote: > From: Wei Chen <wei.chen@arm.com> > > On Armv8-A, Xen has a fixed virtual start address (link address too) for all > Armv8-A platforms. In an MMU based system, Xen can map its loaded address to > this virtual start address. So, on Armv8-A platforms, the Xen start address does > not need to be configurable. But on Armv8-R platforms, there is no MMU to map > loaded address to a fixed virtual address and different platforms will have very > different address space layout. So Xen cannot use a fixed physical address on > MPU based system and need to have it configurable. > > So, we introduce a Kconfig option for users to set the start address. The start > address needs to be aligned to 4KB. We have a check for this alignment. It would suggest to add some explanation why you want the start address to be 4KB aligned. > > In case if the user forgets to set the start address, then 0xffffffff is used > as default. This is to trigger the error (on alignment check) and thereby prompt > user to set the start address. > > Also updated config.h so that it includes mpu/layout.h when CONFIG_MPU is > defined. > > Signed-off-by: Wei Chen <wei.chen@arm.com> > Signed-off-by: Jiamei.Xie <jiamei.xie@arm.com> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> > --- > Changes from :- > > v1 - 1. Fixed some of the coding style issues. > 2. Reworded the help message. > 3. Updated the commit message. > > xen/arch/arm/Kconfig | 10 ++++++++++ > xen/arch/arm/include/asm/config.h | 4 +++- > xen/arch/arm/include/asm/mpu/layout.h | 27 +++++++++++++++++++++++++++ Looking at this patch again, I don't see any modification in xen.lds.S. Is it intended? > 3 files changed, 40 insertions(+), 1 deletion(-) > create mode 100644 xen/arch/arm/include/asm/mpu/layout.h > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig > index e881f5ba57..ab3ef005a6 100644 > --- a/xen/arch/arm/Kconfig > +++ b/xen/arch/arm/Kconfig > @@ -23,6 +23,16 @@ config ARCH_DEFCONFIG > default "arch/arm/configs/arm32_defconfig" if ARM_32 > default "arch/arm/configs/arm64_defconfig" if ARM_64 > > +config XEN_START_ADDRESS > + hex "Xen start address: keep default to use platform defined address" > + default 0xFFFFFFFF > + depends on MPU > + help > + Used to set customized address at which which Xen will be linked on MPU > + systems. Must be aligned to 4KB. > + 0xFFFFFFFF is used as default value to indicate that user has not > + customized this address. > + > menu "Architecture Features" > > choice > diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h > index a2e22b659d..0a51142efd 100644 > --- a/xen/arch/arm/include/asm/config.h > +++ b/xen/arch/arm/include/asm/config.h > @@ -69,8 +69,10 @@ > #include <xen/const.h> > #include <xen/page-size.h> > > -#ifdef CONFIG_MMU > +#if defined(CONFIG_MMU) > #include <asm/mmu/layout.h> > +#elif defined(CONFIG_MPU) > +#include <asm/mpu/layout.h> > #else > # error "Unknown memory management layout" > #endif > diff --git a/xen/arch/arm/include/asm/mpu/layout.h b/xen/arch/arm/include/asm/mpu/layout.h > new file mode 100644 > index 0000000000..f9a5be2d6b > --- /dev/null > +++ b/xen/arch/arm/include/asm/mpu/layout.h > @@ -0,0 +1,27 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > + > +#ifndef __ARM_MPU_LAYOUT_H__ > +#define __ARM_MPU_LAYOUT_H__ > + > +#define XEN_START_ADDRESS CONFIG_XEN_START_ADDRESS > + > +/* > + * All MPU platforms need to provide a XEN_START_ADDRESS for linker. This > + * address indicates where Xen image will be loaded and run from. This > + * address must be aligned to a PAGE_SIZE. Strictly speaking, this doesn't match the Kconfig. AFAIU, we still said the internal code may continue to rely on PAGE_SIZE for the time being. But I think we would want to have a BUILD_BUG_ON(PAGE_SIZE != SZ_4K) to catch any value change. > + */ > +#if (XEN_START_ADDRESS % PAGE_SIZE) != 0 > +#error "XEN_START_ADDRESS must be aligned to PAGE_SIZE" In the error message, you want to mention 4KB. Cheers,
On 19/09/2024 13:42, Julien Grall wrote: > Hi Ayan, Hi Julien, > > On 18/09/2024 19:51, Ayan Kumar Halder wrote: >> From: Wei Chen <wei.chen@arm.com> >> >> On Armv8-A, Xen has a fixed virtual start address (link address too) >> for all >> Armv8-A platforms. In an MMU based system, Xen can map its loaded >> address to >> this virtual start address. So, on Armv8-A platforms, the Xen start >> address does >> not need to be configurable. But on Armv8-R platforms, there is no >> MMU to map >> loaded address to a fixed virtual address and different platforms >> will have very >> different address space layout. So Xen cannot use a fixed physical >> address on >> MPU based system and need to have it configurable. >> >> So, we introduce a Kconfig option for users to set the start address. >> The start >> address needs to be aligned to 4KB. We have a check for this alignment. > > It would suggest to add some explanation why you want the start > address to be 4KB aligned. Let me know if this makes sense. "MPU allows us to define regions which are 64 bits aligned. This restriction comes from the bitfields of PRBAR, PRLAR (the lower 6 bits are 0 extended to provide the base and limit address of a region). This means that the start address of Xen needs to be at least 64 bits aligned (as it will correspond to the start address of memory protection region). As for now Xen on MPU tries to use the same memory alignment restrictions as it has been for MMU. Unlike MMU where the starting virtual address is 2MB, Xen on MPU needs the start address to be 4 KB (ie page size) aligned." Thinking about this a bit more (and based on the discussion in "Re: [PATCH v1 2/4] xen/arm: mpu: Define Xen start address for MPU systems"), I think we can keep the 4 KB restriction for now and relax it later. Let me know what you think. > >> >> In case if the user forgets to set the start address, then 0xffffffff >> is used >> as default. This is to trigger the error (on alignment check) and >> thereby prompt >> user to set the start address. >> >> Also updated config.h so that it includes mpu/layout.h when >> CONFIG_MPU is >> defined. >> >> Signed-off-by: Wei Chen <wei.chen@arm.com> >> Signed-off-by: Jiamei.Xie <jiamei.xie@arm.com> >> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >> --- >> Changes from :- >> >> v1 - 1. Fixed some of the coding style issues. >> 2. Reworded the help message. >> 3. Updated the commit message. >> >> xen/arch/arm/Kconfig | 10 ++++++++++ >> xen/arch/arm/include/asm/config.h | 4 +++- >> xen/arch/arm/include/asm/mpu/layout.h | 27 +++++++++++++++++++++++++++ > > Looking at this patch again, I don't see any modification in > xen.lds.S. Is it intended? If we agree with the justification provided before, then this should be the modification. --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -232,6 +232,12 @@ SECTIONS */ ASSERT(_start == XEN_VIRT_START, "_start != XEN_VIRT_START") +/* + * On MPU based platforms, the starting address is to be provided by user. + * One need to check that it is 4KB aligned. + */ +ASSERT(IS_ALIGNED(_start, 4096), "starting address is misaligned") + > >> 3 files changed, 40 insertions(+), 1 deletion(-) >> create mode 100644 xen/arch/arm/include/asm/mpu/layout.h >> >> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig >> index e881f5ba57..ab3ef005a6 100644 >> --- a/xen/arch/arm/Kconfig >> +++ b/xen/arch/arm/Kconfig >> @@ -23,6 +23,16 @@ config ARCH_DEFCONFIG >> default "arch/arm/configs/arm32_defconfig" if ARM_32 >> default "arch/arm/configs/arm64_defconfig" if ARM_64 >> +config XEN_START_ADDRESS >> + hex "Xen start address: keep default to use platform defined >> address" >> + default 0xFFFFFFFF >> + depends on MPU >> + help >> + Used to set customized address at which which Xen will be >> linked on MPU >> + systems. Must be aligned to 4KB. >> + 0xFFFFFFFF is used as default value to indicate that user has not >> + customized this address. >> + >> menu "Architecture Features" >> choice >> diff --git a/xen/arch/arm/include/asm/config.h >> b/xen/arch/arm/include/asm/config.h >> index a2e22b659d..0a51142efd 100644 >> --- a/xen/arch/arm/include/asm/config.h >> +++ b/xen/arch/arm/include/asm/config.h >> @@ -69,8 +69,10 @@ >> #include <xen/const.h> >> #include <xen/page-size.h> >> -#ifdef CONFIG_MMU >> +#if defined(CONFIG_MMU) >> #include <asm/mmu/layout.h> >> +#elif defined(CONFIG_MPU) >> +#include <asm/mpu/layout.h> >> #else >> # error "Unknown memory management layout" >> #endif >> diff --git a/xen/arch/arm/include/asm/mpu/layout.h >> b/xen/arch/arm/include/asm/mpu/layout.h >> new file mode 100644 >> index 0000000000..f9a5be2d6b >> --- /dev/null >> +++ b/xen/arch/arm/include/asm/mpu/layout.h >> @@ -0,0 +1,27 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> + >> +#ifndef __ARM_MPU_LAYOUT_H__ >> +#define __ARM_MPU_LAYOUT_H__ >> + >> +#define XEN_START_ADDRESS CONFIG_XEN_START_ADDRESS >> + >> +/* >> + * All MPU platforms need to provide a XEN_START_ADDRESS for linker. >> This >> + * address indicates where Xen image will be loaded and run from. This >> + * address must be aligned to a PAGE_SIZE. > > Strictly speaking, this doesn't match the Kconfig. AFAIU, we still > said the internal code may continue to rely on PAGE_SIZE for the time > being. But I think we would want to have a BUILD_BUG_ON(PAGE_SIZE != > SZ_4K) to catch any value change. Yes, I can add this. > >> + */ >> +#if (XEN_START_ADDRESS % PAGE_SIZE) != 0 >> +#error "XEN_START_ADDRESS must be aligned to PAGE_SIZE" > > In the error message, you want to mention 4KB. Ack. - Ayan > > Cheers, >
On 23/09/2024 12:22, Ayan Kumar Halder wrote: > > On 19/09/2024 13:42, Julien Grall wrote: >> Hi Ayan, > Hi Julien, >> >> On 18/09/2024 19:51, Ayan Kumar Halder wrote: >>> From: Wei Chen <wei.chen@arm.com> >>> >>> On Armv8-A, Xen has a fixed virtual start address (link address too) >>> for all >>> Armv8-A platforms. In an MMU based system, Xen can map its loaded >>> address to >>> this virtual start address. So, on Armv8-A platforms, the Xen start >>> address does >>> not need to be configurable. But on Armv8-R platforms, there is no >>> MMU to map >>> loaded address to a fixed virtual address and different platforms >>> will have very >>> different address space layout. So Xen cannot use a fixed physical >>> address on >>> MPU based system and need to have it configurable. >>> >>> So, we introduce a Kconfig option for users to set the start address. >>> The start >>> address needs to be aligned to 4KB. We have a check for this alignment. >> >> It would suggest to add some explanation why you want the start >> address to be 4KB aligned. > > Let me know if this makes sense. > > "MPU allows us to define regions which are 64 bits aligned. This > restriction comes from the bitfields of PRBAR, PRLAR (the lower 6 bits > are 0 extended to provide the base and limit address of a region). This > means that the start address of Xen needs to be at least 64 bits aligned > (as it will correspond to the start address of memory protection region). > > As for now Xen on MPU tries to use the same memory alignment > restrictions as it has been for MMU. Unlike MMU where the starting > virtual address is 2MB, Xen on MPU needs the start address to be 4 KB > (ie page size) aligned." > > Thinking about this a bit more (and based on the discussion in "Re: > [PATCH v1 2/4] xen/arm: mpu: Define Xen start address for MPU systems"), > I think we can keep the 4 KB restriction for now and relax it later. Let > me know what you think. I am fine with that. > >> >>> >>> In case if the user forgets to set the start address, then 0xffffffff >>> is used >>> as default. This is to trigger the error (on alignment check) and >>> thereby prompt >>> user to set the start address. >>> >>> Also updated config.h so that it includes mpu/layout.h when >>> CONFIG_MPU is >>> defined. >>> >>> Signed-off-by: Wei Chen <wei.chen@arm.com> >>> Signed-off-by: Jiamei.Xie <jiamei.xie@arm.com> >>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >>> --- >>> Changes from :- >>> >>> v1 - 1. Fixed some of the coding style issues. >>> 2. Reworded the help message. >>> 3. Updated the commit message. >>> >>> xen/arch/arm/Kconfig | 10 ++++++++++ >>> xen/arch/arm/include/asm/config.h | 4 +++- >>> xen/arch/arm/include/asm/mpu/layout.h | 27 +++++++++++++++++++++++++++ >> >> Looking at this patch again, I don't see any modification in >> xen.lds.S. Is it intended? > > If we agree with the justification provided before, then this should be > the modification. > > --- a/xen/arch/arm/xen.lds.S > +++ b/xen/arch/arm/xen.lds.S > @@ -232,6 +232,12 @@ SECTIONS > */ > ASSERT(_start == XEN_VIRT_START, "_start != XEN_VIRT_START") > > +/* > + * On MPU based platforms, the starting address is to be provided by user. > + * One need to check that it is 4KB aligned. > + */ > +ASSERT(IS_ALIGNED(_start, 4096), "starting address is misaligned") > + Ah I wasn't asking to add an ASSERT (although it would be good so long it is protected by CONFIG_MPU). Instead I was expecting XEN_VIRT_START to be replaced. But looking at the code again, I see you define XEN_VIRT_START for the MPU. I think this is a little bit odd to use XEN_VIRT_* defined for the MPU but it would be ok. Is the expectation that all the MPU code should be using XEN_START_ADDRESS? If so, maybe it would be worth add a comment on top of XEN_VIRT_ADDRESS. Cheers,
Hi Julien, On 24/09/2024 17:44, Julien Grall wrote: > > > On 23/09/2024 12:22, Ayan Kumar Halder wrote: >> >> On 19/09/2024 13:42, Julien Grall wrote: >>> Hi Ayan, >> Hi Julien, >>> >>> On 18/09/2024 19:51, Ayan Kumar Halder wrote: >>>> From: Wei Chen <wei.chen@arm.com> >>>> >>>> On Armv8-A, Xen has a fixed virtual start address (link address >>>> too) for all >>>> Armv8-A platforms. In an MMU based system, Xen can map its loaded >>>> address to >>>> this virtual start address. So, on Armv8-A platforms, the Xen start >>>> address does >>>> not need to be configurable. But on Armv8-R platforms, there is no >>>> MMU to map >>>> loaded address to a fixed virtual address and different platforms >>>> will have very >>>> different address space layout. So Xen cannot use a fixed physical >>>> address on >>>> MPU based system and need to have it configurable. >>>> >>>> So, we introduce a Kconfig option for users to set the start >>>> address. The start >>>> address needs to be aligned to 4KB. We have a check for this >>>> alignment. >>> >>> It would suggest to add some explanation why you want the start >>> address to be 4KB aligned. >> >> Let me know if this makes sense. >> >> "MPU allows us to define regions which are 64 bits aligned. This >> restriction comes from the bitfields of PRBAR, PRLAR (the lower 6 >> bits are 0 extended to provide the base and limit address of a >> region). This means that the start address of Xen needs to be at >> least 64 bits aligned (as it will correspond to the start address of >> memory protection region). >> >> As for now Xen on MPU tries to use the same memory alignment >> restrictions as it has been for MMU. Unlike MMU where the starting >> virtual address is 2MB, Xen on MPU needs the start address to be 4 KB >> (ie page size) aligned." >> >> Thinking about this a bit more (and based on the discussion in "Re: >> [PATCH v1 2/4] xen/arm: mpu: Define Xen start address for MPU >> systems"), I think we can keep the 4 KB restriction for now and relax >> it later. Let me know what you think. > > I am fine with that. > >> >>> >>>> >>>> In case if the user forgets to set the start address, then >>>> 0xffffffff is used >>>> as default. This is to trigger the error (on alignment check) and >>>> thereby prompt >>>> user to set the start address. >>>> >>>> Also updated config.h so that it includes mpu/layout.h when >>>> CONFIG_MPU is >>>> defined. >>>> >>>> Signed-off-by: Wei Chen <wei.chen@arm.com> >>>> Signed-off-by: Jiamei.Xie <jiamei.xie@arm.com> >>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >>>> --- >>>> Changes from :- >>>> >>>> v1 - 1. Fixed some of the coding style issues. >>>> 2. Reworded the help message. >>>> 3. Updated the commit message. >>>> >>>> xen/arch/arm/Kconfig | 10 ++++++++++ >>>> xen/arch/arm/include/asm/config.h | 4 +++- >>>> xen/arch/arm/include/asm/mpu/layout.h | 27 >>>> +++++++++++++++++++++++++++ >>> >>> Looking at this patch again, I don't see any modification in >>> xen.lds.S. Is it intended? >> >> If we agree with the justification provided before, then this should >> be the modification. >> >> --- a/xen/arch/arm/xen.lds.S >> +++ b/xen/arch/arm/xen.lds.S >> @@ -232,6 +232,12 @@ SECTIONS >> */ >> ASSERT(_start == XEN_VIRT_START, "_start != XEN_VIRT_START") >> >> +/* >> + * On MPU based platforms, the starting address is to be provided by >> user. >> + * One need to check that it is 4KB aligned. >> + */ >> +ASSERT(IS_ALIGNED(_start, 4096), "starting address is >> misaligned") >> + > > Ah I wasn't asking to add an ASSERT (although it would be good so long > it is protected by CONFIG_MPU). Instead I was expecting XEN_VIRT_START > to be replaced. But looking at the code again, I see you define > XEN_VIRT_START for the MPU. > > I think this is a little bit odd to use XEN_VIRT_* defined for the MPU > but it would be ok. Is the expectation that all the MPU code should be > using XEN_START_ADDRESS? If so, maybe it would be worth add a comment > on top of XEN_VIRT_ADDRESS. I have defined XEN_VIRT_START as XEN_START_ADDRESS , so that the code continues to use XEN_VIRT_START (as it does for the MMU) and we avoid introducing #if-defs. Does this comment make sense ? /* * For MPU, XEN's virtual start address is same as the physical address. * The reason being MPU treats VA == PA. IOW, it cannot map the physical * address to a different fixed virtual address. So, the virtual start * address is determined by the physical address at which Xen is loaded. */ #define XEN_VIRT_START _AT(paddr_t, XEN_START_ADDRESS) - Ayan > > Cheers, >
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index e881f5ba57..ab3ef005a6 100644 --- a/xen/arch/arm/Kconfig +++ b/xen/arch/arm/Kconfig @@ -23,6 +23,16 @@ config ARCH_DEFCONFIG default "arch/arm/configs/arm32_defconfig" if ARM_32 default "arch/arm/configs/arm64_defconfig" if ARM_64 +config XEN_START_ADDRESS + hex "Xen start address: keep default to use platform defined address" + default 0xFFFFFFFF + depends on MPU + help + Used to set customized address at which which Xen will be linked on MPU + systems. Must be aligned to 4KB. + 0xFFFFFFFF is used as default value to indicate that user has not + customized this address. + menu "Architecture Features" choice diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h index a2e22b659d..0a51142efd 100644 --- a/xen/arch/arm/include/asm/config.h +++ b/xen/arch/arm/include/asm/config.h @@ -69,8 +69,10 @@ #include <xen/const.h> #include <xen/page-size.h> -#ifdef CONFIG_MMU +#if defined(CONFIG_MMU) #include <asm/mmu/layout.h> +#elif defined(CONFIG_MPU) +#include <asm/mpu/layout.h> #else # error "Unknown memory management layout" #endif diff --git a/xen/arch/arm/include/asm/mpu/layout.h b/xen/arch/arm/include/asm/mpu/layout.h new file mode 100644 index 0000000000..f9a5be2d6b --- /dev/null +++ b/xen/arch/arm/include/asm/mpu/layout.h @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef __ARM_MPU_LAYOUT_H__ +#define __ARM_MPU_LAYOUT_H__ + +#define XEN_START_ADDRESS CONFIG_XEN_START_ADDRESS + +/* + * All MPU platforms need to provide a XEN_START_ADDRESS for linker. This + * address indicates where Xen image will be loaded and run from. This + * address must be aligned to a PAGE_SIZE. + */ +#if (XEN_START_ADDRESS % PAGE_SIZE) != 0 +#error "XEN_START_ADDRESS must be aligned to PAGE_SIZE" +#endif + +#define XEN_VIRT_START _AT(paddr_t, XEN_START_ADDRESS) + +#endif /* __ARM_MPU_LAYOUT_H__ */ +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */