| Message ID | 20230116144106.12544-1-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Harden setup_frametable_mappings | expand |
Hi Michal, > -----Original Message----- > Subject: [PATCH] xen/arm: Harden setup_frametable_mappings > > The amount of supported physical memory depends on the frametable size > and the number of struct page_info entries that can fit into it. Define > a macro PAGE_INFO_SIZE to store the current size of the struct page_info > (i.e. 56B for arm64 and 32B for arm32) and add a sanity check in > setup_frametable_mappings to be notified whenever the size of the > structure changes. Also call a panic if the calculated frametable_size > exceeds the limit defined by FRAMETABLE_SIZE macro. > > Update the comments regarding the frametable in asm/config.h and take > the opportunity to remove unused macro FRAMETABLE_VIRT_END on arm32. > > Signed-off-by: Michal Orzel <michal.orzel@amd.com> Reviewed-by: Henry Wang <Henry.Wang@arm.com> I've also used XTP to test this patch on FVP in both arm32 and arm64 execution mode, and this patch is good, so: Tested-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry
Hi Michal, On 16/01/2023 14:41, Michal Orzel wrote: > The amount of supported physical memory depends on the frametable size > and the number of struct page_info entries that can fit into it. Define > a macro PAGE_INFO_SIZE to store the current size of the struct page_info > (i.e. 56B for arm64 and 32B for arm32) and add a sanity check in > setup_frametable_mappings to be notified whenever the size of the > structure changes. Also call a panic if the calculated frametable_size > exceeds the limit defined by FRAMETABLE_SIZE macro. > > Update the comments regarding the frametable in asm/config.h and take > the opportunity to remove unused macro FRAMETABLE_VIRT_END on arm32. > > Signed-off-by: Michal Orzel <michal.orzel@amd.com> > --- > xen/arch/arm/include/asm/config.h | 5 ++--- > xen/arch/arm/include/asm/mm.h | 11 +++++++++++ > xen/arch/arm/mm.c | 5 +++++ > 3 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h > index 16213c8b677f..d8f99776986f 100644 > --- a/xen/arch/arm/include/asm/config.h > +++ b/xen/arch/arm/include/asm/config.h > @@ -82,7 +82,7 @@ > * ARM32 layout: > * 0 - 12M <COMMON> > * > - * 32M - 128M Frametable: 24 bytes per page for 16GB of RAM > + * 32M - 128M Frametable: 32 bytes per page for 12GB of RAM > * 256M - 1G VMAP: ioremap and early_ioremap use this virtual address > * space > * > @@ -95,7 +95,7 @@ > * > * 1G - 2G VMAP: ioremap and early_ioremap > * > - * 32G - 64G Frametable: 24 bytes per page for 5.3TB of RAM > + * 32G - 64G Frametable: 56 bytes per page for 2TB of RAM > * > * 0x0000008000000000 - 0x00007fffffffffff (127.5TB, L0 slots [1..255]) > * Unused > @@ -127,7 +127,6 @@ > #define FRAMETABLE_VIRT_START _AT(vaddr_t,0x02000000) > #define FRAMETABLE_SIZE MB(128-32) > #define FRAMETABLE_NR (FRAMETABLE_SIZE / sizeof(*frame_table)) > -#define FRAMETABLE_VIRT_END (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1) This is somewhat unrelated to the goal of the patch. We do clean-up in the same patch, but they tend to be in the same of already modified hunk (which is not the case here). So I would prefer if this is split. This would make this patch a potential candidate for backport. > > #define VMAP_VIRT_START _AT(vaddr_t,0x10000000) > #define VMAP_VIRT_SIZE _AT(vaddr_t, GB(1) - MB(256)) > diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h > index 68adcac9fa8d..23dec574eb31 100644 > --- a/xen/arch/arm/include/asm/mm.h > +++ b/xen/arch/arm/include/asm/mm.h > @@ -26,6 +26,17 @@ > */ > #define PFN_ORDER(_pfn) ((_pfn)->v.free.order) > > +/* > + * The size of struct page_info impacts the number of entries that can fit > + * into the frametable area and thus it affects the amount of physical memory > + * we claim to support. Define PAGE_INFO_SIZE to be used for sanity checking. > +*/ > +#ifdef CONFIG_ARM_64 > +#define PAGE_INFO_SIZE 56 > +#else > +#define PAGE_INFO_SIZE 32 > +#endif > + > struct page_info > { > /* Each frame can be threaded onto a doubly-linked list. */ > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index 0fc6f2992dd1..a8c28fa5b768 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -676,6 +676,11 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe) > const unsigned long mapping_size = frametable_size < MB(32) ? MB(2) : MB(32); > int rc; > > + BUILD_BUG_ON(sizeof(struct page_info) != PAGE_INFO_SIZE); > + > + if ( frametable_size > FRAMETABLE_SIZE ) > + panic("RAM size is too big to fit in a frametable area\n"); This is not correct. Depending on the PDX compression, the frametable may end up to cover non-RAM. So I would write: "The cannot frametable cannot cover the physical region 0x%PRIpaddr - 0x%PRIpaddr". > + > frametable_base_pdx = mfn_to_pdx(maddr_to_mfn(ps)); > /* Round up to 2M or 32M boundary, as appropriate. */ > frametable_size = ROUNDUP(frametable_size, mapping_size); Cheers,
Hi Julien, On 17/01/2023 10:29, Julien Grall wrote: > > > Hi Michal, > > On 16/01/2023 14:41, Michal Orzel wrote: >> The amount of supported physical memory depends on the frametable size >> and the number of struct page_info entries that can fit into it. Define >> a macro PAGE_INFO_SIZE to store the current size of the struct page_info >> (i.e. 56B for arm64 and 32B for arm32) and add a sanity check in >> setup_frametable_mappings to be notified whenever the size of the >> structure changes. Also call a panic if the calculated frametable_size >> exceeds the limit defined by FRAMETABLE_SIZE macro. >> >> Update the comments regarding the frametable in asm/config.h and take >> the opportunity to remove unused macro FRAMETABLE_VIRT_END on arm32. >> >> Signed-off-by: Michal Orzel <michal.orzel@amd.com> >> --- >> xen/arch/arm/include/asm/config.h | 5 ++--- >> xen/arch/arm/include/asm/mm.h | 11 +++++++++++ >> xen/arch/arm/mm.c | 5 +++++ >> 3 files changed, 18 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h >> index 16213c8b677f..d8f99776986f 100644 >> --- a/xen/arch/arm/include/asm/config.h >> +++ b/xen/arch/arm/include/asm/config.h >> @@ -82,7 +82,7 @@ >> * ARM32 layout: >> * 0 - 12M <COMMON> >> * >> - * 32M - 128M Frametable: 24 bytes per page for 16GB of RAM >> + * 32M - 128M Frametable: 32 bytes per page for 12GB of RAM >> * 256M - 1G VMAP: ioremap and early_ioremap use this virtual address >> * space >> * >> @@ -95,7 +95,7 @@ >> * >> * 1G - 2G VMAP: ioremap and early_ioremap >> * >> - * 32G - 64G Frametable: 24 bytes per page for 5.3TB of RAM >> + * 32G - 64G Frametable: 56 bytes per page for 2TB of RAM >> * >> * 0x0000008000000000 - 0x00007fffffffffff (127.5TB, L0 slots [1..255]) >> * Unused >> @@ -127,7 +127,6 @@ >> #define FRAMETABLE_VIRT_START _AT(vaddr_t,0x02000000) >> #define FRAMETABLE_SIZE MB(128-32) >> #define FRAMETABLE_NR (FRAMETABLE_SIZE / sizeof(*frame_table)) >> -#define FRAMETABLE_VIRT_END (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1) > > This is somewhat unrelated to the goal of the patch. We do clean-up in > the same patch, but they tend to be in the same of already modified hunk > (which is not the case here). > > So I would prefer if this is split. This would make this patch a > potential candidate for backport. Just for clarity. Do you mean to separate all the config.h changes or only the FRAMETABLE_VIRT_END removal? I guess the former. > >> >> #define VMAP_VIRT_START _AT(vaddr_t,0x10000000) >> #define VMAP_VIRT_SIZE _AT(vaddr_t, GB(1) - MB(256)) >> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h >> index 68adcac9fa8d..23dec574eb31 100644 >> --- a/xen/arch/arm/include/asm/mm.h >> +++ b/xen/arch/arm/include/asm/mm.h >> @@ -26,6 +26,17 @@ >> */ >> #define PFN_ORDER(_pfn) ((_pfn)->v.free.order) >> >> +/* >> + * The size of struct page_info impacts the number of entries that can fit >> + * into the frametable area and thus it affects the amount of physical memory >> + * we claim to support. Define PAGE_INFO_SIZE to be used for sanity checking. >> +*/ >> +#ifdef CONFIG_ARM_64 >> +#define PAGE_INFO_SIZE 56 >> +#else >> +#define PAGE_INFO_SIZE 32 >> +#endif >> + >> struct page_info >> { >> /* Each frame can be threaded onto a doubly-linked list. */ >> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c >> index 0fc6f2992dd1..a8c28fa5b768 100644 >> --- a/xen/arch/arm/mm.c >> +++ b/xen/arch/arm/mm.c >> @@ -676,6 +676,11 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe) >> const unsigned long mapping_size = frametable_size < MB(32) ? MB(2) : MB(32); >> int rc; >> >> + BUILD_BUG_ON(sizeof(struct page_info) != PAGE_INFO_SIZE); >> + >> + if ( frametable_size > FRAMETABLE_SIZE ) >> + panic("RAM size is too big to fit in a frametable area\n"); > > This is not correct. Depending on the PDX compression, the frametable > may end up to cover non-RAM. So I would write: > > "The cannot frametable cannot cover the physical region 0x%PRIpaddr - > 0x%PRIpaddr". Yes, you're right. > >> + >> frametable_base_pdx = mfn_to_pdx(maddr_to_mfn(ps)); >> /* Round up to 2M or 32M boundary, as appropriate. */ >> frametable_size = ROUNDUP(frametable_size, mapping_size); > > Cheers, > > -- > Julien Grall ~Michal
Hi Michal, On 17/01/2023 09:49, Michal Orzel wrote: > Hi Julien, > > On 17/01/2023 10:29, Julien Grall wrote: >> >> >> Hi Michal, >> >> On 16/01/2023 14:41, Michal Orzel wrote: >>> The amount of supported physical memory depends on the frametable size >>> and the number of struct page_info entries that can fit into it. Define >>> a macro PAGE_INFO_SIZE to store the current size of the struct page_info >>> (i.e. 56B for arm64 and 32B for arm32) and add a sanity check in >>> setup_frametable_mappings to be notified whenever the size of the >>> structure changes. Also call a panic if the calculated frametable_size >>> exceeds the limit defined by FRAMETABLE_SIZE macro. >>> >>> Update the comments regarding the frametable in asm/config.h and take >>> the opportunity to remove unused macro FRAMETABLE_VIRT_END on arm32. >>> >>> Signed-off-by: Michal Orzel <michal.orzel@amd.com> >>> --- >>> xen/arch/arm/include/asm/config.h | 5 ++--- >>> xen/arch/arm/include/asm/mm.h | 11 +++++++++++ >>> xen/arch/arm/mm.c | 5 +++++ >>> 3 files changed, 18 insertions(+), 3 deletions(-) >>> >>> diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h >>> index 16213c8b677f..d8f99776986f 100644 >>> --- a/xen/arch/arm/include/asm/config.h >>> +++ b/xen/arch/arm/include/asm/config.h >>> @@ -82,7 +82,7 @@ >>> * ARM32 layout: >>> * 0 - 12M <COMMON> >>> * >>> - * 32M - 128M Frametable: 24 bytes per page for 16GB of RAM >>> + * 32M - 128M Frametable: 32 bytes per page for 12GB of RAM >>> * 256M - 1G VMAP: ioremap and early_ioremap use this virtual address >>> * space >>> * >>> @@ -95,7 +95,7 @@ >>> * >>> * 1G - 2G VMAP: ioremap and early_ioremap >>> * >>> - * 32G - 64G Frametable: 24 bytes per page for 5.3TB of RAM >>> + * 32G - 64G Frametable: 56 bytes per page for 2TB of RAM >>> * >>> * 0x0000008000000000 - 0x00007fffffffffff (127.5TB, L0 slots [1..255]) >>> * Unused >>> @@ -127,7 +127,6 @@ >>> #define FRAMETABLE_VIRT_START _AT(vaddr_t,0x02000000) >>> #define FRAMETABLE_SIZE MB(128-32) >>> #define FRAMETABLE_NR (FRAMETABLE_SIZE / sizeof(*frame_table)) >>> -#define FRAMETABLE_VIRT_END (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1) >> >> This is somewhat unrelated to the goal of the patch. We do clean-up in >> the same patch, but they tend to be in the same of already modified hunk >> (which is not the case here). >> >> So I would prefer if this is split. This would make this patch a >> potential candidate for backport. > Just for clarity. Do you mean to separate all the config.h changes or only > the FRAMETABLE_VIRT_END removal? I guess the former. The latter. The comment update makes sense here because this is what actually triggered this patch. Cheers,
diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h index 16213c8b677f..d8f99776986f 100644 --- a/xen/arch/arm/include/asm/config.h +++ b/xen/arch/arm/include/asm/config.h @@ -82,7 +82,7 @@ * ARM32 layout: * 0 - 12M <COMMON> * - * 32M - 128M Frametable: 24 bytes per page for 16GB of RAM + * 32M - 128M Frametable: 32 bytes per page for 12GB of RAM * 256M - 1G VMAP: ioremap and early_ioremap use this virtual address * space * @@ -95,7 +95,7 @@ * * 1G - 2G VMAP: ioremap and early_ioremap * - * 32G - 64G Frametable: 24 bytes per page for 5.3TB of RAM + * 32G - 64G Frametable: 56 bytes per page for 2TB of RAM * * 0x0000008000000000 - 0x00007fffffffffff (127.5TB, L0 slots [1..255]) * Unused @@ -127,7 +127,6 @@ #define FRAMETABLE_VIRT_START _AT(vaddr_t,0x02000000) #define FRAMETABLE_SIZE MB(128-32) #define FRAMETABLE_NR (FRAMETABLE_SIZE / sizeof(*frame_table)) -#define FRAMETABLE_VIRT_END (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1) #define VMAP_VIRT_START _AT(vaddr_t,0x10000000) #define VMAP_VIRT_SIZE _AT(vaddr_t, GB(1) - MB(256)) diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h index 68adcac9fa8d..23dec574eb31 100644 --- a/xen/arch/arm/include/asm/mm.h +++ b/xen/arch/arm/include/asm/mm.h @@ -26,6 +26,17 @@ */ #define PFN_ORDER(_pfn) ((_pfn)->v.free.order) +/* + * The size of struct page_info impacts the number of entries that can fit + * into the frametable area and thus it affects the amount of physical memory + * we claim to support. Define PAGE_INFO_SIZE to be used for sanity checking. +*/ +#ifdef CONFIG_ARM_64 +#define PAGE_INFO_SIZE 56 +#else +#define PAGE_INFO_SIZE 32 +#endif + struct page_info { /* Each frame can be threaded onto a doubly-linked list. */ diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 0fc6f2992dd1..a8c28fa5b768 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -676,6 +676,11 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe) const unsigned long mapping_size = frametable_size < MB(32) ? MB(2) : MB(32); int rc; + BUILD_BUG_ON(sizeof(struct page_info) != PAGE_INFO_SIZE); + + if ( frametable_size > FRAMETABLE_SIZE ) + panic("RAM size is too big to fit in a frametable area\n"); + frametable_base_pdx = mfn_to_pdx(maddr_to_mfn(ps)); /* Round up to 2M or 32M boundary, as appropriate. */ frametable_size = ROUNDUP(frametable_size, mapping_size);
The amount of supported physical memory depends on the frametable size and the number of struct page_info entries that can fit into it. Define a macro PAGE_INFO_SIZE to store the current size of the struct page_info (i.e. 56B for arm64 and 32B for arm32) and add a sanity check in setup_frametable_mappings to be notified whenever the size of the structure changes. Also call a panic if the calculated frametable_size exceeds the limit defined by FRAMETABLE_SIZE macro. Update the comments regarding the frametable in asm/config.h and take the opportunity to remove unused macro FRAMETABLE_VIRT_END on arm32. Signed-off-by: Michal Orzel <michal.orzel@amd.com> --- xen/arch/arm/include/asm/config.h | 5 ++--- xen/arch/arm/include/asm/mm.h | 11 +++++++++++ xen/arch/arm/mm.c | 5 +++++ 3 files changed, 18 insertions(+), 3 deletions(-)