| Message ID | 20230626033443.2943270-32-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 |
On 26/06/2023 04:34, Penny Zheng wrote: > CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email. > > > In MPU system, MPU memory region is always mapped PAGE_ALIGN, so in order to > not access unexpected memory area, dtb section in xen.lds.S 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, as > it will be relocated into heap and unmapped at the end of boot. > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > Signed-off-by: Wei Chen <wei.chen@arm.com> > --- > v3: > - map the first 2MB. Check the size and then re-map with an extra 2MB if needed > --- > xen/arch/arm/include/asm/arm64/mpu.h | 3 ++- > xen/arch/arm/include/asm/page.h | 5 +++++ > xen/arch/arm/mm.c | 26 ++++++++++++++++++++------ > xen/arch/arm/mpu/mm.c | 1 + > xen/arch/arm/xen.lds.S | 5 ++++- > 5 files changed, 32 insertions(+), 8 deletions(-) > > diff --git a/xen/arch/arm/include/asm/arm64/mpu.h b/xen/arch/arm/include/asm/arm64/mpu.h > index a6b07bab02..715ea69884 100644 > --- a/xen/arch/arm/include/asm/arm64/mpu.h > +++ b/xen/arch/arm/include/asm/arm64/mpu.h > @@ -72,7 +72,8 @@ typedef union { > unsigned long ns:1; /* Not-Secure */ > unsigned long res:1; /* Reserved 0 by hardware */ > unsigned long limit:42; /* Limit Address */ > - unsigned long pad:16; > + unsigned long pad:15; > + unsigned long tran:1; /* Transient region */ > } reg; > uint64_t bits; > } prlar_t; > diff --git a/xen/arch/arm/include/asm/page.h b/xen/arch/arm/include/asm/page.h > index 85ecd5e4de..a434e2205a 100644 > --- a/xen/arch/arm/include/asm/page.h > +++ b/xen/arch/arm/include/asm/page.h > @@ -97,19 +97,24 @@ > * [3:4] Execute Never > * [5:6] Access Permission > * [7] Region Present > + * [8] Transient Region, e.g. MPU memory region is temproraily > + * mapped for a short time > */ > #define _PAGE_AI_BIT 0 > #define _PAGE_XN_BIT 3 > #define _PAGE_AP_BIT 5 > #define _PAGE_PRESENT_BIT 7 > +#define _PAGE_TRANSIENT_BIT 8 I don't think this is related to MPU. At least when I look at the bit representation of PRBAR_EL1/2, bits [47:6] are for the base address. If my understanding is correct, then please take it out of this patch and put it in a separate MMU related patch. > #define _PAGE_AI (7U << _PAGE_AI_BIT) > #define _PAGE_XN (2U << _PAGE_XN_BIT) > #define _PAGE_RO (2U << _PAGE_AP_BIT) > #define _PAGE_PRESENT (1U << _PAGE_PRESENT_BIT) > +#define _PAGE_TRANSIENT (1U << _PAGE_TRANSIENT_BIT) > #define PAGE_AI_MASK(x) (((x) >> _PAGE_AI_BIT) & 0x7U) > #define PAGE_XN_MASK(x) (((x) >> _PAGE_XN_BIT) & 0x3U) > #define PAGE_AP_MASK(x) (((x) >> _PAGE_AP_BIT) & 0x3U) > #define PAGE_RO_MASK(x) (((x) >> _PAGE_AP_BIT) & 0x2U) > +#define PAGE_TRANSIENT_MASK(x) (((x) >> _PAGE_TRANSIENT_BIT) & 0x1U) > #endif /* CONFIG_HAS_MPU */ > > /* > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index d35e7e280f..8625066256 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -61,8 +61,17 @@ void flush_page_to_ram(unsigned long mfn, bool sync_icache) > > void * __init early_fdt_map(paddr_t fdt_paddr) > { > +#ifndef CONFIG_HAS_MPU > /* We are using 2MB superpage for mapping the FDT */ > paddr_t base_paddr = fdt_paddr & SECOND_MASK; > + unsigned int flags = PAGE_HYPERVISOR_RO | _PAGE_BLOCK; > + unsigned long base_virt = BOOT_FDT_VIRT_START; > +#else > + /* MPU region must be PAGE aligned */ > + paddr_t base_paddr = fdt_paddr & PAGE_MASK; > + unsigned int flags = PAGE_HYPERVISOR_RO | _PAGE_TRANSIENT; > + unsigned long base_virt = ~0UL; > +#endif > paddr_t offset; > void *fdt_virt; > uint32_t size; > @@ -79,18 +88,24 @@ void * __init early_fdt_map(paddr_t fdt_paddr) > if ( !fdt_paddr || fdt_paddr % MIN_FDT_ALIGN ) > return NULL; > > +#ifndef CONFIG_HAS_MPU > /* The FDT is mapped using 2MB superpage */ > BUILD_BUG_ON(BOOT_FDT_VIRT_START % SZ_2M); > +#endif > > - rc = map_pages_to_xen(BOOT_FDT_VIRT_START, maddr_to_mfn(base_paddr), > - SZ_2M >> PAGE_SHIFT, > - PAGE_HYPERVISOR_RO | _PAGE_BLOCK); > + rc = map_pages_to_xen(base_virt, maddr_to_mfn(base_paddr), > + SZ_2M >> PAGE_SHIFT, flags); > if ( rc ) > panic("Unable to map the device-tree.\n"); > > > +#ifndef CONFIG_HAS_MPU > offset = fdt_paddr % SECOND_SIZE; > fdt_virt = (void *)BOOT_FDT_VIRT_START + offset; > +#else > + offset = fdt_paddr % PAGE_SIZE; > + fdt_virt = (void *)fdt_paddr; > +#endif > > if ( fdt_magic(fdt_virt) != FDT_MAGIC ) > return NULL; > @@ -101,10 +116,9 @@ void * __init early_fdt_map(paddr_t fdt_paddr) > > if ( (offset + size) > SZ_2M ) > { > - rc = map_pages_to_xen(BOOT_FDT_VIRT_START + SZ_2M, > + rc = map_pages_to_xen(base_virt + SZ_2M, > maddr_to_mfn(base_paddr + SZ_2M), > - SZ_2M >> PAGE_SHIFT, > - PAGE_HYPERVISOR_RO | _PAGE_BLOCK); > + SZ_2M >> PAGE_SHIFT, flags); > if ( rc ) > panic("Unable to map the device-tree\n"); > } > diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c > index 14a1309ca1..f4ce19d36a 100644 > --- a/xen/arch/arm/mpu/mm.c > +++ b/xen/arch/arm/mpu/mm.c > @@ -448,6 +448,7 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit, > /* Set permission */ > xen_mpumap[idx].prbar.reg.ap = PAGE_AP_MASK(flags); > xen_mpumap[idx].prbar.reg.xn = PAGE_XN_MASK(flags); > + xen_mpumap[idx].prlar.reg.tran = PAGE_TRANSIENT_MASK(flags); ReferARM DDI 0600A.dID120821 , G1.3.23, PRLAR_EL2, Protection Region Limit Address Register (EL2), I don't seethis bit described anywhere. Do we really need this bit for MPU ? - Ayan > > write_protection_region((const pr_t*)(&xen_mpumap[idx]), idx); > } > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S > index 4f7daa7dca..f2715d7cb7 100644 > --- a/xen/arch/arm/xen.lds.S > +++ b/xen/arch/arm/xen.lds.S > @@ -216,7 +216,10 @@ SECTIONS > _end = . ; > > /* Section for the device tree blob (if any). */ > - .dtb : { *(.dtb) } :text > + .dtb : { > + . = ALIGN(PAGE_SIZE); > + *(.dtb) > + } :text > > DWARF2_DEBUG_SECTIONS > > -- > 2.25.1 > >
Hi, On 2023/6/30 01:22, Ayan Kumar Halder wrote: > > On 26/06/2023 04:34, Penny Zheng wrote: >> CAUTION: This message has originated from an External Source. Please >> use proper judgment and caution when opening attachments, clicking >> links, or responding to this email. >> >> >> In MPU system, MPU memory region is always mapped PAGE_ALIGN, so in >> order to >> not access unexpected memory area, dtb section in xen.lds.S 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, as >> it will be relocated into heap and unmapped at the end of boot. >> >> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >> Signed-off-by: Wei Chen <wei.chen@arm.com> >> --- >> v3: >> - map the first 2MB. Check the size and then re-map with an extra 2MB >> if needed >> --- >> xen/arch/arm/include/asm/arm64/mpu.h | 3 ++- >> xen/arch/arm/include/asm/page.h | 5 +++++ >> xen/arch/arm/mm.c | 26 ++++++++++++++++++++------ >> xen/arch/arm/mpu/mm.c | 1 + >> xen/arch/arm/xen.lds.S | 5 ++++- >> 5 files changed, 32 insertions(+), 8 deletions(-) >> >> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h >> b/xen/arch/arm/include/asm/arm64/mpu.h >> index a6b07bab02..715ea69884 100644 >> --- a/xen/arch/arm/include/asm/arm64/mpu.h >> +++ b/xen/arch/arm/include/asm/arm64/mpu.h >> @@ -72,7 +72,8 @@ typedef union { >> unsigned long ns:1; /* Not-Secure */ >> unsigned long res:1; /* Reserved 0 by hardware */ >> unsigned long limit:42; /* Limit Address */ >> - unsigned long pad:16; >> + unsigned long pad:15; >> + unsigned long tran:1; /* Transient region */ >> } reg; >> uint64_t bits; >> } prlar_t; >> diff --git a/xen/arch/arm/include/asm/page.h >> b/xen/arch/arm/include/asm/page.h >> index 85ecd5e4de..a434e2205a 100644 >> --- a/xen/arch/arm/include/asm/page.h >> +++ b/xen/arch/arm/include/asm/page.h >> @@ -97,19 +97,24 @@ >> * [3:4] Execute Never >> * [5:6] Access Permission >> * [7] Region Present >> + * [8] Transient Region, e.g. MPU memory region is temproraily >> + * mapped for a short time >> */ >> #define _PAGE_AI_BIT 0 >> #define _PAGE_XN_BIT 3 >> #define _PAGE_AP_BIT 5 >> #define _PAGE_PRESENT_BIT 7 >> +#define _PAGE_TRANSIENT_BIT 8 > I don't think this is related to MPU. At least when I look at the bit > representation of PRBAR_EL1/2, This set of _PAGE_xxx flags aren't compliant with PRBAR_EL1/2 register map. It is a flag passed to function map_pages_to_xen() to indicate memory attributes and permission. This bit _PAGE_TRANSIENT is to tell whether the new adding MPU region is a fixed one, or a temporary one. The MPU region created for early FDT is a temporary one, as it will be unmapped at the end of boot. > > bits [47:6] are for the base address. > > If my understanding is correct, then please take it out of this patch > and put it in a separate MMU related patch. > >> #define _PAGE_AI (7U << _PAGE_AI_BIT) >> #define _PAGE_XN (2U << _PAGE_XN_BIT) >> #define _PAGE_RO (2U << _PAGE_AP_BIT) >> #define _PAGE_PRESENT (1U << _PAGE_PRESENT_BIT) >> +#define _PAGE_TRANSIENT (1U << _PAGE_TRANSIENT_BIT) >> #define PAGE_AI_MASK(x) (((x) >> _PAGE_AI_BIT) & 0x7U) >> #define PAGE_XN_MASK(x) (((x) >> _PAGE_XN_BIT) & 0x3U) >> #define PAGE_AP_MASK(x) (((x) >> _PAGE_AP_BIT) & 0x3U) >> #define PAGE_RO_MASK(x) (((x) >> _PAGE_AP_BIT) & 0x2U) >> +#define PAGE_TRANSIENT_MASK(x) (((x) >> _PAGE_TRANSIENT_BIT) & 0x1U) >> #endif /* CONFIG_HAS_MPU */ >> >> /* >> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c >> index d35e7e280f..8625066256 100644 >> --- a/xen/arch/arm/mm.c >> +++ b/xen/arch/arm/mm.c >> @@ -61,8 +61,17 @@ void flush_page_to_ram(unsigned long mfn, bool >> sync_icache) >> >> void * __init early_fdt_map(paddr_t fdt_paddr) >> { >> +#ifndef CONFIG_HAS_MPU >> /* We are using 2MB superpage for mapping the FDT */ >> paddr_t base_paddr = fdt_paddr & SECOND_MASK; >> + unsigned int flags = PAGE_HYPERVISOR_RO | _PAGE_BLOCK; >> + unsigned long base_virt = BOOT_FDT_VIRT_START; >> +#else >> + /* MPU region must be PAGE aligned */ >> + paddr_t base_paddr = fdt_paddr & PAGE_MASK; >> + unsigned int flags = PAGE_HYPERVISOR_RO | _PAGE_TRANSIENT; >> + unsigned long base_virt = ~0UL; >> +#endif >> paddr_t offset; >> void *fdt_virt; >> uint32_t size; >> @@ -79,18 +88,24 @@ void * __init early_fdt_map(paddr_t fdt_paddr) >> if ( !fdt_paddr || fdt_paddr % MIN_FDT_ALIGN ) >> return NULL; >> >> +#ifndef CONFIG_HAS_MPU >> /* The FDT is mapped using 2MB superpage */ >> BUILD_BUG_ON(BOOT_FDT_VIRT_START % SZ_2M); >> +#endif >> >> - rc = map_pages_to_xen(BOOT_FDT_VIRT_START, maddr_to_mfn(base_paddr), >> - SZ_2M >> PAGE_SHIFT, >> - PAGE_HYPERVISOR_RO | _PAGE_BLOCK); >> + rc = map_pages_to_xen(base_virt, maddr_to_mfn(base_paddr), >> + SZ_2M >> PAGE_SHIFT, flags); >> if ( rc ) >> panic("Unable to map the device-tree.\n"); >> >> >> +#ifndef CONFIG_HAS_MPU >> offset = fdt_paddr % SECOND_SIZE; >> fdt_virt = (void *)BOOT_FDT_VIRT_START + offset; >> +#else >> + offset = fdt_paddr % PAGE_SIZE; >> + fdt_virt = (void *)fdt_paddr; >> +#endif >> >> if ( fdt_magic(fdt_virt) != FDT_MAGIC ) >> return NULL; >> @@ -101,10 +116,9 @@ void * __init early_fdt_map(paddr_t fdt_paddr) >> >> if ( (offset + size) > SZ_2M ) >> { >> - rc = map_pages_to_xen(BOOT_FDT_VIRT_START + SZ_2M, >> + rc = map_pages_to_xen(base_virt + SZ_2M, >> maddr_to_mfn(base_paddr + SZ_2M), >> - SZ_2M >> PAGE_SHIFT, >> - PAGE_HYPERVISOR_RO | _PAGE_BLOCK); >> + SZ_2M >> PAGE_SHIFT, flags); >> if ( rc ) >> panic("Unable to map the device-tree\n"); >> } >> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c >> index 14a1309ca1..f4ce19d36a 100644 >> --- a/xen/arch/arm/mpu/mm.c >> +++ b/xen/arch/arm/mpu/mm.c >> @@ -448,6 +448,7 @@ static int xen_mpumap_update_entry(paddr_t base, >> paddr_t limit, >> /* Set permission */ >> xen_mpumap[idx].prbar.reg.ap = PAGE_AP_MASK(flags); >> xen_mpumap[idx].prbar.reg.xn = PAGE_XN_MASK(flags); >> + xen_mpumap[idx].prlar.reg.tran = PAGE_TRANSIENT_MASK(flags); > > ReferARM DDI 0600A.dID120821 , G1.3.23, PRLAR_EL2, Protection Region > Limit Address Register (EL2), I don't seethis bit described anywhere. Do > we really need this bit for MPU ? > Yes, It is introduced for software implementation. You could see commit [PATCH v3 36/52] xen/mpu: implememt ioremap_xxx in MPU and commit [PATCH v3 38/52] xen/mpu: map domain page in MPU system for better understanding. > > - Ayan > >> >> write_protection_region((const pr_t*)(&xen_mpumap[idx]), idx); >> } >> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S >> index 4f7daa7dca..f2715d7cb7 100644 >> --- a/xen/arch/arm/xen.lds.S >> +++ b/xen/arch/arm/xen.lds.S >> @@ -216,7 +216,10 @@ SECTIONS >> _end = . ; >> >> /* Section for the device tree blob (if any). */ >> - .dtb : { *(.dtb) } :text >> + .dtb : { >> + . = ALIGN(PAGE_SIZE); >> + *(.dtb) >> + } :text >> >> DWARF2_DEBUG_SECTIONS >> >> -- >> 2.25.1 >> >>
On 30/06/2023 05:07, Penny Zheng wrote: > Hi, Hi Penny, > > > On 2023/6/30 01:22, Ayan Kumar Halder wrote: >> >> On 26/06/2023 04:34, Penny Zheng wrote: >>> CAUTION: This message has originated from an External Source. Please >>> use proper judgment and caution when opening attachments, clicking >>> links, or responding to this email. >>> >>> >>> In MPU system, MPU memory region is always mapped PAGE_ALIGN, so in >>> order to >>> not access unexpected memory area, dtb section in xen.lds.S 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, as >>> it will be relocated into heap and unmapped at the end of boot. >>> >>> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >>> Signed-off-by: Wei Chen <wei.chen@arm.com> >>> --- >>> v3: >>> - map the first 2MB. Check the size and then re-map with an extra >>> 2MB if needed >>> --- >>> xen/arch/arm/include/asm/arm64/mpu.h | 3 ++- >>> xen/arch/arm/include/asm/page.h | 5 +++++ >>> xen/arch/arm/mm.c | 26 ++++++++++++++++++++------ >>> xen/arch/arm/mpu/mm.c | 1 + >>> xen/arch/arm/xen.lds.S | 5 ++++- >>> 5 files changed, 32 insertions(+), 8 deletions(-) >>> >>> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h >>> b/xen/arch/arm/include/asm/arm64/mpu.h >>> index a6b07bab02..715ea69884 100644 >>> --- a/xen/arch/arm/include/asm/arm64/mpu.h >>> +++ b/xen/arch/arm/include/asm/arm64/mpu.h >>> @@ -72,7 +72,8 @@ typedef union { >>> unsigned long ns:1; /* Not-Secure */ >>> unsigned long res:1; /* Reserved 0 by hardware */ >>> unsigned long limit:42; /* Limit Address */ >>> - unsigned long pad:16; >>> + unsigned long pad:15; >>> + unsigned long tran:1; /* Transient region */ >>> } reg; >>> uint64_t bits; >>> } prlar_t; >>> diff --git a/xen/arch/arm/include/asm/page.h >>> b/xen/arch/arm/include/asm/page.h >>> index 85ecd5e4de..a434e2205a 100644 >>> --- a/xen/arch/arm/include/asm/page.h >>> +++ b/xen/arch/arm/include/asm/page.h >>> @@ -97,19 +97,24 @@ >>> * [3:4] Execute Never >>> * [5:6] Access Permission >>> * [7] Region Present >>> + * [8] Transient Region, e.g. MPU memory region is temproraily >>> + * mapped for a short time >>> */ >>> #define _PAGE_AI_BIT 0 >>> #define _PAGE_XN_BIT 3 >>> #define _PAGE_AP_BIT 5 >>> #define _PAGE_PRESENT_BIT 7 >>> +#define _PAGE_TRANSIENT_BIT 8 >> I don't think this is related to MPU. At least when I look at the bit >> representation of PRBAR_EL1/2, > > This set of _PAGE_xxx flags aren't compliant with PRBAR_EL1/2 register > map. > It is a flag passed to function map_pages_to_xen() to indicate memory > attributes and permission. But aren't you writing these flags to PRBAR_EL1/EL2 when you call xen_mpumap_update_entry(). In the below snippet of xen_mpumap_update_entry(), IIUC, you are writing these flags. xen_mpumap[idx].prbar.reg.ap = PAGE_AP_MASK(flags); xen_mpumap[idx].prbar.reg.xn = PAGE_XN_MASK(flags); write_protection_region((const pr_t*)(&xen_mpumap[idx]), idx); Please clarify here. In this case, I don't prefer mixing hardware specific bits with software only representation for these reasons :- 1. It makes it confusing and hard to differentiate the hardware specific attrbutes from software only. 2. Also, refer xen/arch/arm/include/asm/arm64/mpu.h, typedef union prbar_t {} :- typedef union { struct __packed { unsigned long xn:2; /* Execute-Never */ unsigned long ap:2; /* Acess Permission */ unsigned long sh:2; /* Sharebility */ unsigned long base:42; /* Base Address */ unsigned long pad:12; unsigned long p2m_type:4; /* Ignore by hardware. Used to store p2m types.*/ ** ReferARM DDI 0600A.d ID120821, Glossary for RES0 For a bit in a read/write register, it is IMPLEMENTATION DEFINED whether: 1. The bit is hardwired to 0. In this case: * Reads of the bit always return 0. * Writes to the bit are ignored. If the writes are ignored and you read back 0s, then the software logic which relies on p2m_type can break. Thus, I will prefer keeping RES0 unused so that the software works consistently across all silicons. ** } reg; uint64_t bits; } prbar_t; I see you are using RES0 bits in prlar_t {} as well. My objections applies here as well. - Ayan > This bit _PAGE_TRANSIENT is to tell whether the new adding MPU region > is a fixed one, or a temporary one. > The MPU region created for early FDT is a temporary one, as it will be > unmapped at the end of boot. > >> >> bits [47:6] are for the base address. >> >> If my understanding is correct, then please take it out of this patch >> and put it in a separate MMU related patch. >> >>> #define _PAGE_AI (7U << _PAGE_AI_BIT) >>> #define _PAGE_XN (2U << _PAGE_XN_BIT) >>> #define _PAGE_RO (2U << _PAGE_AP_BIT) >>> #define _PAGE_PRESENT (1U << _PAGE_PRESENT_BIT) >>> +#define _PAGE_TRANSIENT (1U << _PAGE_TRANSIENT_BIT) >>> #define PAGE_AI_MASK(x) (((x) >> _PAGE_AI_BIT) & 0x7U) >>> #define PAGE_XN_MASK(x) (((x) >> _PAGE_XN_BIT) & 0x3U) >>> #define PAGE_AP_MASK(x) (((x) >> _PAGE_AP_BIT) & 0x3U) >>> #define PAGE_RO_MASK(x) (((x) >> _PAGE_AP_BIT) & 0x2U) >>> +#define PAGE_TRANSIENT_MASK(x) (((x) >> _PAGE_TRANSIENT_BIT) & 0x1U) >>> #endif /* CONFIG_HAS_MPU */ >>> >>> /* >>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c >>> index d35e7e280f..8625066256 100644 >>> --- a/xen/arch/arm/mm.c >>> +++ b/xen/arch/arm/mm.c >>> @@ -61,8 +61,17 @@ void flush_page_to_ram(unsigned long mfn, bool >>> sync_icache) >>> >>> void * __init early_fdt_map(paddr_t fdt_paddr) >>> { >>> +#ifndef CONFIG_HAS_MPU >>> /* We are using 2MB superpage for mapping the FDT */ >>> paddr_t base_paddr = fdt_paddr & SECOND_MASK; >>> + unsigned int flags = PAGE_HYPERVISOR_RO | _PAGE_BLOCK; >>> + unsigned long base_virt = BOOT_FDT_VIRT_START; >>> +#else >>> + /* MPU region must be PAGE aligned */ >>> + paddr_t base_paddr = fdt_paddr & PAGE_MASK; >>> + unsigned int flags = PAGE_HYPERVISOR_RO | _PAGE_TRANSIENT; >>> + unsigned long base_virt = ~0UL; >>> +#endif >>> paddr_t offset; >>> void *fdt_virt; >>> uint32_t size; >>> @@ -79,18 +88,24 @@ void * __init early_fdt_map(paddr_t fdt_paddr) >>> if ( !fdt_paddr || fdt_paddr % MIN_FDT_ALIGN ) >>> return NULL; >>> >>> +#ifndef CONFIG_HAS_MPU >>> /* The FDT is mapped using 2MB superpage */ >>> BUILD_BUG_ON(BOOT_FDT_VIRT_START % SZ_2M); >>> +#endif >>> >>> - rc = map_pages_to_xen(BOOT_FDT_VIRT_START, >>> maddr_to_mfn(base_paddr), >>> - SZ_2M >> PAGE_SHIFT, >>> - PAGE_HYPERVISOR_RO | _PAGE_BLOCK); >>> + rc = map_pages_to_xen(base_virt, maddr_to_mfn(base_paddr), >>> + SZ_2M >> PAGE_SHIFT, flags); >>> if ( rc ) >>> panic("Unable to map the device-tree.\n"); >>> >>> >>> +#ifndef CONFIG_HAS_MPU >>> offset = fdt_paddr % SECOND_SIZE; >>> fdt_virt = (void *)BOOT_FDT_VIRT_START + offset; >>> +#else >>> + offset = fdt_paddr % PAGE_SIZE; >>> + fdt_virt = (void *)fdt_paddr; >>> +#endif >>> >>> if ( fdt_magic(fdt_virt) != FDT_MAGIC ) >>> return NULL; >>> @@ -101,10 +116,9 @@ void * __init early_fdt_map(paddr_t fdt_paddr) >>> >>> if ( (offset + size) > SZ_2M ) >>> { >>> - rc = map_pages_to_xen(BOOT_FDT_VIRT_START + SZ_2M, >>> + rc = map_pages_to_xen(base_virt + SZ_2M, >>> maddr_to_mfn(base_paddr + SZ_2M), >>> - SZ_2M >> PAGE_SHIFT, >>> - PAGE_HYPERVISOR_RO | _PAGE_BLOCK); >>> + SZ_2M >> PAGE_SHIFT, flags); >>> if ( rc ) >>> panic("Unable to map the device-tree\n"); >>> } >>> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c >>> index 14a1309ca1..f4ce19d36a 100644 >>> --- a/xen/arch/arm/mpu/mm.c >>> +++ b/xen/arch/arm/mpu/mm.c >>> @@ -448,6 +448,7 @@ static int xen_mpumap_update_entry(paddr_t base, >>> paddr_t limit, >>> /* Set permission */ >>> xen_mpumap[idx].prbar.reg.ap = PAGE_AP_MASK(flags); >>> xen_mpumap[idx].prbar.reg.xn = PAGE_XN_MASK(flags); >>> + xen_mpumap[idx].prlar.reg.tran = PAGE_TRANSIENT_MASK(flags); >> >> ReferARM DDI 0600A.dID120821 , G1.3.23, PRLAR_EL2, Protection Region >> Limit Address Register (EL2), I don't seethis bit described anywhere. >> Do we really need this bit for MPU ? >> > > Yes, It is introduced for software implementation. > You could see commit [PATCH v3 36/52] xen/mpu: implememt ioremap_xxx > in MPU and commit [PATCH v3 38/52] xen/mpu: map domain page in MPU > system for better understanding. > >> >> - Ayan >> >>> >>> write_protection_region((const pr_t*)(&xen_mpumap[idx]), >>> idx); >>> } >>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S >>> index 4f7daa7dca..f2715d7cb7 100644 >>> --- a/xen/arch/arm/xen.lds.S >>> +++ b/xen/arch/arm/xen.lds.S >>> @@ -216,7 +216,10 @@ SECTIONS >>> _end = . ; >>> >>> /* Section for the device tree blob (if any). */ >>> - .dtb : { *(.dtb) } :text >>> + .dtb : { >>> + . = ALIGN(PAGE_SIZE); >>> + *(.dtb) >>> + } :text >>> >>> DWARF2_DEBUG_SECTIONS >>> >>> -- >>> 2.25.1 >>> >>>
On 30/06/2023 11:49, Ayan Kumar Halder wrote: > > On 30/06/2023 05:07, Penny Zheng wrote: >> Hi, > Hi Penny, >> >> >> On 2023/6/30 01:22, Ayan Kumar Halder wrote: >>> >>> On 26/06/2023 04:34, Penny Zheng wrote: >>>> CAUTION: This message has originated from an External Source. Please >>>> use proper judgment and caution when opening attachments, clicking >>>> links, or responding to this email. >>>> >>>> >>>> In MPU system, MPU memory region is always mapped PAGE_ALIGN, so in >>>> order to >>>> not access unexpected memory area, dtb section in xen.lds.S 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, as >>>> it will be relocated into heap and unmapped at the end of boot. >>>> >>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >>>> Signed-off-by: Wei Chen <wei.chen@arm.com> >>>> --- >>>> v3: >>>> - map the first 2MB. Check the size and then re-map with an extra >>>> 2MB if needed >>>> --- >>>> xen/arch/arm/include/asm/arm64/mpu.h | 3 ++- >>>> xen/arch/arm/include/asm/page.h | 5 +++++ >>>> xen/arch/arm/mm.c | 26 ++++++++++++++++++++------ >>>> xen/arch/arm/mpu/mm.c | 1 + >>>> xen/arch/arm/xen.lds.S | 5 ++++- >>>> 5 files changed, 32 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h >>>> b/xen/arch/arm/include/asm/arm64/mpu.h >>>> index a6b07bab02..715ea69884 100644 >>>> --- a/xen/arch/arm/include/asm/arm64/mpu.h >>>> +++ b/xen/arch/arm/include/asm/arm64/mpu.h >>>> @@ -72,7 +72,8 @@ typedef union { >>>> unsigned long ns:1; /* Not-Secure */ >>>> unsigned long res:1; /* Reserved 0 by hardware */ >>>> unsigned long limit:42; /* Limit Address */ >>>> - unsigned long pad:16; >>>> + unsigned long pad:15; >>>> + unsigned long tran:1; /* Transient region */ >>>> } reg; >>>> uint64_t bits; >>>> } prlar_t; >>>> diff --git a/xen/arch/arm/include/asm/page.h >>>> b/xen/arch/arm/include/asm/page.h >>>> index 85ecd5e4de..a434e2205a 100644 >>>> --- a/xen/arch/arm/include/asm/page.h >>>> +++ b/xen/arch/arm/include/asm/page.h >>>> @@ -97,19 +97,24 @@ >>>> * [3:4] Execute Never >>>> * [5:6] Access Permission >>>> * [7] Region Present >>>> + * [8] Transient Region, e.g. MPU memory region is temproraily >>>> + * mapped for a short time >>>> */ >>>> #define _PAGE_AI_BIT 0 >>>> #define _PAGE_XN_BIT 3 >>>> #define _PAGE_AP_BIT 5 >>>> #define _PAGE_PRESENT_BIT 7 >>>> +#define _PAGE_TRANSIENT_BIT 8 >>> I don't think this is related to MPU. At least when I look at the bit >>> representation of PRBAR_EL1/2, >> >> This set of _PAGE_xxx flags aren't compliant with PRBAR_EL1/2 register >> map. >> It is a flag passed to function map_pages_to_xen() to indicate memory >> attributes and permission. > > But aren't you writing these flags to PRBAR_EL1/EL2 when you call > xen_mpumap_update_entry(). > > In the below snippet of xen_mpumap_update_entry(), IIUC, you are writing > these flags. > > xen_mpumap[idx].prbar.reg.ap = PAGE_AP_MASK(flags); > xen_mpumap[idx].prbar.reg.xn = PAGE_XN_MASK(flags); > > write_protection_region((const pr_t*)(&xen_mpumap[idx]), idx); > > Please clarify here. > > In this case, I don't prefer mixing hardware specific bits with software > only representation for these reasons :- > > 1. It makes it confusing and hard to differentiate the hardware specific > attrbutes from software only. Penny's approach matches what we are doing in the MMU code. We want to have a way for the caller to pass just set of flags and let the callee to decide what to do with them. This may be flags converted for HW fields or just used by the logic. If you disagree with this approach, then can you propose a different way that we can discuss? > > 2. Also, refer xen/arch/arm/include/asm/arm64/mpu.h, typedef union > prbar_t {} :- > > typedef union { > struct __packed { > unsigned long xn:2; /* Execute-Never */ > unsigned long ap:2; /* Acess Permission */ > unsigned long sh:2; /* Sharebility */ > unsigned long base:42; /* Base Address */ > unsigned long pad:12; > unsigned long p2m_type:4; /* Ignore by hardware. Used to store > p2m types.*/ You are right, the top bits are RES0 (not ignored like on ARMv8-A). So we can't use to store the p2m_type. Cheers,
Hi Julien, On 30/06/2023 12:22, Julien Grall wrote: > > > On 30/06/2023 11:49, Ayan Kumar Halder wrote: >> >> On 30/06/2023 05:07, Penny Zheng wrote: >>> Hi, >> Hi Penny, >>> >>> >>> On 2023/6/30 01:22, Ayan Kumar Halder wrote: >>>> >>>> On 26/06/2023 04:34, Penny Zheng wrote: >>>>> CAUTION: This message has originated from an External Source. >>>>> Please use proper judgment and caution when opening attachments, >>>>> clicking links, or responding to this email. >>>>> >>>>> >>>>> In MPU system, MPU memory region is always mapped PAGE_ALIGN, so >>>>> in order to >>>>> not access unexpected memory area, dtb section in xen.lds.S 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, as >>>>> it will be relocated into heap and unmapped at the end of boot. >>>>> >>>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >>>>> Signed-off-by: Wei Chen <wei.chen@arm.com> >>>>> --- >>>>> v3: >>>>> - map the first 2MB. Check the size and then re-map with an extra >>>>> 2MB if needed >>>>> --- >>>>> xen/arch/arm/include/asm/arm64/mpu.h | 3 ++- >>>>> xen/arch/arm/include/asm/page.h | 5 +++++ >>>>> xen/arch/arm/mm.c | 26 >>>>> ++++++++++++++++++++------ >>>>> xen/arch/arm/mpu/mm.c | 1 + >>>>> xen/arch/arm/xen.lds.S | 5 ++++- >>>>> 5 files changed, 32 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h >>>>> b/xen/arch/arm/include/asm/arm64/mpu.h >>>>> index a6b07bab02..715ea69884 100644 >>>>> --- a/xen/arch/arm/include/asm/arm64/mpu.h >>>>> +++ b/xen/arch/arm/include/asm/arm64/mpu.h >>>>> @@ -72,7 +72,8 @@ typedef union { >>>>> unsigned long ns:1; /* Not-Secure */ >>>>> unsigned long res:1; /* Reserved 0 by hardware */ >>>>> unsigned long limit:42; /* Limit Address */ >>>>> - unsigned long pad:16; >>>>> + unsigned long pad:15; >>>>> + unsigned long tran:1; /* Transient region */ >>>>> } reg; >>>>> uint64_t bits; >>>>> } prlar_t; >>>>> diff --git a/xen/arch/arm/include/asm/page.h >>>>> b/xen/arch/arm/include/asm/page.h >>>>> index 85ecd5e4de..a434e2205a 100644 >>>>> --- a/xen/arch/arm/include/asm/page.h >>>>> +++ b/xen/arch/arm/include/asm/page.h >>>>> @@ -97,19 +97,24 @@ >>>>> * [3:4] Execute Never >>>>> * [5:6] Access Permission >>>>> * [7] Region Present >>>>> + * [8] Transient Region, e.g. MPU memory region is temproraily >>>>> + * mapped for a short time >>>>> */ >>>>> #define _PAGE_AI_BIT 0 >>>>> #define _PAGE_XN_BIT 3 >>>>> #define _PAGE_AP_BIT 5 >>>>> #define _PAGE_PRESENT_BIT 7 >>>>> +#define _PAGE_TRANSIENT_BIT 8 >>>> I don't think this is related to MPU. At least when I look at the >>>> bit representation of PRBAR_EL1/2, >>> >>> This set of _PAGE_xxx flags aren't compliant with PRBAR_EL1/2 >>> register map. >>> It is a flag passed to function map_pages_to_xen() to indicate memory >>> attributes and permission. >> >> But aren't you writing these flags to PRBAR_EL1/EL2 when you call >> xen_mpumap_update_entry(). >> >> In the below snippet of xen_mpumap_update_entry(), IIUC, you are >> writing these flags. >> >> xen_mpumap[idx].prbar.reg.ap = PAGE_AP_MASK(flags); >> xen_mpumap[idx].prbar.reg.xn = PAGE_XN_MASK(flags); >> >> write_protection_region((const pr_t*)(&xen_mpumap[idx]), idx); >> >> Please clarify here. >> >> In this case, I don't prefer mixing hardware specific bits with >> software only representation for these reasons :- >> >> 1. It makes it confusing and hard to differentiate the hardware >> specific attrbutes from software only. > > Penny's approach matches what we are doing in the MMU code. We want to > have a way for the caller to pass just set of flags and let the callee > to decide what to do with them. > > This may be flags converted for HW fields or just used by the logic. > > If you disagree with this approach, then can you propose a different > way that we can discuss? In MMU, I could see "struct lpae_pt_t{ .avail }" is used for reference count. Something like this might look better (where the hardware specific bits are not reused for sw logic) struct { struct __packed { ... /* the existing bits as they are */ } lpae_pt_t; unsigned int ref_count; } p2m_entry And use "p2m_entry.ref_count" for the reference counting. I see that this change is quite intrusive, thus I will not argue for this. - Ayan
Hi, On 30/06/2023 15:42, Ayan Kumar Halder wrote: > Hi Julien, > > On 30/06/2023 12:22, Julien Grall wrote: >> >> >> On 30/06/2023 11:49, Ayan Kumar Halder wrote: >>> >>> On 30/06/2023 05:07, Penny Zheng wrote: >>>> Hi, >>> Hi Penny, >>>> >>>> >>>> On 2023/6/30 01:22, Ayan Kumar Halder wrote: >>>>> >>>>> On 26/06/2023 04:34, Penny Zheng wrote: >>>>>> CAUTION: This message has originated from an External Source. >>>>>> Please use proper judgment and caution when opening attachments, >>>>>> clicking links, or responding to this email. >>>>>> >>>>>> >>>>>> In MPU system, MPU memory region is always mapped PAGE_ALIGN, so >>>>>> in order to >>>>>> not access unexpected memory area, dtb section in xen.lds.S 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, as >>>>>> it will be relocated into heap and unmapped at the end of boot. >>>>>> >>>>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >>>>>> Signed-off-by: Wei Chen <wei.chen@arm.com> >>>>>> --- >>>>>> v3: >>>>>> - map the first 2MB. Check the size and then re-map with an extra >>>>>> 2MB if needed >>>>>> --- >>>>>> xen/arch/arm/include/asm/arm64/mpu.h | 3 ++- >>>>>> xen/arch/arm/include/asm/page.h | 5 +++++ >>>>>> xen/arch/arm/mm.c | 26 >>>>>> ++++++++++++++++++++------ >>>>>> xen/arch/arm/mpu/mm.c | 1 + >>>>>> xen/arch/arm/xen.lds.S | 5 ++++- >>>>>> 5 files changed, 32 insertions(+), 8 deletions(-) >>>>>> >>>>>> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h >>>>>> b/xen/arch/arm/include/asm/arm64/mpu.h >>>>>> index a6b07bab02..715ea69884 100644 >>>>>> --- a/xen/arch/arm/include/asm/arm64/mpu.h >>>>>> +++ b/xen/arch/arm/include/asm/arm64/mpu.h >>>>>> @@ -72,7 +72,8 @@ typedef union { >>>>>> unsigned long ns:1; /* Not-Secure */ >>>>>> unsigned long res:1; /* Reserved 0 by hardware */ >>>>>> unsigned long limit:42; /* Limit Address */ >>>>>> - unsigned long pad:16; >>>>>> + unsigned long pad:15; >>>>>> + unsigned long tran:1; /* Transient region */ >>>>>> } reg; >>>>>> uint64_t bits; >>>>>> } prlar_t; >>>>>> diff --git a/xen/arch/arm/include/asm/page.h >>>>>> b/xen/arch/arm/include/asm/page.h >>>>>> index 85ecd5e4de..a434e2205a 100644 >>>>>> --- a/xen/arch/arm/include/asm/page.h >>>>>> +++ b/xen/arch/arm/include/asm/page.h >>>>>> @@ -97,19 +97,24 @@ >>>>>> * [3:4] Execute Never >>>>>> * [5:6] Access Permission >>>>>> * [7] Region Present >>>>>> + * [8] Transient Region, e.g. MPU memory region is temproraily >>>>>> + * mapped for a short time >>>>>> */ >>>>>> #define _PAGE_AI_BIT 0 >>>>>> #define _PAGE_XN_BIT 3 >>>>>> #define _PAGE_AP_BIT 5 >>>>>> #define _PAGE_PRESENT_BIT 7 >>>>>> +#define _PAGE_TRANSIENT_BIT 8 >>>>> I don't think this is related to MPU. At least when I look at the >>>>> bit representation of PRBAR_EL1/2, >>>> >>>> This set of _PAGE_xxx flags aren't compliant with PRBAR_EL1/2 >>>> register map. >>>> It is a flag passed to function map_pages_to_xen() to indicate memory >>>> attributes and permission. >>> >>> But aren't you writing these flags to PRBAR_EL1/EL2 when you call >>> xen_mpumap_update_entry(). >>> >>> In the below snippet of xen_mpumap_update_entry(), IIUC, you are >>> writing these flags. >>> >>> xen_mpumap[idx].prbar.reg.ap = PAGE_AP_MASK(flags); >>> xen_mpumap[idx].prbar.reg.xn = PAGE_XN_MASK(flags); >>> >>> write_protection_region((const pr_t*)(&xen_mpumap[idx]), idx); >>> >>> Please clarify here. >>> >>> In this case, I don't prefer mixing hardware specific bits with >>> software only representation for these reasons :- >>> >>> 1. It makes it confusing and hard to differentiate the hardware >>> specific attrbutes from software only. >> >> Penny's approach matches what we are doing in the MMU code. We want to >> have a way for the caller to pass just set of flags and let the callee >> to decide what to do with them. >> >> This may be flags converted for HW fields or just used by the logic. >> >> If you disagree with this approach, then can you propose a different >> way that we can discuss? > > In MMU, I could see "struct lpae_pt_t{ .avail }" is used for reference > count. > > Something like this might look better (where the hardware specific bits > are not reused for sw logic) > > struct { > > struct __packed { > > ... /* the existing bits as they are */ > > } lpae_pt_t; > > unsigned int ref_count; > > } p2m_entry > > And use "p2m_entry.ref_count" for the reference counting. So where would you store 'ref_count'? In the existing approach, this is store in the PTE and that's fine because the bits 58:55 are reserved for software use. This is right in the middle of the PTE. So we have no other choice other than describing right in the middle of lpae_pt_t. Cheers,
Hi, On 2023/6/30 23:02, Julien Grall wrote: > Hi, > > On 30/06/2023 15:42, Ayan Kumar Halder wrote: >> Hi Julien, >> >> On 30/06/2023 12:22, Julien Grall wrote: >>> >>> >>> On 30/06/2023 11:49, Ayan Kumar Halder wrote: >>>> >>>> On 30/06/2023 05:07, Penny Zheng wrote: >>>>> Hi, >>>> Hi Penny, >>>>> >>>>> >>>>> On 2023/6/30 01:22, Ayan Kumar Halder wrote: >>>>>> >>>>>> On 26/06/2023 04:34, Penny Zheng wrote: >>>>>>> CAUTION: This message has originated from an External Source. >>>>>>> Please use proper judgment and caution when opening attachments, >>>>>>> clicking links, or responding to this email. >>>>>>> >>>>>>> >>>>>>> In MPU system, MPU memory region is always mapped PAGE_ALIGN, so >>>>>>> in order to >>>>>>> not access unexpected memory area, dtb section in xen.lds.S >>>>>>> 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, as >>>>>>> it will be relocated into heap and unmapped at the end of boot. >>>>>>> >>>>>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >>>>>>> Signed-off-by: Wei Chen <wei.chen@arm.com> >>>>>>> --- >>>>>>> v3: >>>>>>> - map the first 2MB. Check the size and then re-map with an extra >>>>>>> 2MB if needed >>>>>>> --- >>>>>>> xen/arch/arm/include/asm/arm64/mpu.h | 3 ++- >>>>>>> xen/arch/arm/include/asm/page.h | 5 +++++ >>>>>>> xen/arch/arm/mm.c | 26 >>>>>>> ++++++++++++++++++++------ >>>>>>> xen/arch/arm/mpu/mm.c | 1 + >>>>>>> xen/arch/arm/xen.lds.S | 5 ++++- >>>>>>> 5 files changed, 32 insertions(+), 8 deletions(-) >>>>>>> >>>>>>> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h >>>>>>> b/xen/arch/arm/include/asm/arm64/mpu.h >>>>>>> index a6b07bab02..715ea69884 100644 >>>>>>> --- a/xen/arch/arm/include/asm/arm64/mpu.h >>>>>>> +++ b/xen/arch/arm/include/asm/arm64/mpu.h >>>>>>> @@ -72,7 +72,8 @@ typedef union { >>>>>>> unsigned long ns:1; /* Not-Secure */ >>>>>>> unsigned long res:1; /* Reserved 0 by hardware */ >>>>>>> unsigned long limit:42; /* Limit Address */ >>>>>>> - unsigned long pad:16; >>>>>>> + unsigned long pad:15; >>>>>>> + unsigned long tran:1; /* Transient region */ >>>>>>> } reg; >>>>>>> uint64_t bits; >>>>>>> } prlar_t; >>>>>>> diff --git a/xen/arch/arm/include/asm/page.h >>>>>>> b/xen/arch/arm/include/asm/page.h >>>>>>> index 85ecd5e4de..a434e2205a 100644 >>>>>>> --- a/xen/arch/arm/include/asm/page.h >>>>>>> +++ b/xen/arch/arm/include/asm/page.h >>>>>>> @@ -97,19 +97,24 @@ >>>>>>> * [3:4] Execute Never >>>>>>> * [5:6] Access Permission >>>>>>> * [7] Region Present >>>>>>> + * [8] Transient Region, e.g. MPU memory region is temproraily >>>>>>> + * mapped for a short time >>>>>>> */ >>>>>>> #define _PAGE_AI_BIT 0 >>>>>>> #define _PAGE_XN_BIT 3 >>>>>>> #define _PAGE_AP_BIT 5 >>>>>>> #define _PAGE_PRESENT_BIT 7 >>>>>>> +#define _PAGE_TRANSIENT_BIT 8 >>>>>> I don't think this is related to MPU. At least when I look at the >>>>>> bit representation of PRBAR_EL1/2, >>>>> >>>>> This set of _PAGE_xxx flags aren't compliant with PRBAR_EL1/2 >>>>> register map. >>>>> It is a flag passed to function map_pages_to_xen() to indicate memory >>>>> attributes and permission. >>>> >>>> But aren't you writing these flags to PRBAR_EL1/EL2 when you call >>>> xen_mpumap_update_entry(). >>>> >>>> In the below snippet of xen_mpumap_update_entry(), IIUC, you are >>>> writing these flags. >>>> >>>> xen_mpumap[idx].prbar.reg.ap = PAGE_AP_MASK(flags); >>>> xen_mpumap[idx].prbar.reg.xn = PAGE_XN_MASK(flags); >>>> >>>> write_protection_region((const pr_t*)(&xen_mpumap[idx]), idx); >>>> >>>> Please clarify here. >>>> >>>> In this case, I don't prefer mixing hardware specific bits with >>>> software only representation for these reasons :- >>>> >>>> 1. It makes it confusing and hard to differentiate the hardware >>>> specific attrbutes from software only. >>> >>> Penny's approach matches what we are doing in the MMU code. We want >>> to have a way for the caller to pass just set of flags and let the >>> callee to decide what to do with them. >>> >>> This may be flags converted for HW fields or just used by the logic. >>> >>> If you disagree with this approach, then can you propose a different >>> way that we can discuss? Thanks ayan for pointing out that RES0 is not suitable for storing software-only flags, agreed. Then, maybe we should refine the existing "struct pr_t" to store these sw bits, like: ``` typedef union { struct { uint8_t tran:1; /* Transient region */ uint8_t p2m_type:4; /* Used to store p2m types.*/ }; uint8_t bits; } pr_flags; /* MPU Protection Region */ typedef struct { prbar_t prbar; prlar_t prlar; pr_flags flags; } pr_t; ``` The drawback is that, considering the padding, "struct pr_t" expands from 16 bytes to 24 bytes. Wdyt? >> >> In MMU, I could see "struct lpae_pt_t{ .avail }" is used for reference >> count. >> >> Something like this might look better (where the hardware specific >> bits are not reused for sw logic) >> >> struct { >> >> struct __packed { >> >> ... /* the existing bits as they are */ >> >> } lpae_pt_t; >> >> unsigned int ref_count; >> >> } p2m_entry >> > And use "p2m_entry.ref_count" for the reference counting. > > So where would you store 'ref_count'? In the existing approach, this is > store in the PTE and that's fine because the bits 58:55 are reserved for > software use. > > This is right in the middle of the PTE. So we have no other choice other > than describing right in the middle of lpae_pt_t. > > Cheers, >
Hi, On 03/07/2023 06:12, Penny Zheng wrote: > Hi, > > > On 2023/6/30 23:02, Julien Grall wrote: >> Hi, >> >> On 30/06/2023 15:42, Ayan Kumar Halder wrote: >>> Hi Julien, >>> >>> On 30/06/2023 12:22, Julien Grall wrote: >>>> >>>> >>>> On 30/06/2023 11:49, Ayan Kumar Halder wrote: >>>>> >>>>> On 30/06/2023 05:07, Penny Zheng wrote: >>>>>> Hi, >>>>> Hi Penny, >>>>>> >>>>>> >>>>>> On 2023/6/30 01:22, Ayan Kumar Halder wrote: >>>>>>> >>>>>>> On 26/06/2023 04:34, Penny Zheng wrote: >>>>>>>> CAUTION: This message has originated from an External Source. >>>>>>>> Please use proper judgment and caution when opening attachments, >>>>>>>> clicking links, or responding to this email. >>>>>>>> >>>>>>>> >>>>>>>> In MPU system, MPU memory region is always mapped PAGE_ALIGN, so >>>>>>>> in order to >>>>>>>> not access unexpected memory area, dtb section in xen.lds.S >>>>>>>> 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, as >>>>>>>> it will be relocated into heap and unmapped at the end of boot. >>>>>>>> >>>>>>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >>>>>>>> Signed-off-by: Wei Chen <wei.chen@arm.com> >>>>>>>> --- >>>>>>>> v3: >>>>>>>> - map the first 2MB. Check the size and then re-map with an >>>>>>>> extra 2MB if needed >>>>>>>> --- >>>>>>>> xen/arch/arm/include/asm/arm64/mpu.h | 3 ++- >>>>>>>> xen/arch/arm/include/asm/page.h | 5 +++++ >>>>>>>> xen/arch/arm/mm.c | 26 >>>>>>>> ++++++++++++++++++++------ >>>>>>>> xen/arch/arm/mpu/mm.c | 1 + >>>>>>>> xen/arch/arm/xen.lds.S | 5 ++++- >>>>>>>> 5 files changed, 32 insertions(+), 8 deletions(-) >>>>>>>> >>>>>>>> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h >>>>>>>> b/xen/arch/arm/include/asm/arm64/mpu.h >>>>>>>> index a6b07bab02..715ea69884 100644 >>>>>>>> --- a/xen/arch/arm/include/asm/arm64/mpu.h >>>>>>>> +++ b/xen/arch/arm/include/asm/arm64/mpu.h >>>>>>>> @@ -72,7 +72,8 @@ typedef union { >>>>>>>> unsigned long ns:1; /* Not-Secure */ >>>>>>>> unsigned long res:1; /* Reserved 0 by hardware */ >>>>>>>> unsigned long limit:42; /* Limit Address */ >>>>>>>> - unsigned long pad:16; >>>>>>>> + unsigned long pad:15; >>>>>>>> + unsigned long tran:1; /* Transient region */ >>>>>>>> } reg; >>>>>>>> uint64_t bits; >>>>>>>> } prlar_t; >>>>>>>> diff --git a/xen/arch/arm/include/asm/page.h >>>>>>>> b/xen/arch/arm/include/asm/page.h >>>>>>>> index 85ecd5e4de..a434e2205a 100644 >>>>>>>> --- a/xen/arch/arm/include/asm/page.h >>>>>>>> +++ b/xen/arch/arm/include/asm/page.h >>>>>>>> @@ -97,19 +97,24 @@ >>>>>>>> * [3:4] Execute Never >>>>>>>> * [5:6] Access Permission >>>>>>>> * [7] Region Present >>>>>>>> + * [8] Transient Region, e.g. MPU memory region is temproraily >>>>>>>> + * mapped for a short time >>>>>>>> */ >>>>>>>> #define _PAGE_AI_BIT 0 >>>>>>>> #define _PAGE_XN_BIT 3 >>>>>>>> #define _PAGE_AP_BIT 5 >>>>>>>> #define _PAGE_PRESENT_BIT 7 >>>>>>>> +#define _PAGE_TRANSIENT_BIT 8 >>>>>>> I don't think this is related to MPU. At least when I look at the >>>>>>> bit representation of PRBAR_EL1/2, >>>>>> >>>>>> This set of _PAGE_xxx flags aren't compliant with PRBAR_EL1/2 >>>>>> register map. >>>>>> It is a flag passed to function map_pages_to_xen() to indicate memory >>>>>> attributes and permission. >>>>> >>>>> But aren't you writing these flags to PRBAR_EL1/EL2 when you call >>>>> xen_mpumap_update_entry(). >>>>> >>>>> In the below snippet of xen_mpumap_update_entry(), IIUC, you are >>>>> writing these flags. >>>>> >>>>> xen_mpumap[idx].prbar.reg.ap = PAGE_AP_MASK(flags); >>>>> xen_mpumap[idx].prbar.reg.xn = PAGE_XN_MASK(flags); >>>>> >>>>> write_protection_region((const pr_t*)(&xen_mpumap[idx]), >>>>> idx); >>>>> >>>>> Please clarify here. >>>>> >>>>> In this case, I don't prefer mixing hardware specific bits with >>>>> software only representation for these reasons :- >>>>> >>>>> 1. It makes it confusing and hard to differentiate the hardware >>>>> specific attrbutes from software only. >>>> >>>> Penny's approach matches what we are doing in the MMU code. We want >>>> to have a way for the caller to pass just set of flags and let the >>>> callee to decide what to do with them. >>>> >>>> This may be flags converted for HW fields or just used by the logic. >>>> >>>> If you disagree with this approach, then can you propose a different >>>> way that we can discuss? > > Thanks ayan for pointing out that RES0 is not suitable for storing > software-only flags, agreed. > > Then, maybe we should refine the existing "struct pr_t" to store these > sw bits, like: > ``` > typedef union { > struct { > uint8_t tran:1; /* Transient region */ > uint8_t p2m_type:4; /* Used to store p2m types.*/ Why do you need the p2m_type? > }; > uint8_t bits; > } pr_flags; > > /* MPU Protection Region */ > typedef struct { > prbar_t prbar; > prlar_t prlar; > pr_flags flags; > } pr_t; > ``` > The drawback is that, considering the padding, "struct pr_t" expands > from 16 bytes to 24 bytes. For clarifications, pr_t is going to be used to create an array in Xen, right? If so, what's the expected size of the array? Cheers,
Hi Julien On 2023/7/3 17:20, Julien Grall wrote: > Hi, > > On 03/07/2023 06:12, Penny Zheng wrote: >> Hi, >> >> >> On 2023/6/30 23:02, Julien Grall wrote: >>> Hi, >>> >>> On 30/06/2023 15:42, Ayan Kumar Halder wrote: >>>> Hi Julien, >>>> >>>> On 30/06/2023 12:22, Julien Grall wrote: >>>>> >>>>> >>>>> On 30/06/2023 11:49, Ayan Kumar Halder wrote: >>>>>> >>>>>> On 30/06/2023 05:07, Penny Zheng wrote: >>>>>>> Hi, >>>>>> Hi Penny, >>>>>>> >>>>>>> >>>>>>> On 2023/6/30 01:22, Ayan Kumar Halder wrote: >>>>>>>> >>>>>>>> On 26/06/2023 04:34, Penny Zheng wrote: >>>>>>>>> CAUTION: This message has originated from an External Source. >>>>>>>>> Please use proper judgment and caution when opening >>>>>>>>> attachments, clicking links, or responding to this email. >>>>>>>>> >>>>>>>>> >>>>>>>>> In MPU system, MPU memory region is always mapped PAGE_ALIGN, >>>>>>>>> so in order to >>>>>>>>> not access unexpected memory area, dtb section in xen.lds.S >>>>>>>>> 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, as >>>>>>>>> it will be relocated into heap and unmapped at the end of boot. >>>>>>>>> >>>>>>>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >>>>>>>>> Signed-off-by: Wei Chen <wei.chen@arm.com> >>>>>>>>> --- >>>>>>>>> v3: >>>>>>>>> - map the first 2MB. Check the size and then re-map with an >>>>>>>>> extra 2MB if needed >>>>>>>>> --- >>>>>>>>> xen/arch/arm/include/asm/arm64/mpu.h | 3 ++- >>>>>>>>> xen/arch/arm/include/asm/page.h | 5 +++++ >>>>>>>>> xen/arch/arm/mm.c | 26 >>>>>>>>> ++++++++++++++++++++------ >>>>>>>>> xen/arch/arm/mpu/mm.c | 1 + >>>>>>>>> xen/arch/arm/xen.lds.S | 5 ++++- >>>>>>>>> 5 files changed, 32 insertions(+), 8 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h >>>>>>>>> b/xen/arch/arm/include/asm/arm64/mpu.h >>>>>>>>> index a6b07bab02..715ea69884 100644 >>>>>>>>> --- a/xen/arch/arm/include/asm/arm64/mpu.h >>>>>>>>> +++ b/xen/arch/arm/include/asm/arm64/mpu.h >>>>>>>>> @@ -72,7 +72,8 @@ typedef union { >>>>>>>>> unsigned long ns:1; /* Not-Secure */ >>>>>>>>> unsigned long res:1; /* Reserved 0 by hardware */ >>>>>>>>> unsigned long limit:42; /* Limit Address */ >>>>>>>>> - unsigned long pad:16; >>>>>>>>> + unsigned long pad:15; >>>>>>>>> + unsigned long tran:1; /* Transient region */ >>>>>>>>> } reg; >>>>>>>>> uint64_t bits; >>>>>>>>> } prlar_t; >>>>>>>>> diff --git a/xen/arch/arm/include/asm/page.h >>>>>>>>> b/xen/arch/arm/include/asm/page.h >>>>>>>>> index 85ecd5e4de..a434e2205a 100644 >>>>>>>>> --- a/xen/arch/arm/include/asm/page.h >>>>>>>>> +++ b/xen/arch/arm/include/asm/page.h >>>>>>>>> @@ -97,19 +97,24 @@ >>>>>>>>> * [3:4] Execute Never >>>>>>>>> * [5:6] Access Permission >>>>>>>>> * [7] Region Present >>>>>>>>> + * [8] Transient Region, e.g. MPU memory region is temproraily >>>>>>>>> + * mapped for a short time >>>>>>>>> */ >>>>>>>>> #define _PAGE_AI_BIT 0 >>>>>>>>> #define _PAGE_XN_BIT 3 >>>>>>>>> #define _PAGE_AP_BIT 5 >>>>>>>>> #define _PAGE_PRESENT_BIT 7 >>>>>>>>> +#define _PAGE_TRANSIENT_BIT 8 >>>>>>>> I don't think this is related to MPU. At least when I look at >>>>>>>> the bit representation of PRBAR_EL1/2, >>>>>>> >>>>>>> This set of _PAGE_xxx flags aren't compliant with PRBAR_EL1/2 >>>>>>> register map. >>>>>>> It is a flag passed to function map_pages_to_xen() to indicate >>>>>>> memory >>>>>>> attributes and permission. >>>>>> >>>>>> But aren't you writing these flags to PRBAR_EL1/EL2 when you call >>>>>> xen_mpumap_update_entry(). >>>>>> >>>>>> In the below snippet of xen_mpumap_update_entry(), IIUC, you are >>>>>> writing these flags. >>>>>> >>>>>> xen_mpumap[idx].prbar.reg.ap = PAGE_AP_MASK(flags); >>>>>> xen_mpumap[idx].prbar.reg.xn = PAGE_XN_MASK(flags); >>>>>> >>>>>> write_protection_region((const pr_t*)(&xen_mpumap[idx]), >>>>>> idx); >>>>>> >>>>>> Please clarify here. >>>>>> >>>>>> In this case, I don't prefer mixing hardware specific bits with >>>>>> software only representation for these reasons :- >>>>>> >>>>>> 1. It makes it confusing and hard to differentiate the hardware >>>>>> specific attrbutes from software only. >>>>> >>>>> Penny's approach matches what we are doing in the MMU code. We want >>>>> to have a way for the caller to pass just set of flags and let the >>>>> callee to decide what to do with them. >>>>> >>>>> This may be flags converted for HW fields or just used by the logic. >>>>> >>>>> If you disagree with this approach, then can you propose a >>>>> different way that we can discuss? >> >> Thanks ayan for pointing out that RES0 is not suitable for storing >> software-only flags, agreed. >> >> Then, maybe we should refine the existing "struct pr_t" to store these >> sw bits, like: >> ``` >> typedef union { >> struct { >> uint8_t tran:1; /* Transient region */ >> uint8_t p2m_type:4; /* Used to store p2m types.*/ > > Why do you need the p2m_type? > I inherited the usage from MMU. Right now, in commit "[PATCH v3 46/52] xen/mpu: look up entry in p2m table", we introduce the first usage to tell whether it is a valid P2M MPU memory region. In the future, we may also use it to check whether it works as RAM region(p2m_ram_rw). >> }; >> uint8_t bits; >> } pr_flags; >> >> /* MPU Protection Region */ >> typedef struct { >> prbar_t prbar; >> prlar_t prlar; >> pr_flags flags; >> } pr_t; >> ``` >> The drawback is that, considering the padding, "struct pr_t" expands >> from 16 bytes to 24 bytes. > > For clarifications, pr_t is going to be used to create an array in Xen, > right? If so, what's the expected size of the array? > Yes, it is going to be an array. And the maximum length is 255. MPUIR_EL2 identifies the number of regions supported by the EL2 MPU, which is 8-bits wide. The original 16 bytes, even with 255 regions at most, will take up less than 4KB. One page is enough. The following definition could have covered all scenarios. ``` /* EL2 Xen MPU memory region mapping table. */ pr_t __aligned(PAGE_SIZE) __section(".data.page_aligned") xen_mpumap[ARM_MAX_MPU_MEMORY_REGIONS]; ``` Now, FWIT, with 24 bytes, the static xen_mpumap definition only works before the heap allocator are up. When heap is up, we may need to re-allocate xen_mpumap from heap. When MPUIR_EL2.Region is more than 170, an extra page is needed. > Cheers, >
On 13/07/2023 04:12, Penny Zheng wrote: >>> >>> Thanks ayan for pointing out that RES0 is not suitable for storing >>> software-only flags, agreed. >>> >>> Then, maybe we should refine the existing "struct pr_t" to store these >>> sw bits, like: >>> ``` >>> typedef union { >>> struct { >>> uint8_t tran:1; /* Transient region */ >>> uint8_t p2m_type:4; /* Used to store p2m types.*/ >> >> Why do you need the p2m_type? >> > > I inherited the usage from MMU. Right now, in commit "[PATCH v3 46/52] > xen/mpu: look up entry in p2m table", we introduce the first usage to > tell whether it is a valid P2M MPU memory region. In the future, > we may also use it to check whether it works as RAM region(p2m_ram_rw). I find a bit odd to use p2m_type to decide whether this is an MPU memory region describing guest information. It would make more sense to introduce a boolean 'guest' for now. We can add the type if necessary. > >>> }; >>> uint8_t bits; >>> } pr_flags; >>> >>> /* MPU Protection Region */ >>> typedef struct { >>> prbar_t prbar; >>> prlar_t prlar; >>> pr_flags flags; >>> } pr_t; >>> ``` >>> The drawback is that, considering the padding, "struct pr_t" expands >>> from 16 bytes to 24 bytes. >> >> For clarifications, pr_t is going to be used to create an array in >> Xen, right? If so, what's the expected size of the array? >> > > Yes, it is going to be an array. And the maximum length is 255. > MPUIR_EL2 identifies the number of regions supported by the EL2 MPU, > which is 8-bits wide. > The original 16 bytes, even with 255 regions at most, will take up > less than 4KB. One page is enough. The following definition could have > covered all scenarios. > ``` > /* EL2 Xen MPU memory region mapping table. */ > pr_t __aligned(PAGE_SIZE) __section(".data.page_aligned") > xen_mpumap[ARM_MAX_MPU_MEMORY_REGIONS]; > ``` Can you explain why you want the array to be page-aligned? Cheers,
diff --git a/xen/arch/arm/include/asm/arm64/mpu.h b/xen/arch/arm/include/asm/arm64/mpu.h index a6b07bab02..715ea69884 100644 --- a/xen/arch/arm/include/asm/arm64/mpu.h +++ b/xen/arch/arm/include/asm/arm64/mpu.h @@ -72,7 +72,8 @@ typedef union { unsigned long ns:1; /* Not-Secure */ unsigned long res:1; /* Reserved 0 by hardware */ unsigned long limit:42; /* Limit Address */ - unsigned long pad:16; + unsigned long pad:15; + unsigned long tran:1; /* Transient region */ } reg; uint64_t bits; } prlar_t; diff --git a/xen/arch/arm/include/asm/page.h b/xen/arch/arm/include/asm/page.h index 85ecd5e4de..a434e2205a 100644 --- a/xen/arch/arm/include/asm/page.h +++ b/xen/arch/arm/include/asm/page.h @@ -97,19 +97,24 @@ * [3:4] Execute Never * [5:6] Access Permission * [7] Region Present + * [8] Transient Region, e.g. MPU memory region is temproraily + * mapped for a short time */ #define _PAGE_AI_BIT 0 #define _PAGE_XN_BIT 3 #define _PAGE_AP_BIT 5 #define _PAGE_PRESENT_BIT 7 +#define _PAGE_TRANSIENT_BIT 8 #define _PAGE_AI (7U << _PAGE_AI_BIT) #define _PAGE_XN (2U << _PAGE_XN_BIT) #define _PAGE_RO (2U << _PAGE_AP_BIT) #define _PAGE_PRESENT (1U << _PAGE_PRESENT_BIT) +#define _PAGE_TRANSIENT (1U << _PAGE_TRANSIENT_BIT) #define PAGE_AI_MASK(x) (((x) >> _PAGE_AI_BIT) & 0x7U) #define PAGE_XN_MASK(x) (((x) >> _PAGE_XN_BIT) & 0x3U) #define PAGE_AP_MASK(x) (((x) >> _PAGE_AP_BIT) & 0x3U) #define PAGE_RO_MASK(x) (((x) >> _PAGE_AP_BIT) & 0x2U) +#define PAGE_TRANSIENT_MASK(x) (((x) >> _PAGE_TRANSIENT_BIT) & 0x1U) #endif /* CONFIG_HAS_MPU */ /* diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index d35e7e280f..8625066256 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -61,8 +61,17 @@ void flush_page_to_ram(unsigned long mfn, bool sync_icache) void * __init early_fdt_map(paddr_t fdt_paddr) { +#ifndef CONFIG_HAS_MPU /* We are using 2MB superpage for mapping the FDT */ paddr_t base_paddr = fdt_paddr & SECOND_MASK; + unsigned int flags = PAGE_HYPERVISOR_RO | _PAGE_BLOCK; + unsigned long base_virt = BOOT_FDT_VIRT_START; +#else + /* MPU region must be PAGE aligned */ + paddr_t base_paddr = fdt_paddr & PAGE_MASK; + unsigned int flags = PAGE_HYPERVISOR_RO | _PAGE_TRANSIENT; + unsigned long base_virt = ~0UL; +#endif paddr_t offset; void *fdt_virt; uint32_t size; @@ -79,18 +88,24 @@ void * __init early_fdt_map(paddr_t fdt_paddr) if ( !fdt_paddr || fdt_paddr % MIN_FDT_ALIGN ) return NULL; +#ifndef CONFIG_HAS_MPU /* The FDT is mapped using 2MB superpage */ BUILD_BUG_ON(BOOT_FDT_VIRT_START % SZ_2M); +#endif - rc = map_pages_to_xen(BOOT_FDT_VIRT_START, maddr_to_mfn(base_paddr), - SZ_2M >> PAGE_SHIFT, - PAGE_HYPERVISOR_RO | _PAGE_BLOCK); + rc = map_pages_to_xen(base_virt, maddr_to_mfn(base_paddr), + SZ_2M >> PAGE_SHIFT, flags); if ( rc ) panic("Unable to map the device-tree.\n"); +#ifndef CONFIG_HAS_MPU offset = fdt_paddr % SECOND_SIZE; fdt_virt = (void *)BOOT_FDT_VIRT_START + offset; +#else + offset = fdt_paddr % PAGE_SIZE; + fdt_virt = (void *)fdt_paddr; +#endif if ( fdt_magic(fdt_virt) != FDT_MAGIC ) return NULL; @@ -101,10 +116,9 @@ void * __init early_fdt_map(paddr_t fdt_paddr) if ( (offset + size) > SZ_2M ) { - rc = map_pages_to_xen(BOOT_FDT_VIRT_START + SZ_2M, + rc = map_pages_to_xen(base_virt + SZ_2M, maddr_to_mfn(base_paddr + SZ_2M), - SZ_2M >> PAGE_SHIFT, - PAGE_HYPERVISOR_RO | _PAGE_BLOCK); + SZ_2M >> PAGE_SHIFT, flags); if ( rc ) panic("Unable to map the device-tree\n"); } diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c index 14a1309ca1..f4ce19d36a 100644 --- a/xen/arch/arm/mpu/mm.c +++ b/xen/arch/arm/mpu/mm.c @@ -448,6 +448,7 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit, /* Set permission */ xen_mpumap[idx].prbar.reg.ap = PAGE_AP_MASK(flags); xen_mpumap[idx].prbar.reg.xn = PAGE_XN_MASK(flags); + xen_mpumap[idx].prlar.reg.tran = PAGE_TRANSIENT_MASK(flags); write_protection_region((const pr_t*)(&xen_mpumap[idx]), idx); } diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index 4f7daa7dca..f2715d7cb7 100644 --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -216,7 +216,10 @@ SECTIONS _end = . ; /* Section for the device tree blob (if any). */ - .dtb : { *(.dtb) } :text + .dtb : { + . = ALIGN(PAGE_SIZE); + *(.dtb) + } :text DWARF2_DEBUG_SECTIONS