Hi,
On 04/09/2024 13:21, Ayan Kumar Halder wrote:
On 28/08/2024 16:01, Julien Grall wrote:
On MPU systems, XEN_START_ADDRESS is also the
+ * address that Xen image should be loaded at. So for initial MPU
+ * regions setup, we use 2MB for Xen data memory to setup boot
+ * region, or the create boot regions code below will need adjustment.
+ */
+#define XEN_START_MEM_SIZE 0x200000
See above.
Ack.
+
+/*
+ * In boot stage, we will use 1 MPU region:
+ * Region#0: Normal memory for Xen text + data + bss (2MB)
+ */
+#define BOOT_NORMAL_REGION_IDX 0x0
+
+/* MPU normal memory attributes. */
+#define PRBAR_NORMAL_MEM 0x30 /* SH=11 AP=00 XN=00 */
+#define PRLAR_NORMAL_MEM 0x0f /* NS=0 ATTR=111 EN=1 */
+
+.macro write_pr, sel, prbar, prlar
+ msr PRSELR_EL2, \sel
+ dsb sy
I am not sure I understand why this is a dsb rather than isb. Can you
clarify?
ISB is not needed here as the memory protection hasn't been activated
yet. The above instruction just selects the memory region and the below
two instructions sets the base address and limit for that memory region.
After the three instructions, we need an ISB so that the memory
protection takes into affect for further instruction fetches.
We discussed this on Matrix, I have also had a read of the Arm Arm again
(D19.1.2 General behavior of accesses to the AArch64 System registers in
ARM DDI 0487J.a). Writing down mainly for keeping track of the decision.
In general you need an a context synchronization event (e.g. isb) before
the other system registers can start to rely on the change. There are
some exceptions though, but AFAICT PRSELR_EL2 doesn't have any.
IIUC, the behavior of PRBAR_EL2 and PRLAR_EL2 will change depending on
PRSELR_EL2.REGION. Therefore, we need to make sure the new value of
PRSELR_EL2 is seen before we write to the two other registers.
However, a DSB is needed here as the below two instructions depend on
this. So, we definitely want this instruction to complete.
Further, refer to the note in ARM DDI 0600A.d ID120821, C1.7.1
"Protection region attributes"
0.
```Writes to MPU registers are only guaranteed to be visible
following a Context synchronization event and DSB operation.```
Thus, I infer that DSB is necessary here
I read it differently. To me it was more the "dsb" is necessary before
the MPU can start using the new region values. But here an 'isb' should
be fine.
That said, from my experience, certain section of the Arm Arm can be
interpreted differently whic means that someone could implement your
way. This is also not a hot path, so best to use the most restrictive
approach. Therefore I am ok with adding a 'dsb'.
If a "dsb" is necessary, "sy" seems to be quite strict.
I can use "st" here as I am only interested in stores (ie MSR) to complete.
Now the whether we want to restrict it to inner shareable/outer
shareable/POU/full system is a difficult decision to make.
May be here we can use ishst (stores to complete to inner shareable
region). However ....
+ msr PRBAR_EL2, \prbar
+ msr PRLAR_EL2, \prlar
+ dsb sy
This should be visible to outer shareable domain atleast. The reason
being one can use the SH[1:0] bits in PRBAR_EL2 to set the region to
outer shareable.
Thus, the writes to these registers should be visible to outer shareable
domain as well.
I am a bit confused. SH[1:0] is about how the region will be accessed
not up to where should registers are visible. I was expecting that the
MPU registers only need to be visible to the MPU itself.
For instance, when using the MMU, the translation unit is in the
non-shareable domain. So a 'nsh' is sufficient regardless of the
shareability of the page/block.
This is explicitely written in the Arm Arm (see D5-4929 in ARM DDI
0487H.a) but I can't find a similar section for the MPU yet. Although, I
would be a bit surprised if the MPU is not in the non-shareable
domain... Maybe this could be clarified with Arm?
Anyway, for now, I am open to use 'dsb sy' with a TODO to revisit it.
+ isb
Re-quoting the spec from you previous answer:
```
Writes to MPU registers are only guaranteed to be visible
following a Context synchronization event and DSB operation.
```
So this suggests that it should be first an 'isb' and then a 'dsb'. Any
reason you wrote it the other way around?
+.endm
+
+.section .text.header, "ax", %progbits
+
+/*
+ * Static start-of-day EL2 MPU memory layout.
+ *
+ * It has a very simple structure, including:
+ * - 2MB normal memory mappings of xen at XEN_START_ADDRESS, which
+ * is the address where Xen was loaded by the bootloader.
+ */
Please document a bit more the code and how the registers are used.
For an example see the mmu/head.S code.
Ack
+ENTRY(enable_boot_cpu_mm)
+ /* Map Xen start memory to a normal memory region. */
+ mov x0, #BOOT_NORMAL_REGION_IDX
+ ldr x1, =XEN_START_ADDRESS
+ and x1, x1, #MPU_REGION_MASK
+ mov x3, #PRBAR_NORMAL_MEM
+ orr x1, x1, x3
+
+ ldr x2, =XEN_START_ADDRESS
+ mov x3, #(XEN_START_MEM_SIZE - 1)
+ add x2, x2, x3
+ and x2, x2, #MPU_REGION_MASK
+ mov x3, #PRLAR_NORMAL_MEM
+ orr x2, x2, x3
+
+ /*
+ * Write to MPU protection region:
+ * x0 for pr_sel, x1 for prbar x2 for prlar
+ */
+ write_pr x0, x1, x2
+
+ ret
+ENDPROC(enable_boot_cpu_mm)
Missing emacs magic.
You mean it should be
END(enable_boot_cpu_mm) .
/*
* Local variables:
* mode: ASM
* indent-tabs-mode: nil
* End:
*/
Correct.
diff --git a/xen/arch/arm/include/asm/mpu/mm.h
b/xen/arch/arm/include/asm/mpu/mm.h
new file mode 100644
index 0000000000..f5ebca8261
--- /dev/null
+++ b/xen/arch/arm/include/asm/mpu/mm.h
Do we need this file?
In xen/arch/arm/arm64/mpu/head.S, we have
#include <asm/mm.h>
So, it should pick up this file.
OOI, why can't you include asm/arm64/mm.h?
Cheers,
--
Julien Grall
