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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Julien/Luca/all,

On 04/09/2024 19:38, Ayan Kumar Halder wrote:
> On 04/09/2024 19:14, Luca Fancellu wrote:
> > Hi Ayan,
> Hi Luca,
> > Apologies but I can’t do a full review yet,
> No worries. :)
> > > > > +
> > > > > +/* MPU normal memory attributes. */
> > > > > +#define PRBAR_NORMAL_MEM        0x30    /* SH=11 AP=00 XN=00 */
> > > > > +#define PRLAR_NORMAL_MEM        0x0f    /* NS=0 ATTR=111 EN=1 */
> > > > > +
> > > > > +.macro write_pr, sel, prbar, prlar
> > > > > +    msr   PRSELR_EL2, \sel
> > > > > +    dsb   sy
> > > > I am not sure I understand why this is a dsb rather than isb. Can 
> > > > you clarify?
> > > ISB is not needed here as the memory protection hasn't been 
> > > activated yet. The above instruction just selects the memory region 
> > > and the below two instructions sets the base address and limit for 
> > > that memory region. After the three instructions, we need an ISB so 
> > > that the memory protection takes into affect for further instruction 
> > > fetches.
> > > However, a DSB is needed here as the below two instructions depend 
> > > on this. So, we definitely want this instruction to complete.
> > > Further, refer to the note in ARM DDI 0600A.d ID120821, C1.7.1 
> > > "Protection region attributes"
> > > 0.
> > >
> > >    ```Writes to MPU registers are only guaranteed to be visible
> > >    following a Context synchronization event and DSB operation.```
> > >
> > > Thus, I infer that DSB is necessary here.
> > I think this was a mistake from the author of this patch, in my 
> > opinion there should be an ISB
> > after setting PRSELR_ELx, to enforce a synchronisation before writing 
> > PR{B,L}AR_ELx which
> > depends on the value written on PRSELR.
> That synchronisation is enforced by DSB.
>
> From 
> https://developer.arm.com/documentation/dui0489/c/arm-and-thumb-instructions/miscellaneous-instructions/dmb--dsb--and-isb 
> ,
> ```Data Synchronization Barrier acts as a special kind of memory 
> barrier. No instruction in program order after this instruction 
> executes until this instruction completes.
> ...
>
> Instruction Synchronization Barrier flushes the pipeline in the 
> processor, ```
>
> Why should we flush the instruction pipeline after setting PRSELR_ELx 
> ? It does not have any impact on instruction fetch.
After the discussion on matrix, I agree that we need an 'isb' here to 
ensure that there is no instruction reordering.
Thanks for the explanation.

- Ayan