Re: [PATCH v3 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 18/10/2024 23:13, Julien Grall wrote:
> Hi Ayan,
Hi Julien,

Just one clarification.

> On 10/10/2024 15:03, Ayan Kumar Halder wrote:
> > diff --git a/xen/arch/arm/arm64/mpu/Makefile 
> > b/xen/arch/arm/arm64/mpu/Makefile
> > new file mode 100644
> > index 0000000000..3340058c08
> > --- /dev/null
> > +++ b/xen/arch/arm/arm64/mpu/Makefile
> > @@ -0,0 +1 @@
> > +obj-y += head.o
> > diff --git a/xen/arch/arm/arm64/mpu/head.S 
> > b/xen/arch/arm/arm64/mpu/head.S
> > new file mode 100644
> > index 0000000000..4a21bc815c
> > --- /dev/null
> > +++ b/xen/arch/arm/arm64/mpu/head.S
> > @@ -0,0 +1,130 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Start-of-day code for an Armv8-R MPU system.
> > + */
> > +
> > +#include <asm/mm.h>
> > +#include <asm/arm64/mpu/sysregs.h>
> > +
> > +#define REGION_TEXT_PRBAR       0x38    /* SH=11 AP=10 XN=00 */
> > +#define REGION_RO_PRBAR         0x3A    /* SH=11 AP=10 XN=10 */
> > +#define REGION_DATA_PRBAR       0x32    /* SH=11 AP=00 XN=10 */
> > +
> > +#define REGION_NORMAL_PRLAR     0x0f    /* NS=0 ATTR=111 EN=1 */
> > +
> > +/*
> > + * Macro to prepare and set a EL2 MPU memory region.
> > + * We will also create an according MPU memory region entry, which
> > + * is a structure of pr_t,  in table \prmap.
> > + *
> > + * Inputs:
> > + * sel:         region selector
> > + * base:        reg storing base address (should be page-aligned)
> > + * limit:       reg storing limit address
> > + * prbar:       store computed PRBAR_EL2 value
> > + * prlar:       store computed PRLAR_EL2 value
> > + * maxcount:    maximum number of EL2 regions supported
> > + * attr_prbar:  PRBAR_EL2-related memory attributes. If not 
> > specified it will be
> > + *              REGION_DATA_PRBAR
> > + * attr_prlar:  PRLAR_EL2-related memory attributes. If not 
> > specified it will be
> > + *              REGION_NORMAL_PRLAR
> > + */
> > +.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, 
> > attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR
> > +
> > +    /* Check if the number of regions exceeded the count specified 
> > in MPUIR_EL2 */
> > +    add   \sel, \sel, #1
> > +    cmp   \sel, \maxcount
> > +    bgt   fail
> > +
> > +    /* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/
> > +    and   \base, \base, #MPU_REGION_MASK
> > +    mov   \prbar, #\attr_prbar
> > +    orr   \prbar, \prbar, \base
> > +
> > +    /* Limit address should be inclusive */
> > +    sub   \limit, \limit, #1
> > +    and   \limit, \limit, #MPU_REGION_MASK
> > +    mov   \prlar, #\attr_prlar
> > +    orr   \prlar, \prlar, \limit
> > +
> > +    msr   PRSELR_EL2, \sel
> > +    isb
> > +    msr   PRBAR_EL2, \prbar
> > +    msr   PRLAR_EL2, \prlar
> > +    dsb   sy
> > +    isb
> > +.endm
> > +
> > +/* Load the physical address of a symbol into xb */
> > +.macro load_paddr xb, sym
> > +    ldr \xb, =\sym
> > +    add \xb, \xb, x20       /* x20 - Phys offset */
> Sorry I didn't spot this until now. Xen will be linked to a specific 
> physical address, so why do we need to add an offset?
Yes, this needs to be removed. x20 contains 0.
> > +.endm
> > +
> > +/*
> > + * Maps the various sections of Xen (described in xen.lds.S) as 
> > different MPU
> > + * regions.
> > + *
> > + * Inputs:
> > + *   lr : Address to return to.
> > + *
> > + * Clobbers x0 - x5
> > + *
> > + */
> > +FUNC(enable_boot_cpu_mm)
> > +
> > +    /* Check if the number of regions exceeded the count specified 
> > in MPUIR_EL2 */
> AFAIU, this doesn't match what the instruction is doing below.
Sorry, this needs to be removed.
> > +    mrs   x5, MPUIR_EL2
> > +
> > +    /* x0: region sel */
> > +    mov   x0, xzr
> > +    /* Xen text section. */
> > +    load_paddr x1, _stext
> > +    load_paddr x2, _etext
> > +    cmp x1, x2
> > +    beq 1f
> This check seems to be excessive... I can't think of a reason why 
> there would be no text at all... Same for a lot of the checks below.
Is it ok if we have this excess check ? The downsides are only a small 
increase in code size and boot time. Otherwise, I need to justify why we 
have this checks in some places, but not in others.
> > +    prepare_xen_region x0, x1, x2, x3, x4, x5, 
> > attr_prbar=REGION_TEXT_PRBAR
> > +
> > +1:  /* Xen read-only data section. */
> > +    load_paddr x1, _srodata
> > +    load_paddr x2, _erodata
> > +    cmp x1, x2
> > +    beq 2f
> > +    prepare_xen_region x0, x1, x2, x3, x4, x5, 
> > attr_prbar=REGION_RO_PRBAR
> > +
> > +2:  /* Xen read-only after init and data section. (RW data) */
> > +    load_paddr x1, __ro_after_init_start
> > +    load_paddr x2, __init_begin
> > +    cmp x1, x2
> > +    beq 3f
> > +    prepare_xen_region x0, x1, x2, x3, x4, x5
> > +
> > +3:  /* Xen code section. */
> > +    load_paddr x1, __init_begin
> > +    load_paddr x2, __init_data_begin
> > +    cmp x1, x2
> > +    beq 4f
> > +    prepare_xen_region x0, x1, x2, x3, x4, x5, 
> > attr_prbar=REGION_TEXT_PRBAR
> > +
> > +4:  /* Xen data and BSS section. */
> > +    load_paddr x1, __init_data_begin
> > +    load_paddr x2, __bss_end
> > +    cmp x1, x2
> > +    beq 5f
> > +    prepare_xen_region x0, x1, x2, x3, x4, x5
> > +
> > +5:
> > +    ret
> > +
> > +fail:
> This name is a bit too generic given you use within a macro. Also, I 
> think it should be its own local function because the macro can be 
> used anywhere.
Ack. I will convert this to a function.

- Ayan