Re: [PATCH] xen/arm: Harden setup_frametable_mappings

[Julien Grall <julien@xxxxxxx>]

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

--
Julien Grall