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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 15/10/2024 17:51, Julien Grall wrote:
> Hi Ayan,
Hi Julien,
> On 15/10/2024 16:56, Ayan Kumar Halder wrote:
> > On 14/10/2024 20:03, Luca Fancellu wrote:
> > > Hi Ayan,
> > Hi Luca,
> > > > 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 */
> > > NIT alignment
> > Ack
> > > > +#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
> > > I think there is an issue adding 1 here, because the very first 
> > > region we are going to fill will be the 1st even if we intended the 
> > > 0th.
> > > Probably moving this one at the end will fix the issue
> > We are also using 'sel' to compare against the maximum number of 
> > regions supported. So, for the first region it needs to be 1 
> > otherwise there is a risk of comparing 0 (ie first region) with 0 
> > (max supported regions).
> > May be what I can do is ...
> >
> > > > +    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
> > > > +
> > /* Regions should start from 0 */
> >
> > sub     \sel, \sel, #1
> I didn't review the full patch yet. But couldn't we use ``bge``? This 
> would cover "maxcount == 0" and avoid to increment and then decrement 
> \sel.
Oh yes. My bad , I missed this.

- Ayan