[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 4/6] xen/arm: mpu: Create boot-time MPU protection regions
On 18/10/2024 23:13, Julien Grall wrote: Hi Ayan, Hi Julien, Just one clarification. On 10/10/2024 15:03, Ayan Kumar Halder wrote:diff --git a/xen/arch/arm/arm64/mpu/Makefile b/xen/arch/arm/arm64/mpu/Makefilenew file mode 100644 index 0000000000..3340058c08 --- /dev/null +++ b/xen/arch/arm/arm64/mpu/Makefile @@ -0,0 +1 @@ +obj-y += head.odiff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.Snew file mode 100644 index 0000000000..4a21bc815c --- /dev/null +++ b/xen/arch/arm/arm64/mpu/head.S @@ -0,0 +1,130 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Start-of-day code for an Armv8-R MPU system. + */ + +#include <asm/mm.h> +#include <asm/arm64/mpu/sysregs.h> + +#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_NORMAL_PRLAR 0x0f /* NS=0 ATTR=111 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. + * + * Inputs: + * sel: region selector + * base: reg storing base address (should be page-aligned) + * 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 + */+.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR++ /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */+ add \sel, \sel, #1 + cmp \sel, \maxcount + bgt fail + + /* 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 +.endm + +/* Load the physical address of a symbol into xb */ +.macro load_paddr xb, sym + ldr \xb, =\sym + add \xb, \xb, x20 /* x20 - Phys offset */Sorry I didn't spot this until now. Xen will be linked to a specific physical address, so why do we need to add an offset? Yes, this needs to be removed. x20 contains 0. +.endm + +/*+ * Maps the various sections of Xen (described in xen.lds.S) as different MPU+ * regions. + * + * Inputs: + * lr : Address to return to. + * + * Clobbers x0 - x5 + * + */ +FUNC(enable_boot_cpu_mm) ++ /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */AFAIU, this doesn't match what the instruction is doing below. Sorry, this needs to be removed. Is it ok if we have this excess check ? The downsides are only a small increase in code size and boot time. Otherwise, I need to justify why we have this checks in some places, but not in others.+ mrs x5, MPUIR_EL2 + + /* x0: region sel */ + mov x0, xzr + /* Xen text section. */ + load_paddr x1, _stext + load_paddr x2, _etext + cmp x1, x2 + beq 1fThis check seems to be excessive... I can't think of a reason why there would be no text at all... Same for a lot of the checks below. + prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR+ +1: /* Xen read-only data section. */ + load_paddr x1, _srodata + load_paddr x2, _erodata + cmp x1, x2 + beq 2f+ prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR+ +2: /* Xen read-only after init and data section. (RW data) */ + load_paddr x1, __ro_after_init_start + load_paddr x2, __init_begin + cmp x1, x2 + beq 3f + prepare_xen_region x0, x1, x2, x3, x4, x5 + +3: /* Xen code section. */ + load_paddr x1, __init_begin + load_paddr x2, __init_data_begin + cmp x1, x2 + beq 4f+ prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR+ +4: /* Xen data and BSS section. */ + load_paddr x1, __init_data_begin + load_paddr x2, __bss_end + cmp x1, x2 + beq 5f + prepare_xen_region x0, x1, x2, x3, x4, x5 + +5: + ret + +fail:This name is a bit too generic given you use within a macro. Also, I think it should be its own local function because the macro can be used anywhere. Ack. I will convert this to a function. - Ayan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |