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

Re: [PATCH v3 2/5] xen/arm: Move some of the functions to common file


  • To: Julien Grall <julien@xxxxxxx>, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • From: Ayan Kumar Halder <ayankuma@xxxxxxx>
  • Date: Mon, 31 Mar 2025 13:38:30 +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=arcselector10001; 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=x3UGQACn5z4ZnDIPbPEYZTBhIi2Fl+pw5xtcqgngY14=; b=FDZpyt6o/Ex/s7GGmNWjWVRVSBRgtb0ihuC/TYKNMj7ROEPH8LGzepZi4bgDs3A3cacsRSq89q2L+n2hvcdXTbkZhYaPtlfirj+m3/6x5OhrZhO5ddKJ0KmqJp5ozcmulyJFAnERszhi0lkCxuvzKWrdtsXH9H3GSd7tzedj3aquC4QZIDSGsv2SXkc/4UBF3hWak52P5RHgQ9PVxgG5UABEuRsSthbuUx7T7Lj4Aw+HzrUFVEqzsO46Qx+tS6BH6NrOcrsHHa5vtVxMshO7FEfhy/01R56z9QSA+N1dp6WiWAm+kzCbJKXdgkWxhW+UplJrYDFXAR1/j0P76ERMtQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=HcN2+71k+7BZIJs1QZNCwcT/6bx8WyHL9SqVen/471xGMTwjpTOBYsB8JlzZdozo/eRQGHooyoWJ+jlXZq67mk3goPYn5p0XHdLEiYjlBH2ofhoEr2FhJfdYCzn3jVmoDczp3TI26weWsZIyQlCRmfHvhTPz/6dAYPAfWaOT7AaFI1jFmL3dVPUiTpH74WPnMg3z1ESXw1RQ/Axt1+rPE5hqg5lwA07/v/+T4hsZ0wAEAagB/beMR6xrL5RkBnqajCGVvZGxA+HdUbcC9wkUCpkkX0uMT3L+sDhotM4Ml2iM0GV0LLucSieztrUoTAyQlfk9Tq6YlJ4pfa6qdaEpeA==
  • 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>, Michal Orzel <michal.orzel@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Mon, 31 Mar 2025 12:38:49 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>


On 30/03/2025 22:06, Julien Grall wrote:
Hi Ayan,

Hi Julien/Michal,

Michal - I agree to use {READ/WRTIE}_SYSREG_ASM() instead of LOAD/STORE.

I need your thoughts on the following.


On 30/03/2025 19:03, Ayan Kumar Halder wrote:
Added a new file prepare_xen_region.inc to hold the common earlyboot MPU regions
configurations across arm64 and arm32.

While I understand the desire to consolidate the code, I am quite unconvinced this should be done for assembly code. A few examples below why.

I would be interested to hear the view of the other Arm maintainers.


prepare_xen_region, enable_boot_cpu, fail_insufficient_regions() will be used by both arm32 and arm64. Thus, they have been moved to prepare_xen_region.inc.

REGION_* are moved to arm64/sysregs.h. Introduced LOAD_SYSREG and STORE_SYSREG to read/write to the system registers from the common asm file. One could not reuse READ_SYSREG and WRITE_SYSREG as they have been defined to be invoked from
C files.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
---
Changes from

v1 -

1. enable_mpu() now sets HMAIR{0,1} registers. This is similar to what is being done in enable_mmu(). All the mm related configurations happen in this
function.

2. Fixed some typos.

v2 -
1. Extracted the arm64 head.S functions/macros in a common file.

  xen/arch/arm/arm64/mpu/head.S                 | 132 +-----------------
  xen/arch/arm/include/asm/arm64/sysregs.h      |  15 ++
  .../include/asm/mpu/prepare_xen_region.inc    | 128 +++++++++++++++++
  3 files changed, 148 insertions(+), 127 deletions(-)
  create mode 100644 xen/arch/arm/include/asm/mpu/prepare_xen_region.inc

diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
index 4d00de4869..90b4c8c18f 100644
--- a/xen/arch/arm/arm64/mpu/head.S
+++ b/xen/arch/arm/arm64/mpu/head.S
@@ -3,83 +3,7 @@
   * Start-of-day code for an Armv8-R MPU system.
   */
  -#include <asm/early_printk.h>
-#include <asm/mpu.h>
-
-/* Backgroud region enable/disable */
-#define SCTLR_ELx_BR    BIT(17, UL)
-
-#define REGION_TEXT_PRBAR       0x38    /* SH=11 AP=10 XN=00 */
-#define REGION_RO_PRBAR         0x3A    /* SH=11 AP=10 XN=10 */
-#define REGION_DATA_PRBAR       0x32    /* SH=11 AP=00 XN=10 */
-#define REGION_DEVICE_PRBAR     0x22    /* SH=10 AP=00 XN=10 */
-
-#define REGION_NORMAL_PRLAR     0x0f    /* NS=0 ATTR=111 EN=1 */
-#define REGION_DEVICE_PRLAR     0x09    /* NS=0 ATTR=100 EN=1 */
-
-/*
- * Macro to prepare and set a EL2 MPU memory region.
- * We will also create an according MPU memory region entry, which
- * is a structure of pr_t,  in table \prmap.
- *
- * sel:         region selector
- * base:        reg storing base address
- * limit:       reg storing limit address
- * prbar:       store computed PRBAR_EL2 value
- * prlar:       store computed PRLAR_EL2 value
- * maxcount:    maximum number of EL2 regions supported
- * attr_prbar:  PRBAR_EL2-related memory attributes. If not specified it will be
- *              REGION_DATA_PRBAR
- * attr_prlar:  PRLAR_EL2-related memory attributes. If not specified it will be
- *              REGION_NORMAL_PRLAR
- *
- * Preserves \maxcount
- * Output:
- *  \sel: Next available region selector index.
- * Clobbers \base, \limit, \prbar, \prlar
- *
- * Note that all parameters using registers should be distinct.
- */
-.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR
-    /* Check if the region is empty */
-    cmp   \base, \limit
-    beq   1f
-
-    /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */
-    cmp   \sel, \maxcount
-    bge   fail_insufficient_regions
-
-    /* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/
-    and   \base, \base, #MPU_REGION_MASK
-    mov   \prbar, #\attr_prbar
-    orr   \prbar, \prbar, \base
-
-    /* Limit address should be inclusive */
-    sub   \limit, \limit, #1
-    and   \limit, \limit, #MPU_REGION_MASK
-    mov   \prlar, #\attr_prlar
-    orr   \prlar, \prlar, \limit
-
-    msr   PRSELR_EL2, \sel
-    isb
-    msr   PRBAR_EL2, \prbar
-    msr   PRLAR_EL2, \prlar
-    dsb   sy
-    isb
-
-    add   \sel, \sel, #1
-
-1:
-.endm
-
-/*
- * Failure caused due to insufficient MPU regions.
- */
-FUNC_LOCAL(fail_insufficient_regions)
-    PRINT("- Selected MPU region is above the implemented number in MPUIR_EL2 -\r\n")
-1:  wfe
-    b   1b
-END(fail_insufficient_regions)
+#include <asm/mpu/prepare_xen_region.inc>
    /*
   * Enable EL2 MPU and data cache
@@ -108,62 +32,16 @@ END(enable_mpu)
   * Maps the various sections of Xen (described in xen.lds.S) as different MPU
   * regions.
   *
- * Clobbers x0 - x5
+ * Clobbers x0 - x6
   *
   */
  FUNC(enable_boot_cpu_mm)
-    /* Get the number of regions specified in MPUIR_EL2 */
-    mrs   x5, MPUIR_EL2
-    and   x5, x5, #NUM_MPU_REGIONS_MASK
-
-    /* x0: region sel */
-    mov   x0, xzr
-    /* Xen text section. */
-    ldr   x1, =_stext
-    ldr   x2, =_etext
-    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
-
-    /* Xen read-only data section. */
-    ldr   x1, =_srodata
-    ldr   x2, =_erodata
-    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR
-
-    /* Xen read-only after init and data section. (RW data) */
-    ldr   x1, =__ro_after_init_start
-    ldr   x2, =__init_begin
-    prepare_xen_region x0, x1, x2, x3, x4, x5
-
-    /* Xen code section. */
-    ldr   x1, =__init_begin
-    ldr   x2, =__init_data_begin
-    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
-
-    /* Xen data and BSS section. */
-    ldr   x1, =__init_data_begin
-    ldr   x2, =__bss_end
-    prepare_xen_region x0, x1, x2, x3, x4, x5
-
-#ifdef CONFIG_EARLY_PRINTK
-    /* Xen early UART section. */
-    ldr   x1, =CONFIG_EARLY_UART_BASE_ADDRESS
-    ldr   x2, =(CONFIG_EARLY_UART_BASE_ADDRESS + CONFIG_EARLY_UART_SIZE) -    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_DEVICE_PRBAR, attr_prlar=REGION_DEVICE_PRLAR
-#endif
-
-    b    enable_mpu
+    mov   x6, lr

Aside what I wrote above, why do we need to save/restore lr?

In the case of arm32, there is no 'ret' instruction.

When enable_boot_cpu_mm() ----(invokes)--> enable_mpu(), LR should be saved and restored. Otherwise, one cannot return to the caller of enable_boot_cpu_mm()


+    enable_boot_cpu x0, x1, x2, x3, x4, x5
+    mov   lr, x6
      ret
  END(enable_boot_cpu_mm)
  -/*
- * We don't yet support secondary CPUs bring-up. Implement a dummy helper to
- * please the common code.
- */
-ENTRY(enable_secondary_cpu_mm)
-    PRINT("- SMP not enabled yet -\r\n")
-1:  wfe
-    b 1b
-ENDPROC(enable_secondary_cpu_mm)
-
  /*
   * Local variables:
   * mode: ASM
diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h
index b593e4028b..9b833fe73b 100644
--- a/xen/arch/arm/include/asm/arm64/sysregs.h
+++ b/xen/arch/arm/include/asm/arm64/sysregs.h
@@ -462,6 +462,19 @@
  #define ZCR_ELx_LEN_SIZE             9
  #define ZCR_ELx_LEN_MASK             0x1ff
  +#define REGION_TEXT_PRBAR       0x38    /* SH=11 AP=10 XN=00 */
+#define REGION_RO_PRBAR         0x3A    /* SH=11 AP=10 XN=10 */
+#define REGION_DATA_PRBAR       0x32    /* SH=11 AP=00 XN=10 */
+#define REGION_DEVICE_PRBAR     0x22    /* SH=10 AP=00 XN=10 */

While those makes sense in sysreg.h because they are definition based on the Arm Arm. The definition for ...

+
+#define REGION_NORMAL_PRLAR     0x0f    /* NS=0 ATTR=111 EN=1 */
+#define REGION_DEVICE_PRLAR     0x09    /* NS=0 ATTR=100 EN=1 */

for PRLAR are specific to Xen. So I think they should be moved under mpu/. I would also consider *_PRBAR ones.

Also, are those values truely arm64 specific? Asking because you are using them in common code.

These definitions (like the _PRBAR) are based on Arm Arm as well. The only difference is that these two values are common for arm32 and arm64.

I preferred to duplicate these values in both the header files so that they stay together with _PRBAR. One needs to use both _PRBAR and _PRLAR to set a MPU region. Thus, I don't prefer to split them in separate header files.


+
+#define STORE_SYSREG(v, name) "msr " __stringify(name,) #v;
+#define LOAD_SYSREG(v, name) "mrs " #v __stringify(,) #name;
+
+#ifndef __ASSEMBLY__
+
  /* Access to system registers */
    #define WRITE_SYSREG64(v, name) do {                    \
@@ -481,6 +494,8 @@
  #define WRITE_SYSREG_LR(v, index)  WRITE_SYSREG(v, ICH_LR_REG(index))
  #define READ_SYSREG_LR(index) READ_SYSREG(ICH_LR_REG(index))
  +#endif /* __ASSEMBLY__ */
+
  #endif /* _ASM_ARM_ARM64_SYSREGS_H */
    /*
diff --git a/xen/arch/arm/include/asm/mpu/prepare_xen_region.inc b/xen/arch/arm/include/asm/mpu/prepare_xen_region.inc
new file mode 100644
index 0000000000..3402ed23da
--- /dev/null
+++ b/xen/arch/arm/include/asm/mpu/prepare_xen_region.inc
@@ -0,0 +1,128 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <asm/sysregs.h>
+#include <asm/mpu.h>
+
+/* Backgroud region enable/disable */
+#define SCTLR_ELx_BR    BIT(17, UL)
+
+/*
+ * Macro to prepare and set a EL2 MPU memory region.
+ * We will also create an according MPU memory region entry, which
+ * is a structure of pr_t,  in table \prmap.
+ *
+ * sel:         region selector
+ * base:        reg storing base address
+ * limit:       reg storing limit address
+ * prbar:       store computed PRBAR_EL2 value
+ * prlar:       store computed PRLAR_EL2 value
+ * maxcount:    maximum number of EL2 regions supported
+ * attr_prbar:  PRBAR_EL2-related memory attributes. If not specified it will be
+ *              REGION_DATA_PRBAR
+ * attr_prlar:  PRLAR_EL2-related memory attributes. If not specified it will be
+ *              REGION_NORMAL_PRLAR
+ *
+ * Preserves maxcount
+ * Output:
+ *  sel: Next available region selector index.
+ * Clobbers base, limit, prbar, prlar
+ *
+ * Note that all parameters using registers should be distinct.
+ */
+.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR
+    /* Check if the region is empty */
+    cmp   \base, \limit
+    beq   1f
+
+    /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */
+    cmp   \sel, \maxcount
+    bge   fail_insufficient_regions
+
+    /* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/
+    and   \base, \base, #MPU_REGION_MASK
+    mov   \prbar, #\attr_prbar
+    orr   \prbar, \prbar, \base
+
+    /* Limit address should be inclusive */
+    sub   \limit, \limit, #1
+    and   \limit, \limit, #MPU_REGION_MASK
+    mov   \prlar, #\attr_prlar
+    orr   \prlar, \prlar, \limit
+
+    STORE_SYSREG(\sel, PRSELR_EL2)
+    isb
+    STORE_SYSREG(\prbar, PRBAR_EL2)
+    STORE_SYSREG(\prlar, PRLAR_EL2)
+    dsb   sy
+    isb
+
+    add   \sel, \sel, #1
+
+1:
+.endm
+
> +.macro enable_boot_cpu, reg0, reg1, reg2, reg3, reg4, reg5

If we go this approach, this will need some documentation on top (similar to the other macro in this file).
Ack. will do.

+    /* Get the number of regions specified in MPUIR_EL2 */
+    LOAD_SYSREG(\reg5, MPUIR_EL2)
+    and   \reg5, \reg5, #NUM_MPU_REGIONS_MASK
+
+    /* reg0: region sel */
+    mov   \reg0, #0
+    /* Xen text section. */
+    ldr   \reg1, =_stext

For instance, on Arm32, this could be replaced with ``mov_w`` which is doesn't involve memory load.

Yes, this is a minor difference. Remember that this code is executed only once during boot time. While there is a performance penalty with ldr, should we really use mov_w (and as a consequence have a different implementation for arm32) where 99% code is the same and we have lesser code to mantain.

(Julien/Michal :- need your comments here)


+    ldr   \reg2, =_etext
+    prepare_xen_region \reg0, \reg1, \reg2, \reg3, \reg4, \reg5, attr_prbar=REGION_TEXT_PRBAR
+
+    /* Xen read-only data section. */
+    ldr   \reg1, =_srodata
+    ldr   \reg2, =_erodata
+    prepare_xen_region \reg0, \reg1, \reg2, \reg3, \reg4, \reg5, attr_prbar=REGION_RO_PRBAR
+
+    /* Xen read-only after init and data section. (RW data) */
+    ldr   \reg1, =__ro_after_init_start
+    ldr   \reg2, =__init_begin
+    prepare_xen_region \reg0, \reg1, \reg2, \reg3, \reg4, \reg5
+
+    /* Xen code section. */
+    ldr   \reg1, =__init_begin
+    ldr   \reg2, =__init_data_begin
+    prepare_xen_region \reg0, \reg1, \reg2, \reg3, \reg4, \reg5, attr_prbar=REGION_TEXT_PRBAR
+
+    /* Xen data and BSS section. */
+    ldr   \reg1, =__init_data_begin
+    ldr   \reg2, =__bss_end
+    prepare_xen_region \reg0, \reg1, \reg2, \reg3, \reg4, \reg5
+
+#ifdef CONFIG_EARLY_PRINTK
+    /* Xen early UART section. */
+    ldr   \reg1, =CONFIG_EARLY_UART_BASE_ADDRESS
+    ldr   \reg2, =(CONFIG_EARLY_UART_BASE_ADDRESS + CONFIG_EARLY_UART_SIZE) +    prepare_xen_region \reg0, \reg1, \reg2, \reg3, \reg4, \reg5, attr_prbar=REGION_DEVICE_PRBAR, attr_prlar=REGION_DEVICE_PRLAR
+#endif
+
+    bl    enable_mpu
+.endm
+
+/* Failure caused due to insufficient MPU regions. */
+FUNC_LOCAL(fail_insufficient_regions)
+    PRINT("- Selected MPU region is above the implemented number in MPUIR_EL2 -\r\n")
+1:  wfe
+    b   1b
+END(fail_insufficient_regions)
+
+/*
+ * We don't yet support secondary CPUs bring-up. Implement a dummy helper to
+ * please the common code.
+ */
+ENTRY(enable_secondary_cpu_mm)

I really doubt we will be able to keep this function common in the future.

Are you ok if we keep it here for now ?


Looking a bit further ahead, the MPU registers are not core specific (unlike MMU). Thus, there is no MPU specific configuration that we should be doing for the secondary cores. We can investigate more on this when we enable SMP support.

- Ayan


+    PRINT("- SMP not enabled yet -\r\n")
+1:  wfe
+    b 1b
+ENDPROC(enable_secondary_cpu_mm)
+
+/*
+ * Local variables:
+ * mode: ASM
+ * indent-tabs-mode: nil
+ * End:
+ */

Cheers,




 


Rackspace

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