Re: [PATCH v3 27/52] xen/mpu: introduce setup_mm_mappings

[Julien Grall <julien@xxxxxxx>]

Hi,

On 29/06/2023 15:05, 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.
> >
> > Function setup_pagetables is responsible for boot-time pagetable setup
> > in MMU system at C world.
> > In MPU system, as we have already built up start-of-day Xen MPU memory
> > region mapping in assembly boot-time, here we only need to do a few
> > memory management data initializtion, including reading the number of
> > maximum MPU regions supported by the EL2 MPU, and setting the according
> > bitfield for regions enabled in assembly boot-time, in bitmap 
> > xen_mpumap_mask.
> > This bitmap xen_mpumap_mask is responsible for recording the usage of 
> > EL2 MPU
> > memory regions.
> >
> > In order to keep only one codeflow in arm/setup.c, setup_mm_mappings
> > , with a more generic name, is introduced to replace setup_pagetables.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > ---
> > v3:
> > - introduce bitmap xen_mpumap_mask for dynamic allocation on MPU regions
> > ---
> >   xen/arch/arm/include/asm/arm64/mpu.h     |  1 +
> >   xen/arch/arm/include/asm/arm64/sysregs.h |  3 +++
> >   xen/arch/arm/include/asm/mm.h            |  4 ++--
> >   xen/arch/arm/mmu/mm.c                    |  7 +++++-
> >   xen/arch/arm/mpu/mm.c                    | 30 ++++++++++++++++++++++++
> >   xen/arch/arm/setup.c                     |  2 +-
> >   6 files changed, 43 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/arch/arm/include/asm/arm64/mpu.h 
> > b/xen/arch/arm/include/asm/arm64/mpu.h
> > index 6ec2c10b14..407fec66c9 100644
> > --- a/xen/arch/arm/include/asm/arm64/mpu.h
> > +++ b/xen/arch/arm/include/asm/arm64/mpu.h
> > @@ -19,6 +19,7 @@
> >    * or it needs adjustment.
> >    */
> >   #define REGION_UART_SEL            0x07
> > +#define MPUIR_REGION_MASK          ((_AC(1, UL) << 8) - 1)
> May be this is simpler to read
>
> #define MPUIR_REGION_MASK _AC(0xFF, UL)
>
> Also, you can move it to xen/arch/arm/include/asm/mpu.h as it is common 
> between R52 and R82.
I would actually prefer if we use GENMASK(...).

[...]

> > diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> > index fb6bb721b1..e06a6e5810 100644
> > --- a/xen/arch/arm/mpu/mm.c
> > +++ b/xen/arch/arm/mpu/mm.c
> > @@ -20,6 +20,7 @@
> >    */
> >
> >   #include <xen/init.h>
> > +#include <xen/mm.h>
> >   #include <xen/page-size.h>
> >   #include <asm/arm64/mpu.h>
> >
> > @@ -27,6 +28,35 @@
> >   pr_t __aligned(PAGE_SIZE) __section(".data.page_aligned")
> >        xen_mpumap[ARM_MAX_MPU_MEMORY_REGIONS];
> >
> > +/* Maximum number of supported MPU memory regions by the EL2 MPU. */
> > +uint8_t __ro_after_init max_xen_mpumap;
> > +
> > +/*
> > + * Bitmap xen_mpumap_mask is to record the usage of EL2 MPU memory 
> > regions.
> > + * Bit 0 represents MPU memory region 0, bit 1 represents MPU memory
> > + * region 1, ..., and so on.
> > + * If a MPU memory region gets enabled, set the according bit to 1.
> > + */
> > +static DECLARE_BITMAP(xen_mpumap_mask, ARM_MAX_MPU_MEMORY_REGIONS);
> > +
> > +void __init setup_mm_mappings(unsigned long boot_phys_offset)
> > +{
> > +    unsigned int nr_regions = REGION_UART_SEL, i = 0;
> > +
> > +    /*
> > +     * MPUIR_EL2.Region[0:7] identifies the number of regions 
> > supported by
> > +     * the EL2 MPU.
> > +     */
> > +    max_xen_mpumap = (uint8_t)(READ_SYSREG(MPUIR_EL2) & 
> > MPUIR_REGION_MASK);
> NIT:- You may dop "& MPUIR_REGION_MASK " as the other bits are RES0
From the Arm Arm (look for the definition of RES0 in the glossary):

"
To preserve forward compatibility, software:
• Must not rely on the bit reading as 0.
• Must use an SBZP policy to write to the bit.
"

So we should not drop "& MPUIR_REGION_MASK".

Cheers,

--
Julien Grall