Re: [PATCH] xen/arm: Harden setup_frametable_mappings

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxx>
> ---
>   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,

--
Julien Grall