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

[Julien Grall <julien@xxxxxxx>]

Hi Ayan,

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.
> > 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?
> 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?

> 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.
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.
Cheers,

--
Julien Grall