Re: [PATCH v2 2/3] arm/mpu: Provide and populate MPU C data structures

[Julien Grall <julien@xxxxxxx>]

Hi Ayan,

On 09/06/2025 11:43, Ayan Kumar Halder wrote:
> Hi,
>
> On 09/06/2025 10:16, Ayan Kumar Halder wrote:
> > On 09/06/2025 09:42, Julien Grall wrote:
> > > Hi Ayan,
> > Hi Julien,
> > > On 09/06/2025 09:27, Ayan Kumar Halder wrote:
> > > > On 09/06/2025 08:41, Orzel, Michal wrote:
> > > > > > diff --git a/xen/arch/arm/include/asm/mpu/regions.inc b/xen/arch/ 
> > > > > > arm/ include/asm/mpu/regions.inc
> > > > > > index 6b8c233e6c..631b0b2b86 100644
> > > > > > --- a/xen/arch/arm/include/asm/mpu/regions.inc
> > > > > > +++ b/xen/arch/arm/include/asm/mpu/regions.inc
> > > > > > @@ -24,7 +24,7 @@
> > > > > >   #define XEN_MPUMAP_ENTRY_SHIFT  0x3     /* 8 byte structure */
> > > > > >   .macro store_pair reg1, reg2, dst
> > > > > > -    .word 0xe7f000f0                    /* unimplemented */
> > > > > > +    stm \dst, {\reg1, \reg2}  /* reg2 should be a higher register 
> > > > > > than reg1 */
> > > > > Didn't we agree not to use STM (I suggested it but then Julien 
> > > > > pointed out that
> > > > > it's use in macro might not be the best)?
> > > > Ah my last response was not sent.
> > > >
> > > > I realized that I cannot use strd due to the following error
> > > >
> > > > Error: first transfer register must be even -- `strd r3,r4,[r1]'
> > > Ah I missed the "even" part when reading the specification. However, 
> > > we control the set of registers, so we can't we follow the 
> > > restriction? This would be better...
> > I am ok to follow this. I just want to make sure that this looks ok to 
> > you
> > prepare_xen_region() invokes store_pair(). They are in common header.
> >
> > So we need to make the change wherever prepare_xen_region() is invoked 
> > from arm32/mpu/head.S. This would look like
> > --- a/xen/arch/arm/arm32/mpu/head.S
> > +++ b/xen/arch/arm/arm32/mpu/head.S
> > @@ -58,33 +58,33 @@ FUNC(enable_boot_cpu_mm)
> >      /* Xen text section. */
> >      mov_w   r1, _stext
> >      mov_w   r2, _etext
> > -    prepare_xen_region r0, r1, r2, r3, r4, r5, 
> > attr_prbar=REGION_TEXT_PRBAR
> > +    prepare_xen_region r0, r1, r2, r4, r3, r5, 
> > attr_prbar=REGION_TEXT_PRBAR
> My mistake, It should be
>
> prepare_xen_region r0, r1, r2, r4, r5, r6, attr_prbar=REGION_TEXT_PRBAR
>
> We will be clobbering an extra register.
Why can't we use r3 instead of r6?

> So this would look a different different from its arm64 counterpart.

I don't see the problem with that. The code is not common. And TBH 
prepare_xen_region should never have been common ... but I will not 
re-open this discussion :).
Cheers,

--
Julien Grall