| Message ID | 20230113052914.3845596-21-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Add Armv8-R64 MPU support to Xen - Part#1 | expand |
Hi, On 13/01/2023 05:28, Penny Zheng wrote: > In MPU system, device tree binary can be packed with Xen > image through CONFIG_DTB_FILE, or provided by bootloader through x0. > > In MPU system, each section in xen.lds.S is PAGE_SIZE aligned. > So in order to not overlap with the previous BSS section, dtb section > should be made page-aligned too. > We add . = ALIGN(PAGE_SIZE); in the head of dtb section to make it happen. > > In this commit, we map early FDT with a transient MPU memory region at > rear with REGION_HYPERVISOR_BOOT. > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > Signed-off-by: Wei Chen <wei.chen@arm.com> > --- > xen/arch/arm/include/asm/arm64/mpu.h | 5 +++ > xen/arch/arm/mm_mpu.c | 63 +++++++++++++++++++++++++--- > xen/arch/arm/xen.lds.S | 5 ++- > 3 files changed, 67 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/include/asm/arm64/mpu.h b/xen/arch/arm/include/asm/arm64/mpu.h > index fcde6ad0db..b85e420a90 100644 > --- a/xen/arch/arm/include/asm/arm64/mpu.h > +++ b/xen/arch/arm/include/asm/arm64/mpu.h > @@ -45,18 +45,22 @@ > * [3:4] Execute Never > * [5:6] Access Permission > * [7] Region Present > + * [8] Boot-only Region > */ > #define _REGION_AI_BIT 0 > #define _REGION_XN_BIT 3 > #define _REGION_AP_BIT 5 > #define _REGION_PRESENT_BIT 7 > +#define _REGION_BOOTONLY_BIT 8 In a follow-up patch, you are replacing BOOTONLY with TRANSIENT. Please avoid renaming new functions and instead introduce them with the correct name from the start. > #define _REGION_XN (2U << _REGION_XN_BIT) > #define _REGION_RO (2U << _REGION_AP_BIT) > #define _REGION_PRESENT (1U << _REGION_PRESENT_BIT) > +#define _REGION_BOOTONLY (1U << _REGION_BOOTONLY_BIT) > #define REGION_AI_MASK(x) (((x) >> _REGION_AI_BIT) & 0x7U) > #define REGION_XN_MASK(x) (((x) >> _REGION_XN_BIT) & 0x3U) > #define REGION_AP_MASK(x) (((x) >> _REGION_AP_BIT) & 0x3U) > #define REGION_RO_MASK(x) (((x) >> _REGION_AP_BIT) & 0x2U) > +#define REGION_BOOTONLY_MASK(x) (((x) >> _REGION_BOOTONLY_BIT) & 0x1U) > > /* > * _REGION_NORMAL is convenience define. It is not meant to be used > @@ -68,6 +72,7 @@ > #define REGION_HYPERVISOR_RO (_REGION_NORMAL|_REGION_XN|_REGION_RO) > > #define REGION_HYPERVISOR REGION_HYPERVISOR_RW > +#define REGION_HYPERVISOR_BOOT (REGION_HYPERVISOR_RW|_REGION_BOOTONLY) > > #define INVALID_REGION (~0UL) > > diff --git a/xen/arch/arm/mm_mpu.c b/xen/arch/arm/mm_mpu.c > index 08720a7c19..b34dbf4515 100644 > --- a/xen/arch/arm/mm_mpu.c > +++ b/xen/arch/arm/mm_mpu.c > @@ -20,11 +20,16 @@ > */ > > #include <xen/init.h> > +#include <xen/libfdt/libfdt.h> > #include <xen/mm.h> > #include <xen/page-size.h> > +#include <xen/pfn.h> > +#include <xen/sizes.h> > #include <xen/spinlock.h> > #include <asm/arm64/mpu.h> > +#include <asm/early_printk.h> > #include <asm/page.h> > +#include <asm/setup.h> > > #ifdef NDEBUG > static inline void > @@ -62,6 +67,8 @@ uint64_t __ro_after_init max_xen_mpumap; > > static DEFINE_SPINLOCK(xen_mpumap_lock); > > +static paddr_t dtb_paddr; > + > /* Write a MPU protection region */ > #define WRITE_PROTECTION_REGION(sel, pr, prbar_el2, prlar_el2) ({ \ > uint64_t _sel = sel; \ > @@ -403,7 +410,16 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit, > > /* During boot time, the default index is next_fixed_region_idx. */ > if ( system_state <= SYS_STATE_active ) > - idx = next_fixed_region_idx; > + { > + /* > + * If it is a boot-only region (i.e. region for early FDT), > + * it shall be added from the tail for late init re-organizing > + */ > + if ( REGION_BOOTONLY_MASK(flags) ) > + idx = next_transient_region_idx; > + else > + idx = next_fixed_region_idx; > + } > > xen_mpumap[idx] = pr_of_xenaddr(base, limit - 1, REGION_AI_MASK(flags)); > /* Set permission */ > @@ -465,14 +481,51 @@ int map_pages_to_xen(unsigned long virt, > mfn_to_maddr(mfn_add(mfn, nr_mfns)), flags); > } > > -/* TODO: Implementation on the first usage */ > -void dump_hyp_walk(vaddr_t addr) > +void * __init early_fdt_map(paddr_t fdt_paddr) A fair amount of this code is the same as the MMU version. Can we share some code? > { > + void *fdt_virt; > + uint32_t size; > + > + /* > + * Check whether the physical FDT address is set and meets the minimum > + * alignment requirement. Since we are relying on MIN_FDT_ALIGN to be at > + * least 8 bytes so that we always access the magic and size fields > + * of the FDT header after mapping the first chunk, double check if > + * that is indeed the case. > + */ > + BUILD_BUG_ON(MIN_FDT_ALIGN < 8); > + if ( !fdt_paddr || fdt_paddr % MIN_FDT_ALIGN ) > + return NULL; > + > + dtb_paddr = fdt_paddr; > + /* > + * In MPU system, device tree binary can be packed with Xen image > + * through CONFIG_DTB_FILE, or provided by bootloader through x0. > + * Map FDT with a transient MPU memory region of MAX_FDT_SIZE. > + * After that, we can do some magic check. > + */ > + if ( map_pages_to_xen(round_pgdown(fdt_paddr), > + maddr_to_mfn(round_pgdown(fdt_paddr)), > + round_pgup(MAX_FDT_SIZE) >> PAGE_SHIFT, > + REGION_HYPERVISOR_BOOT) ) > + panic("Unable to map the device-tree.\n"); > + > + /* VA == PA */ > + fdt_virt = maddr_to_virt(fdt_paddr); > + > + if ( fdt_magic(fdt_virt) != FDT_MAGIC ) > + return NULL; > + > + size = fdt_totalsize(fdt_virt); > + if ( size > MAX_FDT_SIZE ) > + return NULL; > + > + return fdt_virt; > } > > -void * __init early_fdt_map(paddr_t fdt_paddr) Same as the previous patch, why do you implmenet early_fdt_map() at a different place in the file? > +/* TODO: Implementation on the first usage */ > +void dump_hyp_walk(vaddr_t addr) > { > - return NULL; > } > > void __init remove_early_mappings(void) > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S > index 79965a3c17..0565e22a1f 100644 > --- a/xen/arch/arm/xen.lds.S > +++ b/xen/arch/arm/xen.lds.S > @@ -218,7 +218,10 @@ SECTIONS > _end = . ; > > /* Section for the device tree blob (if any). */ > - .dtb : { *(.dtb) } :text > + .dtb : { > + . = ALIGN(PAGE_SIZE); > + *(.dtb) > + } :text > > DWARF2_DEBUG_SECTIONS > Cheers,
Hi, A few more remarks. On 13/01/2023 05:28, Penny Zheng wrote: > In MPU system, device tree binary can be packed with Xen > image through CONFIG_DTB_FILE, or provided by bootloader through x0. > > In MPU system, each section in xen.lds.S is PAGE_SIZE aligned. > So in order to not overlap with the previous BSS section, dtb section > should be made page-aligned too. > We add . = ALIGN(PAGE_SIZE); in the head of dtb section to make it happen. > > In this commit, we map early FDT with a transient MPU memory region at > rear with REGION_HYPERVISOR_BOOT. > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > Signed-off-by: Wei Chen <wei.chen@arm.com> > --- > xen/arch/arm/include/asm/arm64/mpu.h | 5 +++ > xen/arch/arm/mm_mpu.c | 63 +++++++++++++++++++++++++--- > xen/arch/arm/xen.lds.S | 5 ++- > 3 files changed, 67 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/include/asm/arm64/mpu.h b/xen/arch/arm/include/asm/arm64/mpu.h > index fcde6ad0db..b85e420a90 100644 > --- a/xen/arch/arm/include/asm/arm64/mpu.h > +++ b/xen/arch/arm/include/asm/arm64/mpu.h > @@ -45,18 +45,22 @@ > * [3:4] Execute Never > * [5:6] Access Permission > * [7] Region Present > + * [8] Boot-only Region > */ > #define _REGION_AI_BIT 0 > #define _REGION_XN_BIT 3 > #define _REGION_AP_BIT 5 > #define _REGION_PRESENT_BIT 7 > +#define _REGION_BOOTONLY_BIT 8 > #define _REGION_XN (2U << _REGION_XN_BIT) > #define _REGION_RO (2U << _REGION_AP_BIT) > #define _REGION_PRESENT (1U << _REGION_PRESENT_BIT) > +#define _REGION_BOOTONLY (1U << _REGION_BOOTONLY_BIT) > #define REGION_AI_MASK(x) (((x) >> _REGION_AI_BIT) & 0x7U) > #define REGION_XN_MASK(x) (((x) >> _REGION_XN_BIT) & 0x3U) > #define REGION_AP_MASK(x) (((x) >> _REGION_AP_BIT) & 0x3U) > #define REGION_RO_MASK(x) (((x) >> _REGION_AP_BIT) & 0x2U) > +#define REGION_BOOTONLY_MASK(x) (((x) >> _REGION_BOOTONLY_BIT) & 0x1U) > > /* > * _REGION_NORMAL is convenience define. It is not meant to be used > @@ -68,6 +72,7 @@ > #define REGION_HYPERVISOR_RO (_REGION_NORMAL|_REGION_XN|_REGION_RO) > > #define REGION_HYPERVISOR REGION_HYPERVISOR_RW > +#define REGION_HYPERVISOR_BOOT (REGION_HYPERVISOR_RW|_REGION_BOOTONLY) > > #define INVALID_REGION (~0UL) > > diff --git a/xen/arch/arm/mm_mpu.c b/xen/arch/arm/mm_mpu.c > index 08720a7c19..b34dbf4515 100644 > --- a/xen/arch/arm/mm_mpu.c > +++ b/xen/arch/arm/mm_mpu.c > @@ -20,11 +20,16 @@ > */ > > #include <xen/init.h> > +#include <xen/libfdt/libfdt.h> > #include <xen/mm.h> > #include <xen/page-size.h> > +#include <xen/pfn.h> > +#include <xen/sizes.h> > #include <xen/spinlock.h> > #include <asm/arm64/mpu.h> > +#include <asm/early_printk.h> > #include <asm/page.h> > +#include <asm/setup.h> > > #ifdef NDEBUG > static inline void > @@ -62,6 +67,8 @@ uint64_t __ro_after_init max_xen_mpumap; > > static DEFINE_SPINLOCK(xen_mpumap_lock); > > +static paddr_t dtb_paddr; > + > /* Write a MPU protection region */ > #define WRITE_PROTECTION_REGION(sel, pr, prbar_el2, prlar_el2) ({ \ > uint64_t _sel = sel; \ > @@ -403,7 +410,16 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit, > > /* During boot time, the default index is next_fixed_region_idx. */ > if ( system_state <= SYS_STATE_active ) > - idx = next_fixed_region_idx; > + { > + /* > + * If it is a boot-only region (i.e. region for early FDT), > + * it shall be added from the tail for late init re-organizing > + */ > + if ( REGION_BOOTONLY_MASK(flags) ) > + idx = next_transient_region_idx; > + else > + idx = next_fixed_region_idx; > + } > > xen_mpumap[idx] = pr_of_xenaddr(base, limit - 1, REGION_AI_MASK(flags)); > /* Set permission */ > @@ -465,14 +481,51 @@ int map_pages_to_xen(unsigned long virt, > mfn_to_maddr(mfn_add(mfn, nr_mfns)), flags); > } > > -/* TODO: Implementation on the first usage */ > -void dump_hyp_walk(vaddr_t addr) > +void * __init early_fdt_map(paddr_t fdt_paddr) > { > + void *fdt_virt; > + uint32_t size; > + > + /* > + * Check whether the physical FDT address is set and meets the minimum > + * alignment requirement. Since we are relying on MIN_FDT_ALIGN to be at > + * least 8 bytes so that we always access the magic and size fields > + * of the FDT header after mapping the first chunk, double check if > + * that is indeed the case. > + */ > + BUILD_BUG_ON(MIN_FDT_ALIGN < 8); > + if ( !fdt_paddr || fdt_paddr % MIN_FDT_ALIGN ) > + return NULL; > + > + dtb_paddr = fdt_paddr; > + /* > + * In MPU system, device tree binary can be packed with Xen image > + * through CONFIG_DTB_FILE, or provided by bootloader through x0. The behavior you describe is not specific to the MPU system. I also don't quite understand how describing the method to pass the DT actually matters here. > + * Map FDT with a transient MPU memory region of MAX_FDT_SIZE. > + * After that, we can do some magic check. > + */ > + if ( map_pages_to_xen(round_pgdown(fdt_paddr), I haven't looked at the rest of the series. But from here, it seems a bit strange to use map_pages_to_xen() because the virt and the phys should be the same. Do you plan to share some code where map_pages_to_xen() will be used? > + maddr_to_mfn(round_pgdown(fdt_paddr)), > + round_pgup(MAX_FDT_SIZE) >> PAGE_SHIFT, This will not work properly is the Device-Tree is MAX_FDT_SIZE (could already be page-aligned) but the start address is not page-aligned. But I think trying to map the maximum size from the start could potentially result to some issue. Below the excerpt from the Image documentation: "The device tree blob (dtb) must be placed on an 8-byte boundary and must not exceed 2 megabytes in size. Since the dtb will be mapped cacheable using blocks of up to 2 megabytes in size, it must not be placed within any 2M region which must be mapped with any specific attributes." So it would be better to map the first 2MB. Check the size and then re-map with an extra 2MB if needed. > + REGION_HYPERVISOR_BOOT) ) > + panic("Unable to map the device-tree.\n"); > + > + /* VA == PA */ I have seen in a few places where you add a similar comment. But I am not sure to understand how this help to describe the implementation of maddr_to_virt(). > + fdt_virt = maddr_to_virt(fdt_paddr); > + > + if ( fdt_magic(fdt_virt) != FDT_MAGIC ) > + return NULL; > + > + size = fdt_totalsize(fdt_virt); > + if ( size > MAX_FDT_SIZE ) > + return NULL; > + > + return fdt_virt; > } > > -void * __init early_fdt_map(paddr_t fdt_paddr) > +/* TODO: Implementation on the first usage */ > +void dump_hyp_walk(vaddr_t addr) > { > - return NULL; > } > > void __init remove_early_mappings(void) > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S > index 79965a3c17..0565e22a1f 100644 > --- a/xen/arch/arm/xen.lds.S > +++ b/xen/arch/arm/xen.lds.S > @@ -218,7 +218,10 @@ SECTIONS > _end = . ; > > /* Section for the device tree blob (if any). */ > - .dtb : { *(.dtb) } :text > + .dtb : { > + . = ALIGN(PAGE_SIZE); > + *(.dtb) > + } :text > > DWARF2_DEBUG_SECTIONS > Cheers,
Hi Julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Monday, February 6, 2023 6:11 PM > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org > Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini > <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; > Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > Subject: Re: [PATCH v2 20/40] xen/mpu: plump early_fdt_map in MPU > systems > > Hi, > > A few more remarks. > > On 13/01/2023 05:28, Penny Zheng wrote: > > In MPU system, device tree binary can be packed with Xen image through > > CONFIG_DTB_FILE, or provided by bootloader through x0. > > > > In MPU system, each section in xen.lds.S is PAGE_SIZE aligned. > > So in order to not overlap with the previous BSS section, dtb section > > should be made page-aligned too. > > We add . = ALIGN(PAGE_SIZE); in the head of dtb section to make it happen. > > > > In this commit, we map early FDT with a transient MPU memory region at > > rear with REGION_HYPERVISOR_BOOT. > > > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > > Signed-off-by: Wei Chen <wei.chen@arm.com> > > --- > > xen/arch/arm/include/asm/arm64/mpu.h | 5 +++ > > xen/arch/arm/mm_mpu.c | 63 +++++++++++++++++++++++++--- > > xen/arch/arm/xen.lds.S | 5 ++- > > 3 files changed, 67 insertions(+), 6 deletions(-) > > > > diff --git a/xen/arch/arm/include/asm/arm64/mpu.h > > b/xen/arch/arm/include/asm/arm64/mpu.h > > index fcde6ad0db..b85e420a90 100644 > > --- a/xen/arch/arm/include/asm/arm64/mpu.h > > +++ b/xen/arch/arm/include/asm/arm64/mpu.h > > @@ -45,18 +45,22 @@ > > * [3:4] Execute Never > > * [5:6] Access Permission > > * [7] Region Present > > + * [8] Boot-only Region > > */ > > #define _REGION_AI_BIT 0 > > #define _REGION_XN_BIT 3 > > #define _REGION_AP_BIT 5 > > #define _REGION_PRESENT_BIT 7 > > +#define _REGION_BOOTONLY_BIT 8 > > #define _REGION_XN (2U << _REGION_XN_BIT) > > #define _REGION_RO (2U << _REGION_AP_BIT) > > #define _REGION_PRESENT (1U << _REGION_PRESENT_BIT) > > +#define _REGION_BOOTONLY (1U << _REGION_BOOTONLY_BIT) > > #define REGION_AI_MASK(x) (((x) >> _REGION_AI_BIT) & 0x7U) > > #define REGION_XN_MASK(x) (((x) >> _REGION_XN_BIT) & 0x3U) > > #define REGION_AP_MASK(x) (((x) >> _REGION_AP_BIT) & 0x3U) > > #define REGION_RO_MASK(x) (((x) >> _REGION_AP_BIT) & 0x2U) > > +#define REGION_BOOTONLY_MASK(x) (((x) >> _REGION_BOOTONLY_BIT) > & 0x1U) > > > > /* > > * _REGION_NORMAL is convenience define. It is not meant to be used > > @@ -68,6 +72,7 @@ > > #define REGION_HYPERVISOR_RO > (_REGION_NORMAL|_REGION_XN|_REGION_RO) > > > > #define REGION_HYPERVISOR REGION_HYPERVISOR_RW > > +#define REGION_HYPERVISOR_BOOT > (REGION_HYPERVISOR_RW|_REGION_BOOTONLY) > > > > #define INVALID_REGION (~0UL) > > > > diff --git a/xen/arch/arm/mm_mpu.c b/xen/arch/arm/mm_mpu.c index > > 08720a7c19..b34dbf4515 100644 > > --- a/xen/arch/arm/mm_mpu.c > > +++ b/xen/arch/arm/mm_mpu.c > > @@ -20,11 +20,16 @@ > > */ > > > > #include <xen/init.h> > > +#include <xen/libfdt/libfdt.h> > > #include <xen/mm.h> > > #include <xen/page-size.h> > > +#include <xen/pfn.h> > > +#include <xen/sizes.h> > > #include <xen/spinlock.h> > > #include <asm/arm64/mpu.h> > > +#include <asm/early_printk.h> > > #include <asm/page.h> > > +#include <asm/setup.h> > > > > #ifdef NDEBUG > > static inline void > > @@ -62,6 +67,8 @@ uint64_t __ro_after_init max_xen_mpumap; > > > > static DEFINE_SPINLOCK(xen_mpumap_lock); > > > > +static paddr_t dtb_paddr; > > + > > /* Write a MPU protection region */ > > #define WRITE_PROTECTION_REGION(sel, pr, prbar_el2, prlar_el2) ({ \ > > uint64_t _sel = sel; \ > > @@ -403,7 +410,16 @@ static int xen_mpumap_update_entry(paddr_t > base, > > paddr_t limit, > > > > /* During boot time, the default index is next_fixed_region_idx. */ > > if ( system_state <= SYS_STATE_active ) > > - idx = next_fixed_region_idx; > > + { > > + /* > > + * If it is a boot-only region (i.e. region for early FDT), > > + * it shall be added from the tail for late init re-organizing > > + */ > > + if ( REGION_BOOTONLY_MASK(flags) ) > > + idx = next_transient_region_idx; > > + else > > + idx = next_fixed_region_idx; > > + } > > > > xen_mpumap[idx] = pr_of_xenaddr(base, limit - 1, > REGION_AI_MASK(flags)); > > /* Set permission */ > > @@ -465,14 +481,51 @@ int map_pages_to_xen(unsigned long virt, > > mfn_to_maddr(mfn_add(mfn, nr_mfns)), flags); > > } > > > > -/* TODO: Implementation on the first usage */ -void > > dump_hyp_walk(vaddr_t addr) > > +void * __init early_fdt_map(paddr_t fdt_paddr) > > { > > + void *fdt_virt; > > + uint32_t size; > > + > > + /* > > + * Check whether the physical FDT address is set and meets the > minimum > > + * alignment requirement. Since we are relying on MIN_FDT_ALIGN to > be at > > + * least 8 bytes so that we always access the magic and size fields > > + * of the FDT header after mapping the first chunk, double check if > > + * that is indeed the case. > > + */ > > + BUILD_BUG_ON(MIN_FDT_ALIGN < 8); > > + if ( !fdt_paddr || fdt_paddr % MIN_FDT_ALIGN ) > > + return NULL; > > + > > + dtb_paddr = fdt_paddr; > > + /* > > + * In MPU system, device tree binary can be packed with Xen image > > + * through CONFIG_DTB_FILE, or provided by bootloader through x0. > > The behavior you describe is not specific to the MPU system. I also don't > quite understand how describing the method to pass the DT actually matters > here. > > > + * Map FDT with a transient MPU memory region of MAX_FDT_SIZE. > > + * After that, we can do some magic check. > > + */ > > + if ( map_pages_to_xen(round_pgdown(fdt_paddr), > > I haven't looked at the rest of the series. But from here, it seems a bit strange > to use map_pages_to_xen() because the virt and the phys should be the > same. > Hmm, t thought map_pages_to_xen, is to set up memory mapping for access. In MPU, we also need to set up a MPU memory region for the FDT, even without virt-to-phys conversion > Do you plan to share some code where map_pages_to_xen() will be used? > Each time, in C boot-time, we build up a new MPU memory region for stage 1 EL2 memory mapping, we use this map_pages_to_xen to complete. I think it has the same effect as it has in MMU, other than MMU sets up virt-to-phys memory mapping and MPU always sets up identity memory mapping. > > + maddr_to_mfn(round_pgdown(fdt_paddr)), > > + round_pgup(MAX_FDT_SIZE) >> PAGE_SHIFT, > > This will not work properly is the Device-Tree is MAX_FDT_SIZE (could > already be page-aligned) but the start address is not page-aligned. > > But I think trying to map the maximum size from the start could potentially > result to some issue. Below the excerpt from the Image > documentation: > > "The device tree blob (dtb) must be placed on an 8-byte boundary and must > not exceed 2 megabytes in size. Since the dtb will be mapped cacheable using > blocks of up to 2 megabytes in size, it must not be placed within any 2M > region which must be mapped with any specific attributes." > > So it would be better to map the first 2MB. Check the size and then re-map > with an extra 2MB if needed. > Oh, under special circumstances, the current implementation will map exceeding 2MB. Thanks for explanation! I will map as you suggested. > > + REGION_HYPERVISOR_BOOT) ) > + panic("Unable to > map the device-tree.\n"); > > + > > + /* VA == PA */ > > I have seen in a few places where you add a similar comment. But I am not > sure to understand how this help to describe the implementation of > maddr_to_virt(). > > > + fdt_virt = maddr_to_virt(fdt_paddr); > > + > > + if ( fdt_magic(fdt_virt) != FDT_MAGIC ) > > + return NULL; > > + > > + size = fdt_totalsize(fdt_virt); > > + if ( size > MAX_FDT_SIZE ) > > + return NULL; > > + > > + return fdt_virt; > > } > > > > -void * __init early_fdt_map(paddr_t fdt_paddr) > > +/* TODO: Implementation on the first usage */ void > > +dump_hyp_walk(vaddr_t addr) > > { > > - return NULL; > > } > > > > void __init remove_early_mappings(void) diff --git > > a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index > > 79965a3c17..0565e22a1f 100644 > > --- a/xen/arch/arm/xen.lds.S > > +++ b/xen/arch/arm/xen.lds.S > > @@ -218,7 +218,10 @@ SECTIONS > > _end = . ; > > > > /* Section for the device tree blob (if any). */ > > - .dtb : { *(.dtb) } :text > > + .dtb : { > > + . = ALIGN(PAGE_SIZE); > > + *(.dtb) > > + } :text > > > > DWARF2_DEBUG_SECTIONS > > > > Cheers, > > -- > Julien Grall
On 07/02/2023 06:30, Penny Zheng wrote: > Hi Julien Hi Penny, >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Sent: Monday, February 6, 2023 6:11 PM >> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org >> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini >> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; >> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >> Subject: Re: [PATCH v2 20/40] xen/mpu: plump early_fdt_map in MPU >> systems >> >> Hi, >> >> A few more remarks. >> >> On 13/01/2023 05:28, Penny Zheng wrote: >>> In MPU system, device tree binary can be packed with Xen image through >>> CONFIG_DTB_FILE, or provided by bootloader through x0. >>> >>> In MPU system, each section in xen.lds.S is PAGE_SIZE aligned. >>> So in order to not overlap with the previous BSS section, dtb section >>> should be made page-aligned too. >>> We add . = ALIGN(PAGE_SIZE); in the head of dtb section to make it happen. >>> >>> In this commit, we map early FDT with a transient MPU memory region at >>> rear with REGION_HYPERVISOR_BOOT. >>> >>> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >>> Signed-off-by: Wei Chen <wei.chen@arm.com> >>> --- >>> xen/arch/arm/include/asm/arm64/mpu.h | 5 +++ >>> xen/arch/arm/mm_mpu.c | 63 +++++++++++++++++++++++++--- >>> xen/arch/arm/xen.lds.S | 5 ++- >>> 3 files changed, 67 insertions(+), 6 deletions(-) >>> >>> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h >>> b/xen/arch/arm/include/asm/arm64/mpu.h >>> index fcde6ad0db..b85e420a90 100644 >>> --- a/xen/arch/arm/include/asm/arm64/mpu.h >>> +++ b/xen/arch/arm/include/asm/arm64/mpu.h >>> @@ -45,18 +45,22 @@ >>> * [3:4] Execute Never >>> * [5:6] Access Permission >>> * [7] Region Present >>> + * [8] Boot-only Region >>> */ >>> #define _REGION_AI_BIT 0 >>> #define _REGION_XN_BIT 3 >>> #define _REGION_AP_BIT 5 >>> #define _REGION_PRESENT_BIT 7 >>> +#define _REGION_BOOTONLY_BIT 8 >>> #define _REGION_XN (2U << _REGION_XN_BIT) >>> #define _REGION_RO (2U << _REGION_AP_BIT) >>> #define _REGION_PRESENT (1U << _REGION_PRESENT_BIT) >>> +#define _REGION_BOOTONLY (1U << _REGION_BOOTONLY_BIT) >>> #define REGION_AI_MASK(x) (((x) >> _REGION_AI_BIT) & 0x7U) >>> #define REGION_XN_MASK(x) (((x) >> _REGION_XN_BIT) & 0x3U) >>> #define REGION_AP_MASK(x) (((x) >> _REGION_AP_BIT) & 0x3U) >>> #define REGION_RO_MASK(x) (((x) >> _REGION_AP_BIT) & 0x2U) >>> +#define REGION_BOOTONLY_MASK(x) (((x) >> _REGION_BOOTONLY_BIT) >> & 0x1U) >>> >>> /* >>> * _REGION_NORMAL is convenience define. It is not meant to be used >>> @@ -68,6 +72,7 @@ >>> #define REGION_HYPERVISOR_RO >> (_REGION_NORMAL|_REGION_XN|_REGION_RO) >>> >>> #define REGION_HYPERVISOR REGION_HYPERVISOR_RW >>> +#define REGION_HYPERVISOR_BOOT >> (REGION_HYPERVISOR_RW|_REGION_BOOTONLY) >>> >>> #define INVALID_REGION (~0UL) >>> >>> diff --git a/xen/arch/arm/mm_mpu.c b/xen/arch/arm/mm_mpu.c index >>> 08720a7c19..b34dbf4515 100644 >>> --- a/xen/arch/arm/mm_mpu.c >>> +++ b/xen/arch/arm/mm_mpu.c >>> @@ -20,11 +20,16 @@ >>> */ >>> >>> #include <xen/init.h> >>> +#include <xen/libfdt/libfdt.h> >>> #include <xen/mm.h> >>> #include <xen/page-size.h> >>> +#include <xen/pfn.h> >>> +#include <xen/sizes.h> >>> #include <xen/spinlock.h> >>> #include <asm/arm64/mpu.h> >>> +#include <asm/early_printk.h> >>> #include <asm/page.h> >>> +#include <asm/setup.h> >>> >>> #ifdef NDEBUG >>> static inline void >>> @@ -62,6 +67,8 @@ uint64_t __ro_after_init max_xen_mpumap; >>> >>> static DEFINE_SPINLOCK(xen_mpumap_lock); >>> >>> +static paddr_t dtb_paddr; >>> + >>> /* Write a MPU protection region */ >>> #define WRITE_PROTECTION_REGION(sel, pr, prbar_el2, prlar_el2) ({ \ >>> uint64_t _sel = sel; \ >>> @@ -403,7 +410,16 @@ static int xen_mpumap_update_entry(paddr_t >> base, >>> paddr_t limit, >>> >>> /* During boot time, the default index is next_fixed_region_idx. */ >>> if ( system_state <= SYS_STATE_active ) >>> - idx = next_fixed_region_idx; >>> + { >>> + /* >>> + * If it is a boot-only region (i.e. region for early FDT), >>> + * it shall be added from the tail for late init re-organizing >>> + */ >>> + if ( REGION_BOOTONLY_MASK(flags) ) >>> + idx = next_transient_region_idx; >>> + else >>> + idx = next_fixed_region_idx; >>> + } >>> >>> xen_mpumap[idx] = pr_of_xenaddr(base, limit - 1, >> REGION_AI_MASK(flags)); >>> /* Set permission */ >>> @@ -465,14 +481,51 @@ int map_pages_to_xen(unsigned long virt, >>> mfn_to_maddr(mfn_add(mfn, nr_mfns)), flags); >>> } >>> >>> -/* TODO: Implementation on the first usage */ -void >>> dump_hyp_walk(vaddr_t addr) >>> +void * __init early_fdt_map(paddr_t fdt_paddr) >>> { >>> + void *fdt_virt; >>> + uint32_t size; >>> + >>> + /* >>> + * Check whether the physical FDT address is set and meets the >> minimum >>> + * alignment requirement. Since we are relying on MIN_FDT_ALIGN to >> be at >>> + * least 8 bytes so that we always access the magic and size fields >>> + * of the FDT header after mapping the first chunk, double check if >>> + * that is indeed the case. >>> + */ >>> + BUILD_BUG_ON(MIN_FDT_ALIGN < 8); >>> + if ( !fdt_paddr || fdt_paddr % MIN_FDT_ALIGN ) >>> + return NULL; >>> + >>> + dtb_paddr = fdt_paddr; >>> + /* >>> + * In MPU system, device tree binary can be packed with Xen image >>> + * through CONFIG_DTB_FILE, or provided by bootloader through x0. >> >> The behavior you describe is not specific to the MPU system. I also don't >> quite understand how describing the method to pass the DT actually matters >> here. >> >>> + * Map FDT with a transient MPU memory region of MAX_FDT_SIZE. >>> + * After that, we can do some magic check. >>> + */ >>> + if ( map_pages_to_xen(round_pgdown(fdt_paddr), >> >> I haven't looked at the rest of the series. But from here, it seems a bit strange >> to use map_pages_to_xen() because the virt and the phys should be the >> same. >> > > Hmm, t thought map_pages_to_xen, is to set up memory mapping for access. > In MPU, we also need to set up a MPU memory region for the FDT, even without > virt-to-phys conversion I think my point was misunderstood. I agree that we need a function to update the MPU. Instead I was asking whether using map_pages_to_xen() rather than creating a new helper with an MPU specific would not be better so we don't have to pass a pointless parameter (virt). That's why... > >> Do you plan to share some code where map_pages_to_xen() will be used? ... I was asking if you were going to share code with the MMU that may end up to use this function. If yes, then I agree in common code, it would be best to use map_pages_to_xen(). For MPU specific code, I would consider to provide an helper that doesn't need the virt to reduce the amount of unnecessary code. Cheers,
diff --git a/xen/arch/arm/include/asm/arm64/mpu.h b/xen/arch/arm/include/asm/arm64/mpu.h index fcde6ad0db..b85e420a90 100644 --- a/xen/arch/arm/include/asm/arm64/mpu.h +++ b/xen/arch/arm/include/asm/arm64/mpu.h @@ -45,18 +45,22 @@ * [3:4] Execute Never * [5:6] Access Permission * [7] Region Present + * [8] Boot-only Region */ #define _REGION_AI_BIT 0 #define _REGION_XN_BIT 3 #define _REGION_AP_BIT 5 #define _REGION_PRESENT_BIT 7 +#define _REGION_BOOTONLY_BIT 8 #define _REGION_XN (2U << _REGION_XN_BIT) #define _REGION_RO (2U << _REGION_AP_BIT) #define _REGION_PRESENT (1U << _REGION_PRESENT_BIT) +#define _REGION_BOOTONLY (1U << _REGION_BOOTONLY_BIT) #define REGION_AI_MASK(x) (((x) >> _REGION_AI_BIT) & 0x7U) #define REGION_XN_MASK(x) (((x) >> _REGION_XN_BIT) & 0x3U) #define REGION_AP_MASK(x) (((x) >> _REGION_AP_BIT) & 0x3U) #define REGION_RO_MASK(x) (((x) >> _REGION_AP_BIT) & 0x2U) +#define REGION_BOOTONLY_MASK(x) (((x) >> _REGION_BOOTONLY_BIT) & 0x1U) /* * _REGION_NORMAL is convenience define. It is not meant to be used @@ -68,6 +72,7 @@ #define REGION_HYPERVISOR_RO (_REGION_NORMAL|_REGION_XN|_REGION_RO) #define REGION_HYPERVISOR REGION_HYPERVISOR_RW +#define REGION_HYPERVISOR_BOOT (REGION_HYPERVISOR_RW|_REGION_BOOTONLY) #define INVALID_REGION (~0UL) diff --git a/xen/arch/arm/mm_mpu.c b/xen/arch/arm/mm_mpu.c index 08720a7c19..b34dbf4515 100644 --- a/xen/arch/arm/mm_mpu.c +++ b/xen/arch/arm/mm_mpu.c @@ -20,11 +20,16 @@ */ #include <xen/init.h> +#include <xen/libfdt/libfdt.h> #include <xen/mm.h> #include <xen/page-size.h> +#include <xen/pfn.h> +#include <xen/sizes.h> #include <xen/spinlock.h> #include <asm/arm64/mpu.h> +#include <asm/early_printk.h> #include <asm/page.h> +#include <asm/setup.h> #ifdef NDEBUG static inline void @@ -62,6 +67,8 @@ uint64_t __ro_after_init max_xen_mpumap; static DEFINE_SPINLOCK(xen_mpumap_lock); +static paddr_t dtb_paddr; + /* Write a MPU protection region */ #define WRITE_PROTECTION_REGION(sel, pr, prbar_el2, prlar_el2) ({ \ uint64_t _sel = sel; \ @@ -403,7 +410,16 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit, /* During boot time, the default index is next_fixed_region_idx. */ if ( system_state <= SYS_STATE_active ) - idx = next_fixed_region_idx; + { + /* + * If it is a boot-only region (i.e. region for early FDT), + * it shall be added from the tail for late init re-organizing + */ + if ( REGION_BOOTONLY_MASK(flags) ) + idx = next_transient_region_idx; + else + idx = next_fixed_region_idx; + } xen_mpumap[idx] = pr_of_xenaddr(base, limit - 1, REGION_AI_MASK(flags)); /* Set permission */ @@ -465,14 +481,51 @@ int map_pages_to_xen(unsigned long virt, mfn_to_maddr(mfn_add(mfn, nr_mfns)), flags); } -/* TODO: Implementation on the first usage */ -void dump_hyp_walk(vaddr_t addr) +void * __init early_fdt_map(paddr_t fdt_paddr) { + void *fdt_virt; + uint32_t size; + + /* + * Check whether the physical FDT address is set and meets the minimum + * alignment requirement. Since we are relying on MIN_FDT_ALIGN to be at + * least 8 bytes so that we always access the magic and size fields + * of the FDT header after mapping the first chunk, double check if + * that is indeed the case. + */ + BUILD_BUG_ON(MIN_FDT_ALIGN < 8); + if ( !fdt_paddr || fdt_paddr % MIN_FDT_ALIGN ) + return NULL; + + dtb_paddr = fdt_paddr; + /* + * In MPU system, device tree binary can be packed with Xen image + * through CONFIG_DTB_FILE, or provided by bootloader through x0. + * Map FDT with a transient MPU memory region of MAX_FDT_SIZE. + * After that, we can do some magic check. + */ + if ( map_pages_to_xen(round_pgdown(fdt_paddr), + maddr_to_mfn(round_pgdown(fdt_paddr)), + round_pgup(MAX_FDT_SIZE) >> PAGE_SHIFT, + REGION_HYPERVISOR_BOOT) ) + panic("Unable to map the device-tree.\n"); + + /* VA == PA */ + fdt_virt = maddr_to_virt(fdt_paddr); + + if ( fdt_magic(fdt_virt) != FDT_MAGIC ) + return NULL; + + size = fdt_totalsize(fdt_virt); + if ( size > MAX_FDT_SIZE ) + return NULL; + + return fdt_virt; } -void * __init early_fdt_map(paddr_t fdt_paddr) +/* TODO: Implementation on the first usage */ +void dump_hyp_walk(vaddr_t addr) { - return NULL; } void __init remove_early_mappings(void) diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index 79965a3c17..0565e22a1f 100644 --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -218,7 +218,10 @@ SECTIONS _end = . ; /* Section for the device tree blob (if any). */ - .dtb : { *(.dtb) } :text + .dtb : { + . = ALIGN(PAGE_SIZE); + *(.dtb) + } :text DWARF2_DEBUG_SECTIONS