[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Julien Grall <julien@xxxxxxx>, Penny Zheng <Penny.Zheng@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • From: Ayan Kumar Halder <ayankuma@xxxxxxx>
  • Date: Fri, 30 Jun 2023 10:26:08 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=++18t5IrWfF6PP2c7FUZG6roHKCyELeko2HsbSaV2ts=; b=Df7wXB8euYdUnjVhIn1NaeRg99QgFNXLKk0H/HtZAGqxU9BVh/vSoYcvygD6E7mxPVtwF6OjAzGgtRgwJFxEDfGUjIhknqNVohiaZnVK+1B4g7LEc7aPhM3/7hMZgzUcbwNn8Xdn67cplAXUALV8ZJLCCevX04+/QiuDkUERQ2FbK5Si57fAF6KzisBbtxaJvBXDZan815cSW/tv1ddbdvPzCH99xeYKFzGvslV6p8lVDpKReI0grTHU9vnK+htgF35TVKYEv4loYjzeaW6M6dtxLEHjyPPJq7d7p9F02dChNcyNRbJ+3r2JfqTqod876QsdQMACpmBojnNHLOvmTw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=MLJV1xpKJ337i/YY72EFDjKJcnhep6z4LEIxJH2lIEDPYtxata32QsmLt6xlz/U+yvRMqsdu/3rtmdx8Po8wj1mj0zvLmSgVebcKOt3fusVumIqWgqLmmGO3JvdpFDg87l3kkF0DTD0NJ6uLNg+AN7wu6g9/aDMsnF8XIsjjE2cCgQlSGwCZ8Y+3fEXWft6UIi5Xz2m1N91AbJ/AL5Www5qYIzewCeoR5E6ImL80MYDFoKKsaTgdvl1OXzNlyoXJSo2tDs81YzilLNCwNdtDOoQQI4QZSljVuQCyc9Z5ox2x9v9mr2FS4NGNSr58KedWpedatj1sN+EY3iDGRc3TZA==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.com;
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Wei Chen <wei.chen@xxxxxxx>
  • Delivery-date: Fri, 30 Jun 2023 09:26:32 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>


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,




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.