Re: [PATCH v3 24/52] xen/mpu: build up start-of-day Xen MPU memory region map

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 29/06/2023 12:55, Julien Grall wrote:
> Hi Ayan,
Hi Julien,
> On 29/06/2023 12:21, Ayan Kumar Halder wrote:
> > On 28/06/2023 14:42, Julien Grall wrote:
> > > What's the guarantee that the compiler will not generate any 
> > > instructions that could generate an alignment fault?
> > I thought by writing in assembly, we tell the compiler what 
> > instructions to generate. For eg
> > ENTRY(set_boot_mpumap)
> >      push {r4}
> >      mov   r2, #0               /* table index */
> > 1:  ldr   r3, [r1], #4         /* r3: prbar */
> >      ldr   r4, [r1], #12        /* r4: prlar */
> >      write_pr r2, r3, r4
> >      add   r2, r2, #1           /* table index ++ */
> >      cmp   r2, r0
> >      blt  1b
> >      pop {r4}
> >      ret
> > ENDPROC(set_boot_mpumap)
> >
> > I ask the compiler to use ldr (and not ldrb) instructions.
> >
> > May be I am missing something very obvious here.
> The problem is not the assembly code. The problem is the C code. You 
> wrote:
>     /*
>      * Since it is the MPU protection region which holds the XEN 
> kernel that
>      * needs updating.
>      * The whole MPU system must be disabled for the update.
>      */
>     disable_mpu();
>
>     /*
>      * Set new MPU memory region configuration.
>      * To avoid the mismatch between nr_xen_mpumap and nr_xen_mpumap
>      * after the relocation of some MPU regions later, here
>      * next_xen_mpumap_index is used.
>      * To avoid unexpected unaligment access fault during MPU disabled,
>      * set_boot_mpumap shall be written in assembly code.
>      */
>     set_boot_mpumap(next_xen_mpumap_index, (pr_t *)boot_mpumap);
>
>     enable_mpu();
>
> You can't guarantee what assembly instructions the compiler will use 
> for any of this code. So if you are concerned about unaligned access 
> when the MPU is disabled, then you should never return to C (even 
> temporarily) while the MPU is off.
yes, agreed.
> > > Furthermore, from my understanding, at least on Armv8-A, there are 
> > > caching problem because you will need to save some registers (for 
> > > the call to set_boot_mpumap()) on the stack with cache disabled. 
> > > This means the cache will be bypassed. But you may then restore the 
> > > registers with the cache enabled (the compiler could decide that it 
> > > is not necessary to read the stack before hand). So you could read 
> > > the wrong data if there is a stale cacheline.
> > Yes, this makes some sense. So will the following make it correct :-
> I am confused. In a previous answer, I voiced my concerned with trying 
> to replace the full MPU table. So it is not clear to me why you are 
> asking me if the following work. Do you still want to do it? If so, why?
I completely agree with you to set up the MPU table in assembly with the 
correct permissions for each section (as done by Penny's patch). That 
would atleast ensure that we don't need to reset the MPU sections for 
Xen again.
What I was trying to understand deeper was some of the objections you 
had raised and if any sort of mitigations are possible.
Again I am not saying that we need to apply the mitigations (if 
available) in this particular scenario.
> > 1. Execute 'dmb' before invoking enable_mpu(). This will ensure that 
> > the registers are strictly restored in set_boot_mpumap() before the 
> > HSCTLR is read.
> I am afraid I don't know how the DMB will enforce that. Can you clarify?
pop {r4}  /* As part of set_boot_mpumap() */

dmb /* This should ensure that r4 is fully restored from the stack 
before the next instruction. At this point, the D cache is still 
disabled. */
> > We do have 'dsb sy' before modifying HSCTLR (ie enabling cache), but 
> > may be we want to be stricter.
> > 2. Invalidate the D cache after "mcr   CP32(r0, HSCTLR)" and then dsb 
> > (to ensure d cache is invalidated), isb (flush the instruction cache 
> > as MPU is enabled), ret.
> I might be missing something here. The ISB instruction will not flush 
> the instruction cache, it will flush the pipeline instead and 
> guarantee that previous instructions will complete before continuing.
Sorry my bad, I meant flushing the pipeline so that pipeline is refilled 
with the instructions from the MPU regions with the correct permissions.
> But overall, the easiest solution is to disable the MPU, update the 
> MPU tables, and then re-enable the MPU all in assembly (i.e. no jump 
> back to C even temporarily).
> So you control the accesses and can limit (if not remove) any write to 
> the memory whilst the cache is disabled.
Yes, agreed. I will rework my patch so that the MPU regions are set up 
with the sections of Xen in assembly (similar to what is being done in 
Penny's patch).
- Ayan

> Cheers,