[PATCH 00/20] xen/arm: Clean-up & fixes in boot/mm code

[PATCH 00/20] xen/arm: Clean-up & fixes in boot/mm code

[Julien Grall]

Hi all,

This is the second part of the boot/memory rework for Xen on Arm. This
part contains mostly clean-up & fixes found during the rework.

The first part of the rework is "xen/arm: TLB flush helpers rework" [1].

For convenience, I provided a branch with all the patches applied based
on next-4.13 (it is staging + patch queued for Arm):

git://xenbits.xen.org/people/julieng/xen-unstable.git branch mm/part2/v1

Cheers,

[1] https://lists.xen.org/archives/html/xen-devel/2019-04/msg01432.html


Julien Grall (20):
  xen/const: Introduce _BITUL and _BITULL
  xen/arm: Rename SCTLR_* defines and remove unused one
  xen/arm: processor: Use _BITUL instead of _AC(1, U) in SCTLR_ defines
  xen/arm: Rework HSCTLR_BASE
  xen/arm: Rework secondary_start prototype
  xen/arm: Remove parameter cpuid from start_xen
  xen/arm64: head: Remove unnecessary comment
  xen/arm64: head: Move earlyprintk messages in .rodata.str
  xen/arm64: head: Correctly report the HW CPU ID
  xen/arm32: head: Correctly report the HW CPU ID
  xen/arm32: head: Don't set MAIR0 and MAIR1
  xen/arm32: head: Always zero r3 before update a page-table entry
  xen/arm32: mm: Avoid to zero and clean cache for CPU0 domheap
  xen/arm32: mm: Avoid cleaning the cache for secondary CPUs page-tables
  xen/arm: mm: Use the shorter version __aligned(PAGE_SIZE) to align
    page-tables
  xen/arm: mm: Protect Xen page-table update with a spinlock
  xen/arm: mm: Initialize page-tables earlier
  xen/arm: mm: Check start is always before end in {destroy,
    modify}_xen_mappings
  xen/arm: Pair call to set_fixmap with call to clear_fixmap in
    copy_from_paddr
  xen/arm: Allow cleaning the directory even when CONFIG_EARLY_PRINTK is
    set

 xen/arch/arm/Makefile           |  5 +++
 xen/arch/arm/Rules.mk           |  7 ----
 xen/arch/arm/arm32/head.S       | 33 ++++-----------
 xen/arch/arm/arm64/head.S       | 40 +++++-------------
 xen/arch/arm/guest_walk.c       |  2 +-
 xen/arch/arm/kernel.c           |  3 +-
 xen/arch/arm/mm.c               | 57 +++++++++++---------------
 xen/arch/arm/setup.c            |  7 ++--
 xen/arch/arm/smpboot.c          |  4 +-
 xen/arch/arm/traps.c            |  6 +--
 xen/include/asm-arm/asm_defns.h |  5 +++
 xen/include/asm-arm/p2m.h       |  4 +-
 xen/include/asm-arm/processor.h | 91 +++++++++++++++++++++++++++++++----------
 xen/include/xen/const.h         |  3 ++
 14 files changed, 137 insertions(+), 130 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 00/20] xen/arm: Clean-up & fixes in boot/mm code

[Julien Grall]

Hi all,

This is the second part of the boot/memory rework for Xen on Arm. This
part contains mostly clean-up & fixes found during the rework.

The first part of the rework is "xen/arm: TLB flush helpers rework" [1].

For convenience, I provided a branch with all the patches applied based
on next-4.13 (it is staging + patch queued for Arm):

git://xenbits.xen.org/people/julieng/xen-unstable.git branch mm/part2/v1

Cheers,

[1] https://lists.xen.org/archives/html/xen-devel/2019-04/msg01432.html


Julien Grall (20):
  xen/const: Introduce _BITUL and _BITULL
  xen/arm: Rename SCTLR_* defines and remove unused one
  xen/arm: processor: Use _BITUL instead of _AC(1, U) in SCTLR_ defines
  xen/arm: Rework HSCTLR_BASE
  xen/arm: Rework secondary_start prototype
  xen/arm: Remove parameter cpuid from start_xen
  xen/arm64: head: Remove unnecessary comment
  xen/arm64: head: Move earlyprintk messages in .rodata.str
  xen/arm64: head: Correctly report the HW CPU ID
  xen/arm32: head: Correctly report the HW CPU ID
  xen/arm32: head: Don't set MAIR0 and MAIR1
  xen/arm32: head: Always zero r3 before update a page-table entry
  xen/arm32: mm: Avoid to zero and clean cache for CPU0 domheap
  xen/arm32: mm: Avoid cleaning the cache for secondary CPUs page-tables
  xen/arm: mm: Use the shorter version __aligned(PAGE_SIZE) to align
    page-tables
  xen/arm: mm: Protect Xen page-table update with a spinlock
  xen/arm: mm: Initialize page-tables earlier
  xen/arm: mm: Check start is always before end in {destroy,
    modify}_xen_mappings
  xen/arm: Pair call to set_fixmap with call to clear_fixmap in
    copy_from_paddr
  xen/arm: Allow cleaning the directory even when CONFIG_EARLY_PRINTK is
    set

 xen/arch/arm/Makefile           |  5 +++
 xen/arch/arm/Rules.mk           |  7 ----
 xen/arch/arm/arm32/head.S       | 33 ++++-----------
 xen/arch/arm/arm64/head.S       | 40 +++++-------------
 xen/arch/arm/guest_walk.c       |  2 +-
 xen/arch/arm/kernel.c           |  3 +-
 xen/arch/arm/mm.c               | 57 +++++++++++---------------
 xen/arch/arm/setup.c            |  7 ++--
 xen/arch/arm/smpboot.c          |  4 +-
 xen/arch/arm/traps.c            |  6 +--
 xen/include/asm-arm/asm_defns.h |  5 +++
 xen/include/asm-arm/p2m.h       |  4 +-
 xen/include/asm-arm/processor.h | 91 +++++++++++++++++++++++++++++++----------
 xen/include/xen/const.h         |  3 ++
 14 files changed, 137 insertions(+), 130 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 01/20] xen/const: Introduce _BITUL and _BITULL

[Julien Grall]

The pattern _AC(1, UL{,L}) << X is commonly used in the headers to make
define usuable in both assembly and C.

So introduce _BITUL and _BITULL to make the code slightly more readable.

The idea has been taken from Linux (see include/uapi/linux.h).

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/include/xen/const.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/include/xen/const.h b/xen/include/xen/const.h
index 0d5b2c64f5..b4b067bffa 100644
--- a/xen/include/xen/const.h
+++ b/xen/include/xen/const.h
@@ -21,4 +21,7 @@
 #define _AT(T,X)	((T)(X))
 #endif
 
+#define _BITUL(x)	(_AC(1, UL) << (x))
+#define _BITULL(x)	(_AC(1, ULL) << (x))
+
 #endif /* __XEN_CONST_H__ */
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 01/20] xen/const: Introduce _BITUL and _BITULL

[Julien Grall]

The pattern _AC(1, UL{,L}) << X is commonly used in the headers to make
define usuable in both assembly and C.

So introduce _BITUL and _BITULL to make the code slightly more readable.

The idea has been taken from Linux (see include/uapi/linux.h).

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/include/xen/const.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/include/xen/const.h b/xen/include/xen/const.h
index 0d5b2c64f5..b4b067bffa 100644
--- a/xen/include/xen/const.h
+++ b/xen/include/xen/const.h
@@ -21,4 +21,7 @@
 #define _AT(T,X)	((T)(X))
 #endif
 
+#define _BITUL(x)	(_AC(1, UL) << (x))
+#define _BITULL(x)	(_AC(1, ULL) << (x))
+
 #endif /* __XEN_CONST_H__ */
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 02/20] xen/arm: Rename SCTLR_* defines and remove unused one

[Julien Grall]

The SCTLR_* are currently used for SCTLR/HSCTLR (arm32) and
SCTLR_EL1/SCTLR_EL2 (arm64).

The naming scheme is actually quite confusing because they may only be
defined for an archicture (or even an exception level). So it is not easy
for the developer to know which one to use.

The naming scheme is reworked by adding Axx_ELx in each define:
    * xx is replaced by 32 or 64 if specific to an architecture
    * x is replaced by 2 (hypervisor) or 1 (kernel) if specific to an
    exception level

While doing the renaming, remove the unused defines (or at least the ones
that are unlikely going to be used).

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm32/head.S       |  5 +++--
 xen/arch/arm/arm64/head.S       |  4 ++--
 xen/arch/arm/guest_walk.c       |  2 +-
 xen/arch/arm/mm.c               |  2 +-
 xen/arch/arm/traps.c            |  6 +++---
 xen/include/asm-arm/p2m.h       |  4 +++-
 xen/include/asm-arm/processor.h | 37 +++++++++++++++++--------------------
 7 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 390a505e05..5ef7e5a2f3 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -244,7 +244,7 @@ cpu_init_done:
          * Alignment checking enabled,
          * MMU translation disabled (for now).
          */
-        ldr   r0, =(HSCTLR_BASE|SCTLR_A)
+        ldr   r0, =(HSCTLR_BASE|SCTLR_AXX_A)
         mcr   CP32(r0, HSCTLR)
 
         /*
@@ -369,7 +369,8 @@ virtphys_clash:
 
         ldr   r1, =paging            /* Explicit vaddr, not RIP-relative */
         mrc   CP32(r0, HSCTLR)
-        orr   r0, r0, #(SCTLR_M|SCTLR_C) /* Enable MMU and D-cache */
+        /* Enable MMU and D-cache */
+        orr   r0, r0, #(SCTLR_Axx_ELx_M|SCTLR_Axx_ELx_C)
         dsb                          /* Flush PTE writes and finish reads */
         mcr   CP32(r0, HSCTLR)       /* now paging is enabled */
         isb                          /* Now, flush the icache */
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 4589a37874..8a6be3352e 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -514,8 +514,8 @@ virtphys_clash:
 
         ldr   x1, =paging            /* Explicit vaddr, not RIP-relative */
         mrs   x0, SCTLR_EL2
-        orr   x0, x0, #SCTLR_M       /* Enable MMU */
-        orr   x0, x0, #SCTLR_C       /* Enable D-cache */
+        orr   x0, x0, #SCTLR_Axx_ELx_M  /* Enable MMU */
+        orr   x0, x0, #SCTLR_Axx_ELx_C  /* Enable D-cache */
         dsb   sy                     /* Flush PTE writes and finish reads */
         msr   SCTLR_EL2, x0          /* now paging is enabled */
         isb                          /* Now, flush the icache */
diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index 1bee198777..54065ac4b6 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -612,7 +612,7 @@ bool guest_walk_tables(const struct vcpu *v, vaddr_t gva,
     *perms = GV2M_READ;
 
     /* If the MMU is disabled, there is no need to translate the gva. */
-    if ( !(sctlr & SCTLR_M) )
+    if ( !(sctlr & SCTLR_Axx_ELx_M) )
     {
         *ipa = gva;
 
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 9d584e4cbf..e090afb976 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -609,7 +609,7 @@ void __init remove_early_mappings(void)
  */
 static void xen_pt_enforce_wnx(void)
 {
-    WRITE_SYSREG32(READ_SYSREG32(SCTLR_EL2) | SCTLR_WXN, SCTLR_EL2);
+    WRITE_SYSREG32(READ_SYSREG32(SCTLR_EL2) | SCTLR_Axx_ELx_WXN, SCTLR_EL2);
     /*
      * The TLBs may cache SCTLR_EL2.WXN. So ensure it is synchronized
      * before flushing the TLBs.
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 1aba970415..6c757c12a8 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -392,9 +392,9 @@ static void cpsr_switch_mode(struct cpu_user_regs *regs, int mode)
     regs->cpsr |= PSR_IRQ_MASK;
     if ( mode == PSR_MODE_ABT )
         regs->cpsr |= PSR_ABT_MASK;
-    if ( sctlr & SCTLR_TE )
+    if ( sctlr & SCTLR_A32_ELx_TE )
         regs->cpsr |= PSR_THUMB;
-    if ( sctlr & SCTLR_EE )
+    if ( sctlr & SCTLR_Axx_ELx_EE )
         regs->cpsr |= PSR_BIG_ENDIAN;
 }
 
@@ -402,7 +402,7 @@ static vaddr_t exception_handler32(vaddr_t offset)
 {
     uint32_t sctlr = READ_SYSREG32(SCTLR_EL1);
 
-    if ( sctlr & SCTLR_V )
+    if ( sctlr & SCTLR_A32_EL1_V )
         return 0xffff0000 + offset;
     else /* always have security exceptions */
         return READ_SYSREG(VBAR_EL1) + offset;
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 041dea827c..2f89bb00c3 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -391,10 +391,12 @@ static inline int set_foreign_p2m_entry(struct domain *d, unsigned long gfn,
  */
 static inline bool vcpu_has_cache_enabled(struct vcpu *v)
 {
+    const uint32_t mask = SCTLR_Axx_ELx_C | SCTLR_Axx_ELx_M;
+
     /* Only works with the current vCPU */
     ASSERT(current == v);
 
-    return (READ_SYSREG32(SCTLR_EL1) & (SCTLR_C|SCTLR_M)) == (SCTLR_C|SCTLR_M);
+    return (READ_SYSREG32(SCTLR_EL1) & mask) == mask;
 }
 
 #endif /* _XEN_P2M_H */
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index b5f515805d..c6f56490b3 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -112,26 +112,23 @@
 #define TTBCR_PD1       (_AC(1,U)<<5)
 
 /* SCTLR System Control Register. */
-/* HSCTLR is a subset of this. */
-#define SCTLR_TE        (_AC(1,U)<<30)
-#define SCTLR_AFE       (_AC(1,U)<<29)
-#define SCTLR_TRE       (_AC(1,U)<<28)
-#define SCTLR_NMFI      (_AC(1,U)<<27)
-#define SCTLR_EE        (_AC(1,U)<<25)
-#define SCTLR_VE        (_AC(1,U)<<24)
-#define SCTLR_U         (_AC(1,U)<<22)
-#define SCTLR_FI        (_AC(1,U)<<21)
-#define SCTLR_WXN       (_AC(1,U)<<19)
-#define SCTLR_HA        (_AC(1,U)<<17)
-#define SCTLR_RR        (_AC(1,U)<<14)
-#define SCTLR_V         (_AC(1,U)<<13)
-#define SCTLR_I         (_AC(1,U)<<12)
-#define SCTLR_Z         (_AC(1,U)<<11)
-#define SCTLR_SW        (_AC(1,U)<<10)
-#define SCTLR_B         (_AC(1,U)<<7)
-#define SCTLR_C         (_AC(1,U)<<2)
-#define SCTLR_A         (_AC(1,U)<<1)
-#define SCTLR_M         (_AC(1,U)<<0)
+
+/* Bits specific to SCTLR_EL1 for Arm32 */
+
+#define SCTLR_A32_EL1_V     (_AC(1,U)<<13)
+
+/* Common bits for SCTLR_ELx for Arm32 */
+
+#define SCTLR_A32_ELx_TE    (_AC(1,U)<<30)
+#define SCTLR_A32_ELx_FI    (_AC(1,U)<<21)
+
+/* Common bits for SCTLR_ELx on all architectures */
+#define SCTLR_Axx_ELx_EE    (_AC(1,U)<<25)
+#define SCTLR_Axx_ELx_WXN   (_AC(1,U)<<19)
+#define SCTLR_Axx_ELx_I     (_AC(1,U)<<12)
+#define SCTLR_Axx_ELx_C     (_AC(1,U)<<2)
+#define SCTLR_Axx_ELx_A     (_AC(1,U)<<1)
+#define SCTLR_Axx_ELx_M     (_AC(1,U)<<0)
 
 #define HSCTLR_BASE     _AC(0x30c51878,U)
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 02/20] xen/arm: Rename SCTLR_* defines and remove unused one

[Julien Grall]

The SCTLR_* are currently used for SCTLR/HSCTLR (arm32) and
SCTLR_EL1/SCTLR_EL2 (arm64).

The naming scheme is actually quite confusing because they may only be
defined for an archicture (or even an exception level). So it is not easy
for the developer to know which one to use.

The naming scheme is reworked by adding Axx_ELx in each define:
    * xx is replaced by 32 or 64 if specific to an architecture
    * x is replaced by 2 (hypervisor) or 1 (kernel) if specific to an
    exception level

While doing the renaming, remove the unused defines (or at least the ones
that are unlikely going to be used).

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm32/head.S       |  5 +++--
 xen/arch/arm/arm64/head.S       |  4 ++--
 xen/arch/arm/guest_walk.c       |  2 +-
 xen/arch/arm/mm.c               |  2 +-
 xen/arch/arm/traps.c            |  6 +++---
 xen/include/asm-arm/p2m.h       |  4 +++-
 xen/include/asm-arm/processor.h | 37 +++++++++++++++++--------------------
 7 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 390a505e05..5ef7e5a2f3 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -244,7 +244,7 @@ cpu_init_done:
          * Alignment checking enabled,
          * MMU translation disabled (for now).
          */
-        ldr   r0, =(HSCTLR_BASE|SCTLR_A)
+        ldr   r0, =(HSCTLR_BASE|SCTLR_AXX_A)
         mcr   CP32(r0, HSCTLR)
 
         /*
@@ -369,7 +369,8 @@ virtphys_clash:
 
         ldr   r1, =paging            /* Explicit vaddr, not RIP-relative */
         mrc   CP32(r0, HSCTLR)
-        orr   r0, r0, #(SCTLR_M|SCTLR_C) /* Enable MMU and D-cache */
+        /* Enable MMU and D-cache */
+        orr   r0, r0, #(SCTLR_Axx_ELx_M|SCTLR_Axx_ELx_C)
         dsb                          /* Flush PTE writes and finish reads */
         mcr   CP32(r0, HSCTLR)       /* now paging is enabled */
         isb                          /* Now, flush the icache */
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 4589a37874..8a6be3352e 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -514,8 +514,8 @@ virtphys_clash:
 
         ldr   x1, =paging            /* Explicit vaddr, not RIP-relative */
         mrs   x0, SCTLR_EL2
-        orr   x0, x0, #SCTLR_M       /* Enable MMU */
-        orr   x0, x0, #SCTLR_C       /* Enable D-cache */
+        orr   x0, x0, #SCTLR_Axx_ELx_M  /* Enable MMU */
+        orr   x0, x0, #SCTLR_Axx_ELx_C  /* Enable D-cache */
         dsb   sy                     /* Flush PTE writes and finish reads */
         msr   SCTLR_EL2, x0          /* now paging is enabled */
         isb                          /* Now, flush the icache */
diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index 1bee198777..54065ac4b6 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -612,7 +612,7 @@ bool guest_walk_tables(const struct vcpu *v, vaddr_t gva,
     *perms = GV2M_READ;
 
     /* If the MMU is disabled, there is no need to translate the gva. */
-    if ( !(sctlr & SCTLR_M) )
+    if ( !(sctlr & SCTLR_Axx_ELx_M) )
     {
         *ipa = gva;
 
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 9d584e4cbf..e090afb976 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -609,7 +609,7 @@ void __init remove_early_mappings(void)
  */
 static void xen_pt_enforce_wnx(void)
 {
-    WRITE_SYSREG32(READ_SYSREG32(SCTLR_EL2) | SCTLR_WXN, SCTLR_EL2);
+    WRITE_SYSREG32(READ_SYSREG32(SCTLR_EL2) | SCTLR_Axx_ELx_WXN, SCTLR_EL2);
     /*
      * The TLBs may cache SCTLR_EL2.WXN. So ensure it is synchronized
      * before flushing the TLBs.
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 1aba970415..6c757c12a8 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -392,9 +392,9 @@ static void cpsr_switch_mode(struct cpu_user_regs *regs, int mode)
     regs->cpsr |= PSR_IRQ_MASK;
     if ( mode == PSR_MODE_ABT )
         regs->cpsr |= PSR_ABT_MASK;
-    if ( sctlr & SCTLR_TE )
+    if ( sctlr & SCTLR_A32_ELx_TE )
         regs->cpsr |= PSR_THUMB;
-    if ( sctlr & SCTLR_EE )
+    if ( sctlr & SCTLR_Axx_ELx_EE )
         regs->cpsr |= PSR_BIG_ENDIAN;
 }
 
@@ -402,7 +402,7 @@ static vaddr_t exception_handler32(vaddr_t offset)
 {
     uint32_t sctlr = READ_SYSREG32(SCTLR_EL1);
 
-    if ( sctlr & SCTLR_V )
+    if ( sctlr & SCTLR_A32_EL1_V )
         return 0xffff0000 + offset;
     else /* always have security exceptions */
         return READ_SYSREG(VBAR_EL1) + offset;
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 041dea827c..2f89bb00c3 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -391,10 +391,12 @@ static inline int set_foreign_p2m_entry(struct domain *d, unsigned long gfn,
  */
 static inline bool vcpu_has_cache_enabled(struct vcpu *v)
 {
+    const uint32_t mask = SCTLR_Axx_ELx_C | SCTLR_Axx_ELx_M;
+
     /* Only works with the current vCPU */
     ASSERT(current == v);
 
-    return (READ_SYSREG32(SCTLR_EL1) & (SCTLR_C|SCTLR_M)) == (SCTLR_C|SCTLR_M);
+    return (READ_SYSREG32(SCTLR_EL1) & mask) == mask;
 }
 
 #endif /* _XEN_P2M_H */
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index b5f515805d..c6f56490b3 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -112,26 +112,23 @@
 #define TTBCR_PD1       (_AC(1,U)<<5)
 
 /* SCTLR System Control Register. */
-/* HSCTLR is a subset of this. */
-#define SCTLR_TE        (_AC(1,U)<<30)
-#define SCTLR_AFE       (_AC(1,U)<<29)
-#define SCTLR_TRE       (_AC(1,U)<<28)
-#define SCTLR_NMFI      (_AC(1,U)<<27)
-#define SCTLR_EE        (_AC(1,U)<<25)
-#define SCTLR_VE        (_AC(1,U)<<24)
-#define SCTLR_U         (_AC(1,U)<<22)
-#define SCTLR_FI        (_AC(1,U)<<21)
-#define SCTLR_WXN       (_AC(1,U)<<19)
-#define SCTLR_HA        (_AC(1,U)<<17)
-#define SCTLR_RR        (_AC(1,U)<<14)
-#define SCTLR_V         (_AC(1,U)<<13)
-#define SCTLR_I         (_AC(1,U)<<12)
-#define SCTLR_Z         (_AC(1,U)<<11)
-#define SCTLR_SW        (_AC(1,U)<<10)
-#define SCTLR_B         (_AC(1,U)<<7)
-#define SCTLR_C         (_AC(1,U)<<2)
-#define SCTLR_A         (_AC(1,U)<<1)
-#define SCTLR_M         (_AC(1,U)<<0)
+
+/* Bits specific to SCTLR_EL1 for Arm32 */
+
+#define SCTLR_A32_EL1_V     (_AC(1,U)<<13)
+
+/* Common bits for SCTLR_ELx for Arm32 */
+
+#define SCTLR_A32_ELx_TE    (_AC(1,U)<<30)
+#define SCTLR_A32_ELx_FI    (_AC(1,U)<<21)
+
+/* Common bits for SCTLR_ELx on all architectures */
+#define SCTLR_Axx_ELx_EE    (_AC(1,U)<<25)
+#define SCTLR_Axx_ELx_WXN   (_AC(1,U)<<19)
+#define SCTLR_Axx_ELx_I     (_AC(1,U)<<12)
+#define SCTLR_Axx_ELx_C     (_AC(1,U)<<2)
+#define SCTLR_Axx_ELx_A     (_AC(1,U)<<1)
+#define SCTLR_Axx_ELx_M     (_AC(1,U)<<0)
 
 #define HSCTLR_BASE     _AC(0x30c51878,U)
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 03/20] xen/arm: processor: Use _BITUL instead of _AC(1, U) in SCTLR_ defines

[Julien Grall]

The newly introduced macro _BITUL makes the code more readable.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/include/asm-arm/processor.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index c6f56490b3..1a143fb6a3 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -115,20 +115,20 @@
 
 /* Bits specific to SCTLR_EL1 for Arm32 */
 
-#define SCTLR_A32_EL1_V     (_AC(1,U)<<13)
+#define SCTLR_A32_EL1_V     _BITUL(13)
 
 /* Common bits for SCTLR_ELx for Arm32 */
 
-#define SCTLR_A32_ELx_TE    (_AC(1,U)<<30)
-#define SCTLR_A32_ELx_FI    (_AC(1,U)<<21)
+#define SCTLR_A32_ELx_TE    _BITUL(30)
+#define SCTLR_A32_ELx_FI    _BITUL(21)
 
 /* Common bits for SCTLR_ELx on all architectures */
-#define SCTLR_Axx_ELx_EE    (_AC(1,U)<<25)
-#define SCTLR_Axx_ELx_WXN   (_AC(1,U)<<19)
-#define SCTLR_Axx_ELx_I     (_AC(1,U)<<12)
-#define SCTLR_Axx_ELx_C     (_AC(1,U)<<2)
-#define SCTLR_Axx_ELx_A     (_AC(1,U)<<1)
-#define SCTLR_Axx_ELx_M     (_AC(1,U)<<0)
+#define SCTLR_Axx_ELx_EE    _BITUL(25)
+#define SCTLR_Axx_ELx_WXN   _BITUL(19)
+#define SCTLR_Axx_ELx_I     _BITUL(12)
+#define SCTLR_Axx_ELx_C     _BITUL(2)
+#define SCTLR_Axx_ELx_A     _BITUL(1)
+#define SCTLR_Axx_ELx_M     _BITUL(0)
 
 #define HSCTLR_BASE     _AC(0x30c51878,U)
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 03/20] xen/arm: processor: Use _BITUL instead of _AC(1, U) in SCTLR_ defines

[Julien Grall]

The newly introduced macro _BITUL makes the code more readable.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/include/asm-arm/processor.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index c6f56490b3..1a143fb6a3 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -115,20 +115,20 @@
 
 /* Bits specific to SCTLR_EL1 for Arm32 */
 
-#define SCTLR_A32_EL1_V     (_AC(1,U)<<13)
+#define SCTLR_A32_EL1_V     _BITUL(13)
 
 /* Common bits for SCTLR_ELx for Arm32 */
 
-#define SCTLR_A32_ELx_TE    (_AC(1,U)<<30)
-#define SCTLR_A32_ELx_FI    (_AC(1,U)<<21)
+#define SCTLR_A32_ELx_TE    _BITUL(30)
+#define SCTLR_A32_ELx_FI    _BITUL(21)
 
 /* Common bits for SCTLR_ELx on all architectures */
-#define SCTLR_Axx_ELx_EE    (_AC(1,U)<<25)
-#define SCTLR_Axx_ELx_WXN   (_AC(1,U)<<19)
-#define SCTLR_Axx_ELx_I     (_AC(1,U)<<12)
-#define SCTLR_Axx_ELx_C     (_AC(1,U)<<2)
-#define SCTLR_Axx_ELx_A     (_AC(1,U)<<1)
-#define SCTLR_Axx_ELx_M     (_AC(1,U)<<0)
+#define SCTLR_Axx_ELx_EE    _BITUL(25)
+#define SCTLR_Axx_ELx_WXN   _BITUL(19)
+#define SCTLR_Axx_ELx_I     _BITUL(12)
+#define SCTLR_Axx_ELx_C     _BITUL(2)
+#define SCTLR_Axx_ELx_A     _BITUL(1)
+#define SCTLR_Axx_ELx_M     _BITUL(0)
 
 #define HSCTLR_BASE     _AC(0x30c51878,U)
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 04/20] xen/arm: Rework HSCTLR_BASE

[Julien Grall]

The current value of HSCTLR_BASE for Arm64 is pretty wrong. It would
actually turn on SCTLR_EL2.nAA (bit 6) on hardware implementing
ARMv8.4-LSE.

Furthermore, the documentation of what is cleared/set in SCTLR_EL2 is
also not correct and looks like to be a verbatim copy from Arm32.

HSCTLR_BASE is replaced with a bunch of per-architecture new defines
helping to understand better what is the initialie value for
SCTLR_EL2/HSCTLR.

Note the defines *_CLEAR are only used to check the state of each bits
are known.

Lastly, the documentation is dropped from arm{32,64}/head.S as it would
be pretty easy to get out-of-sync with the definitions.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm32/head.S       | 12 +---------
 xen/arch/arm/arm64/head.S       | 10 +-------
 xen/include/asm-arm/processor.h | 52 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 53 insertions(+), 21 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 5ef7e5a2f3..8a98607459 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -234,17 +234,7 @@ cpu_init_done:
         ldr   r0, =(TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(0))
         mcr   CP32(r0, HTCR)
 
-        /*
-         * Set up the HSCTLR:
-         * Exceptions in LE ARM,
-         * Low-latency IRQs disabled,
-         * Write-implies-XN disabled (for now),
-         * D-cache disabled (for now),
-         * I-cache enabled,
-         * Alignment checking enabled,
-         * MMU translation disabled (for now).
-         */
-        ldr   r0, =(HSCTLR_BASE|SCTLR_AXX_A)
+        ldr   r0, =HSCTLR_SET
         mcr   CP32(r0, HSCTLR)
 
         /*
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 8a6be3352e..4fe904c51d 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -363,15 +363,7 @@ skip_bss:
 
         msr   tcr_el2, x0
 
-        /* Set up the SCTLR_EL2:
-         * Exceptions in LE ARM,
-         * Low-latency IRQs disabled,
-         * Write-implies-XN disabled (for now),
-         * D-cache disabled (for now),
-         * I-cache enabled,
-         * Alignment checking disabled,
-         * MMU translation disabled (for now). */
-        ldr   x0, =(HSCTLR_BASE)
+        ldr   x0, =SCTLR_EL2_SET
         msr   SCTLR_EL2, x0
 
         /* Ensure that any exceptions encountered at EL2
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 1a143fb6a3..6dac500e40 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -122,6 +122,9 @@
 #define SCTLR_A32_ELx_TE    _BITUL(30)
 #define SCTLR_A32_ELx_FI    _BITUL(21)
 
+/* Common bits for SCTLR_ELx for Arm64 */
+#define SCTLR_A64_ELx_SA    _BITUL(3)
+
 /* Common bits for SCTLR_ELx on all architectures */
 #define SCTLR_Axx_ELx_EE    _BITUL(25)
 #define SCTLR_Axx_ELx_WXN   _BITUL(19)
@@ -130,7 +133,54 @@
 #define SCTLR_Axx_ELx_A     _BITUL(1)
 #define SCTLR_Axx_ELx_M     _BITUL(0)
 
-#define HSCTLR_BASE     _AC(0x30c51878,U)
+#ifdef CONFIG_ARM_32
+
+#define HSCTLR_RES1     (_BITUL(3)  | _BITUL(4)  | _BITUL(5)  | _BITUL(6)  |\
+                         _BITUL(11) | _BITUL(16) | _BITUL(18) | _BITUL(22) |\
+                         _BITUL(23) | _BITUL(28) | _BITUL(29))
+
+#define HSCTLR_RES0     (_BITUL(7)  | _BITUL(8)  | _BITUL(9)  | _BITUL(10) |\
+                         _BITUL(13) | _BITUL(14) | _BITUL(15) | _BITUL(17) |\
+                         _BITUL(20) | _BITUL(24) | _BITUL(26) | _BITUL(27) |\
+                         _BITUL(31))
+
+/* Initial value for HSCTLR */
+#define HSCTLR_SET      (HSCTLR_RES1    | SCTLR_Axx_ELx_A   | SCTLR_Axx_ELx_I)
+
+#define HSCTLR_CLEAR    (HSCTLR_RES0        | SCTLR_Axx_ELx_M   |\
+                         SCTLR_Axx_ELx_C    | SCTLR_Axx_ELx_WXN |\
+                         SCTLR_A32_ELx_FI   | SCTLR_Axx_ELx_EE  |\
+                         SCTLR_A32_ELx_TE)
+
+#if (HSCTLR_SET ^ HSCTLR_CLEAR) != 0xffffffffU
+#error "Inconsistent HSCTLR set/clear bits"
+#endif
+
+#else
+
+#define SCTLR_EL2_RES1  (_BITUL(4)  | _BITUL(5)  | _BITUL(11) | _BITUL(16) |\
+                         _BITUL(18) | _BITUL(22) | _BITUL(23) | _BITUL(28) |\
+                         _BITUL(29))
+
+#define SCTLR_EL2_RES0  (_BITUL(6)  | _BITUL(7)  | _BITUL(8)  | _BITUL(9)  |\
+                         _BITUL(10) | _BITUL(13) | _BITUL(14) | _BITUL(15) |\
+                         _BITUL(17) | _BITUL(20) | _BITUL(21) | _BITUL(24) |\
+                         _BITUL(26) | _BITUL(27) | _BITUL(30) | _BITUL(31) |\
+                         (0xffffffffULL << 32))
+
+/* Initial value for SCTLR_EL2 */
+#define SCTLR_EL2_SET   (SCTLR_EL2_RES1     | SCTLR_A64_ELx_SA  |\
+                         SCTLR_Axx_ELx_I)
+
+#define SCTLR_EL2_CLEAR (SCTLR_EL2_RES0     | SCTLR_Axx_ELx_M   |\
+                         SCTLR_Axx_ELx_A    | SCTLR_Axx_ELx_C   |\
+                         SCTLR_Axx_ELx_WXN  | SCTLR_Axx_ELx_EE)
+
+#if (SCTLR_EL2_SET ^ SCTLR_EL2_CLEAR) != 0xffffffffffffffffUL
+#error "Inconsistent SCTLR_EL2 set/clear bits"
+#endif
+
+#endif
 
 /* HCR Hyp Configuration Register */
 #define HCR_RW          (_AC(1,UL)<<31) /* Register Width, ARM64 only */
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 04/20] xen/arm: Rework HSCTLR_BASE

[Julien Grall]

The current value of HSCTLR_BASE for Arm64 is pretty wrong. It would
actually turn on SCTLR_EL2.nAA (bit 6) on hardware implementing
ARMv8.4-LSE.

Furthermore, the documentation of what is cleared/set in SCTLR_EL2 is
also not correct and looks like to be a verbatim copy from Arm32.

HSCTLR_BASE is replaced with a bunch of per-architecture new defines
helping to understand better what is the initialie value for
SCTLR_EL2/HSCTLR.

Note the defines *_CLEAR are only used to check the state of each bits
are known.

Lastly, the documentation is dropped from arm{32,64}/head.S as it would
be pretty easy to get out-of-sync with the definitions.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm32/head.S       | 12 +---------
 xen/arch/arm/arm64/head.S       | 10 +-------
 xen/include/asm-arm/processor.h | 52 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 53 insertions(+), 21 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 5ef7e5a2f3..8a98607459 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -234,17 +234,7 @@ cpu_init_done:
         ldr   r0, =(TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(0))
         mcr   CP32(r0, HTCR)
 
-        /*
-         * Set up the HSCTLR:
-         * Exceptions in LE ARM,
-         * Low-latency IRQs disabled,
-         * Write-implies-XN disabled (for now),
-         * D-cache disabled (for now),
-         * I-cache enabled,
-         * Alignment checking enabled,
-         * MMU translation disabled (for now).
-         */
-        ldr   r0, =(HSCTLR_BASE|SCTLR_AXX_A)
+        ldr   r0, =HSCTLR_SET
         mcr   CP32(r0, HSCTLR)
 
         /*
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 8a6be3352e..4fe904c51d 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -363,15 +363,7 @@ skip_bss:
 
         msr   tcr_el2, x0
 
-        /* Set up the SCTLR_EL2:
-         * Exceptions in LE ARM,
-         * Low-latency IRQs disabled,
-         * Write-implies-XN disabled (for now),
-         * D-cache disabled (for now),
-         * I-cache enabled,
-         * Alignment checking disabled,
-         * MMU translation disabled (for now). */
-        ldr   x0, =(HSCTLR_BASE)
+        ldr   x0, =SCTLR_EL2_SET
         msr   SCTLR_EL2, x0
 
         /* Ensure that any exceptions encountered at EL2
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 1a143fb6a3..6dac500e40 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -122,6 +122,9 @@
 #define SCTLR_A32_ELx_TE    _BITUL(30)
 #define SCTLR_A32_ELx_FI    _BITUL(21)
 
+/* Common bits for SCTLR_ELx for Arm64 */
+#define SCTLR_A64_ELx_SA    _BITUL(3)
+
 /* Common bits for SCTLR_ELx on all architectures */
 #define SCTLR_Axx_ELx_EE    _BITUL(25)
 #define SCTLR_Axx_ELx_WXN   _BITUL(19)
@@ -130,7 +133,54 @@
 #define SCTLR_Axx_ELx_A     _BITUL(1)
 #define SCTLR_Axx_ELx_M     _BITUL(0)
 
-#define HSCTLR_BASE     _AC(0x30c51878,U)
+#ifdef CONFIG_ARM_32
+
+#define HSCTLR_RES1     (_BITUL(3)  | _BITUL(4)  | _BITUL(5)  | _BITUL(6)  |\
+                         _BITUL(11) | _BITUL(16) | _BITUL(18) | _BITUL(22) |\
+                         _BITUL(23) | _BITUL(28) | _BITUL(29))
+
+#define HSCTLR_RES0     (_BITUL(7)  | _BITUL(8)  | _BITUL(9)  | _BITUL(10) |\
+                         _BITUL(13) | _BITUL(14) | _BITUL(15) | _BITUL(17) |\
+                         _BITUL(20) | _BITUL(24) | _BITUL(26) | _BITUL(27) |\
+                         _BITUL(31))
+
+/* Initial value for HSCTLR */
+#define HSCTLR_SET      (HSCTLR_RES1    | SCTLR_Axx_ELx_A   | SCTLR_Axx_ELx_I)
+
+#define HSCTLR_CLEAR    (HSCTLR_RES0        | SCTLR_Axx_ELx_M   |\
+                         SCTLR_Axx_ELx_C    | SCTLR_Axx_ELx_WXN |\
+                         SCTLR_A32_ELx_FI   | SCTLR_Axx_ELx_EE  |\
+                         SCTLR_A32_ELx_TE)
+
+#if (HSCTLR_SET ^ HSCTLR_CLEAR) != 0xffffffffU
+#error "Inconsistent HSCTLR set/clear bits"
+#endif
+
+#else
+
+#define SCTLR_EL2_RES1  (_BITUL(4)  | _BITUL(5)  | _BITUL(11) | _BITUL(16) |\
+                         _BITUL(18) | _BITUL(22) | _BITUL(23) | _BITUL(28) |\
+                         _BITUL(29))
+
+#define SCTLR_EL2_RES0  (_BITUL(6)  | _BITUL(7)  | _BITUL(8)  | _BITUL(9)  |\
+                         _BITUL(10) | _BITUL(13) | _BITUL(14) | _BITUL(15) |\
+                         _BITUL(17) | _BITUL(20) | _BITUL(21) | _BITUL(24) |\
+                         _BITUL(26) | _BITUL(27) | _BITUL(30) | _BITUL(31) |\
+                         (0xffffffffULL << 32))
+
+/* Initial value for SCTLR_EL2 */
+#define SCTLR_EL2_SET   (SCTLR_EL2_RES1     | SCTLR_A64_ELx_SA  |\
+                         SCTLR_Axx_ELx_I)
+
+#define SCTLR_EL2_CLEAR (SCTLR_EL2_RES0     | SCTLR_Axx_ELx_M   |\
+                         SCTLR_Axx_ELx_A    | SCTLR_Axx_ELx_C   |\
+                         SCTLR_Axx_ELx_WXN  | SCTLR_Axx_ELx_EE)
+
+#if (SCTLR_EL2_SET ^ SCTLR_EL2_CLEAR) != 0xffffffffffffffffUL
+#error "Inconsistent SCTLR_EL2 set/clear bits"
+#endif
+
+#endif
 
 /* HCR Hyp Configuration Register */
 #define HCR_RW          (_AC(1,UL)<<31) /* Register Width, ARM64 only */
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 05/20] xen/arm: Rework secondary_start prototype

[Julien Grall]

None of the parameters of secondary_start are actually used. So turn
secondary_start to a function with no parameters.

Also modify the assembly code to avoid setting-up the registers before
calling secondary_start.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm32/head.S | 6 +++---
 xen/arch/arm/arm64/head.S | 3 ++-
 xen/arch/arm/smpboot.c    | 4 +---
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 8a98607459..b71d7fb11d 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -445,10 +445,10 @@ launch:
         ldr   sp, [r0]
         add   sp, #STACK_SIZE        /* (which grows down from the top). */
         sub   sp, #CPUINFO_sizeof    /* Make room for CPU save record */
-        mov   r0, r10                /* Marshal args: - phys_offset */
-        mov   r1, r8                 /*               - DTB address */
-        mov   r2, r7                 /*               - CPU ID */
         teq   r12, #0
+        moveq r0, r10                /* Marshal args: - phys_offset */
+        moveq r1, r8                 /*               - DTB address */
+        moveq r2, r7                 /*               - CPU ID */
         beq   start_xen              /* and disappear into the land of C */
         b     start_secondary        /* (to the appropriate entry point) */
 
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 4fe904c51d..b26126de53 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -582,10 +582,11 @@ launch:
         sub   x0, x0, #CPUINFO_sizeof /* Make room for CPU save record */
         mov   sp, x0
 
+        cbnz  x22, 1f
+
         mov   x0, x20                /* Marshal args: - phys_offset */
         mov   x1, x21                /*               - FDT */
         mov   x2, x24                /*               - CPU ID */
-        cbnz  x22, 1f
         b     start_xen              /* and disappear into the land of C */
 1:
         b     start_secondary        /* (to the appropriate entry point) */
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index f756444362..00b64c3322 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -297,9 +297,7 @@ smp_prepare_cpus(void)
 }
 
 /* Boot the current CPU */
-void start_secondary(unsigned long boot_phys_offset,
-                     unsigned long fdt_paddr,
-                     unsigned long hwid)
+void start_secondary(void)
 {
     unsigned int cpuid = init_data.cpuid;
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 05/20] xen/arm: Rework secondary_start prototype

[Julien Grall]

None of the parameters of secondary_start are actually used. So turn
secondary_start to a function with no parameters.

Also modify the assembly code to avoid setting-up the registers before
calling secondary_start.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm32/head.S | 6 +++---
 xen/arch/arm/arm64/head.S | 3 ++-
 xen/arch/arm/smpboot.c    | 4 +---
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 8a98607459..b71d7fb11d 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -445,10 +445,10 @@ launch:
         ldr   sp, [r0]
         add   sp, #STACK_SIZE        /* (which grows down from the top). */
         sub   sp, #CPUINFO_sizeof    /* Make room for CPU save record */
-        mov   r0, r10                /* Marshal args: - phys_offset */
-        mov   r1, r8                 /*               - DTB address */
-        mov   r2, r7                 /*               - CPU ID */
         teq   r12, #0
+        moveq r0, r10                /* Marshal args: - phys_offset */
+        moveq r1, r8                 /*               - DTB address */
+        moveq r2, r7                 /*               - CPU ID */
         beq   start_xen              /* and disappear into the land of C */
         b     start_secondary        /* (to the appropriate entry point) */
 
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 4fe904c51d..b26126de53 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -582,10 +582,11 @@ launch:
         sub   x0, x0, #CPUINFO_sizeof /* Make room for CPU save record */
         mov   sp, x0
 
+        cbnz  x22, 1f
+
         mov   x0, x20                /* Marshal args: - phys_offset */
         mov   x1, x21                /*               - FDT */
         mov   x2, x24                /*               - CPU ID */
-        cbnz  x22, 1f
         b     start_xen              /* and disappear into the land of C */
 1:
         b     start_secondary        /* (to the appropriate entry point) */
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index f756444362..00b64c3322 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -297,9 +297,7 @@ smp_prepare_cpus(void)
 }
 
 /* Boot the current CPU */
-void start_secondary(unsigned long boot_phys_offset,
-                     unsigned long fdt_paddr,
-                     unsigned long hwid)
+void start_secondary(void)
 {
     unsigned int cpuid = init_data.cpuid;
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 06/20] xen/arm: Remove parameter cpuid from start_xen

[Julien Grall]

The parameter cpuid is not used by start_xen. So remove it.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm32/head.S | 1 -
 xen/arch/arm/arm64/head.S | 1 -
 xen/arch/arm/setup.c      | 3 +--
 3 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index b71d7fb11d..9f40face98 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -448,7 +448,6 @@ launch:
         teq   r12, #0
         moveq r0, r10                /* Marshal args: - phys_offset */
         moveq r1, r8                 /*               - DTB address */
-        moveq r2, r7                 /*               - CPU ID */
         beq   start_xen              /* and disappear into the land of C */
         b     start_secondary        /* (to the appropriate entry point) */
 
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index b26126de53..cb30d6f22e 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -586,7 +586,6 @@ launch:
 
         mov   x0, x20                /* Marshal args: - phys_offset */
         mov   x1, x21                /*               - FDT */
-        mov   x2, x24                /*               - CPU ID */
         b     start_xen              /* and disappear into the land of C */
 1:
         b     start_secondary        /* (to the appropriate entry point) */
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index ccb0f181ea..6dfbba2927 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -728,8 +728,7 @@ size_t __read_mostly dcache_line_bytes;
 
 /* C entry point for boot CPU */
 void __init start_xen(unsigned long boot_phys_offset,
-                      unsigned long fdt_paddr,
-                      unsigned long cpuid)
+                      unsigned long fdt_paddr)
 {
     size_t fdt_size;
     int cpus, i;
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 06/20] xen/arm: Remove parameter cpuid from start_xen

[Julien Grall]

The parameter cpuid is not used by start_xen. So remove it.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm32/head.S | 1 -
 xen/arch/arm/arm64/head.S | 1 -
 xen/arch/arm/setup.c      | 3 +--
 3 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index b71d7fb11d..9f40face98 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -448,7 +448,6 @@ launch:
         teq   r12, #0
         moveq r0, r10                /* Marshal args: - phys_offset */
         moveq r1, r8                 /*               - DTB address */
-        moveq r2, r7                 /*               - CPU ID */
         beq   start_xen              /* and disappear into the land of C */
         b     start_secondary        /* (to the appropriate entry point) */
 
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index b26126de53..cb30d6f22e 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -586,7 +586,6 @@ launch:
 
         mov   x0, x20                /* Marshal args: - phys_offset */
         mov   x1, x21                /*               - FDT */
-        mov   x2, x24                /*               - CPU ID */
         b     start_xen              /* and disappear into the land of C */
 1:
         b     start_secondary        /* (to the appropriate entry point) */
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index ccb0f181ea..6dfbba2927 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -728,8 +728,7 @@ size_t __read_mostly dcache_line_bytes;
 
 /* C entry point for boot CPU */
 void __init start_xen(unsigned long boot_phys_offset,
-                      unsigned long fdt_paddr,
-                      unsigned long cpuid)
+                      unsigned long fdt_paddr)
 {
     size_t fdt_size;
     int cpus, i;
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 07/20] xen/arm64: head: Remove unnecessary comment

[Julien Grall]

So far, we don't init specific core initialization at boot. So remove
the comment.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm64/head.S | 2 --
 1 file changed, 2 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index cb30d6f22e..ad446e7345 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -344,8 +344,6 @@ el2:    PRINT("- Xen starting at EL2 -\r\n")
 skip_bss:
         PRINT("- Setting up control registers -\r\n")
 
-        /* XXXX call PROCINFO_cpu_init here */
-
         /* Set up memory attribute type tables */
         ldr   x0, =MAIRVAL
         msr   mair_el2, x0
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 07/20] xen/arm64: head: Remove unnecessary comment

[Julien Grall]

So far, we don't init specific core initialization at boot. So remove
the comment.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm64/head.S | 2 --
 1 file changed, 2 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index cb30d6f22e..ad446e7345 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -344,8 +344,6 @@ el2:    PRINT("- Xen starting at EL2 -\r\n")
 skip_bss:
         PRINT("- Setting up control registers -\r\n")
 
-        /* XXXX call PROCINFO_cpu_init here */
-
         /* Set up memory attribute type tables */
         ldr   x0, =MAIRVAL
         msr   mair_el2, x0
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 08/20] xen/arm64: head: Move earlyprintk messages in .rodata.str

[Julien Grall]

At the moment, the earlyprintk messages are interleaved with the
instructions. This makes more difficult to read the objdump output.

Introduce a new macro to add a string in .rodata.str and use it for all
the earlyprintk messages.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---

I haven't done a similar change in arm32 yet because the compiler will
throw an error when using 'adr' when load an address from a different
section (see A5-200 in ARM DDI 0406C.a for the technical reason).
The change is likely to be more elaborate.
---
 xen/arch/arm/arm64/head.S       | 14 +++++---------
 xen/include/asm-arm/asm_defns.h |  5 +++++
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index ad446e7345..b957eb90fb 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -81,13 +81,10 @@
 /* Macro to print a string to the UART, if there is one.
  * Clobbers x0-x3. */
 #ifdef CONFIG_EARLY_PRINTK
-#define PRINT(_s)       \
-        adr   x0, 98f ; \
-        bl    puts    ; \
-        b     99f     ; \
-98:     .asciz _s     ; \
-        .align 2      ; \
-99:
+#define PRINT(_s)           \
+        adr   x0, 98f ;     \
+        bl    puts    ;     \
+        RODATA_STR(98, _s)
 #else /* CONFIG_EARLY_PRINTK */
 #define PRINT(s)
 #endif /* !CONFIG_EARLY_PRINTK */
@@ -633,8 +630,7 @@ init_uart:
 #endif
         adr   x0, 1f
         b     puts
-1:      .asciz "- UART enabled -\r\n"
-        .align 4
+RODATA_STR(1, "- UART enabled -\r\n")
 
 /* Print early debug messages.
  * x0: Nul-terminated string to print.
diff --git a/xen/include/asm-arm/asm_defns.h b/xen/include/asm-arm/asm_defns.h
index 02be83e2b3..3f21def0ab 100644
--- a/xen/include/asm-arm/asm_defns.h
+++ b/xen/include/asm-arm/asm_defns.h
@@ -16,6 +16,11 @@
 # error "unknown ARM variant"
 #endif
 
+#define RODATA_STR(label, msg)                  \
+.pushsection .rodata.str, "aMS", %progbits, 1 ; \
+label:  .asciz msg;                             \
+.popsection
+
 #endif /* __ARM_ASM_DEFNS_H__ */
 /*
  * Local variables:
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 08/20] xen/arm64: head: Move earlyprintk messages in .rodata.str

[Julien Grall]

At the moment, the earlyprintk messages are interleaved with the
instructions. This makes more difficult to read the objdump output.

Introduce a new macro to add a string in .rodata.str and use it for all
the earlyprintk messages.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---

I haven't done a similar change in arm32 yet because the compiler will
throw an error when using 'adr' when load an address from a different
section (see A5-200 in ARM DDI 0406C.a for the technical reason).
The change is likely to be more elaborate.
---
 xen/arch/arm/arm64/head.S       | 14 +++++---------
 xen/include/asm-arm/asm_defns.h |  5 +++++
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index ad446e7345..b957eb90fb 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -81,13 +81,10 @@
 /* Macro to print a string to the UART, if there is one.
  * Clobbers x0-x3. */
 #ifdef CONFIG_EARLY_PRINTK
-#define PRINT(_s)       \
-        adr   x0, 98f ; \
-        bl    puts    ; \
-        b     99f     ; \
-98:     .asciz _s     ; \
-        .align 2      ; \
-99:
+#define PRINT(_s)           \
+        adr   x0, 98f ;     \
+        bl    puts    ;     \
+        RODATA_STR(98, _s)
 #else /* CONFIG_EARLY_PRINTK */
 #define PRINT(s)
 #endif /* !CONFIG_EARLY_PRINTK */
@@ -633,8 +630,7 @@ init_uart:
 #endif
         adr   x0, 1f
         b     puts
-1:      .asciz "- UART enabled -\r\n"
-        .align 4
+RODATA_STR(1, "- UART enabled -\r\n")
 
 /* Print early debug messages.
  * x0: Nul-terminated string to print.
diff --git a/xen/include/asm-arm/asm_defns.h b/xen/include/asm-arm/asm_defns.h
index 02be83e2b3..3f21def0ab 100644
--- a/xen/include/asm-arm/asm_defns.h
+++ b/xen/include/asm-arm/asm_defns.h
@@ -16,6 +16,11 @@
 # error "unknown ARM variant"
 #endif
 
+#define RODATA_STR(label, msg)                  \
+.pushsection .rodata.str, "aMS", %progbits, 1 ; \
+label:  .asciz msg;                             \
+.popsection
+
 #endif /* __ARM_ASM_DEFNS_H__ */
 /*
  * Local variables:
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 09/20] xen/arm64: head: Correctly report the HW CPU ID

[Julien Grall]

There are no reason to consider the HW CPU ID will be 0 when the
processor is part of a uniprocessor system. At best, this will result to
conflicting output as the rest of Xen use the value directly read from
MPIDR_EL1.

So remove the zeroing and logic to check if the CPU is part of a
uniprocessor system.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm64/head.S | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index b957eb90fb..08094a273e 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -277,15 +277,9 @@ GLOBAL(init_secondary)
         mov   x26, #1                /* X26 := skip_zero_bss */
 
 common_start:
-        mov   x24, #0                /* x24 := CPU ID. Initialy zero until we
-                                      * find that multiprocessor extensions are
-                                      * present and the system is SMP  */
         mrs   x0, mpidr_el1
-        tbnz  x0, _MPIDR_UP, 1f      /* Uniprocessor system? */
-
         ldr   x13, =(~MPIDR_HWID_MASK)
         bic   x24, x0, x13           /* Mask out flags to get CPU ID */
-1:
 
         /* Non-boot CPUs wait here until __cpu_up is ready for them */
         cbz   x22, 1f
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 09/20] xen/arm64: head: Correctly report the HW CPU ID

[Julien Grall]

There are no reason to consider the HW CPU ID will be 0 when the
processor is part of a uniprocessor system. At best, this will result to
conflicting output as the rest of Xen use the value directly read from
MPIDR_EL1.

So remove the zeroing and logic to check if the CPU is part of a
uniprocessor system.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm64/head.S | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index b957eb90fb..08094a273e 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -277,15 +277,9 @@ GLOBAL(init_secondary)
         mov   x26, #1                /* X26 := skip_zero_bss */
 
 common_start:
-        mov   x24, #0                /* x24 := CPU ID. Initialy zero until we
-                                      * find that multiprocessor extensions are
-                                      * present and the system is SMP  */
         mrs   x0, mpidr_el1
-        tbnz  x0, _MPIDR_UP, 1f      /* Uniprocessor system? */
-
         ldr   x13, =(~MPIDR_HWID_MASK)
         bic   x24, x0, x13           /* Mask out flags to get CPU ID */
-1:
 
         /* Non-boot CPUs wait here until __cpu_up is ready for them */
         cbz   x22, 1f
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 10/20] xen/arm32: head: Correctly report the HW CPU ID

[Julien Grall]

There are no reason to consider the HW CPU ID will be 0 when the
processor is part of a uniprocessor system. At best, this will result to
conflicting output as the rest of Xen use the value directly read from
MPIDR.

So remove the zeroing and logic to check if the CPU is part of a
uniprocessor system.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm32/head.S | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 9f40face98..d42a13556c 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -124,16 +124,8 @@ GLOBAL(init_secondary)
         mov   r12, #1                /* r12 := is_secondary_cpu */
 
 common_start:
-        mov   r7, #0                 /* r7 := CPU ID. Initialy zero until we
-                                      * find that multiprocessor extensions are
-                                      * present and the system is SMP */
         mrc   CP32(r1, MPIDR)
-        tst   r1, #MPIDR_SMP         /* Multiprocessor extension supported? */
-        beq   1f
-        tst   r1, #MPIDR_UP          /* Uniprocessor system? */
-        bne   1f
         bic   r7, r1, #(~MPIDR_HWID_MASK) /* Mask out flags to get CPU ID */
-1:
 
         /* Non-boot CPUs wait here until __cpu_up is ready for them */
         teq   r12, #0
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 10/20] xen/arm32: head: Correctly report the HW CPU ID

[Julien Grall]

There are no reason to consider the HW CPU ID will be 0 when the
processor is part of a uniprocessor system. At best, this will result to
conflicting output as the rest of Xen use the value directly read from
MPIDR.

So remove the zeroing and logic to check if the CPU is part of a
uniprocessor system.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm32/head.S | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 9f40face98..d42a13556c 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -124,16 +124,8 @@ GLOBAL(init_secondary)
         mov   r12, #1                /* r12 := is_secondary_cpu */
 
 common_start:
-        mov   r7, #0                 /* r7 := CPU ID. Initialy zero until we
-                                      * find that multiprocessor extensions are
-                                      * present and the system is SMP */
         mrc   CP32(r1, MPIDR)
-        tst   r1, #MPIDR_SMP         /* Multiprocessor extension supported? */
-        beq   1f
-        tst   r1, #MPIDR_UP          /* Uniprocessor system? */
-        bne   1f
         bic   r7, r1, #(~MPIDR_HWID_MASK) /* Mask out flags to get CPU ID */
-1:
 
         /* Non-boot CPUs wait here until __cpu_up is ready for them */
         teq   r12, #0
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 11/20] xen/arm32: head: Don't set MAIR0 and MAIR1

[Julien Grall]

The co-processor registers MAIR0 and MAIR1 are managed by EL1. So there
are no need to initialize them during Xen boot.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm32/head.S | 2 --
 1 file changed, 2 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index d42a13556c..3448817aab 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -212,8 +212,6 @@ cpu_init_done:
         /* Set up memory attribute type tables */
         ldr   r0, =MAIR0VAL
         ldr   r1, =MAIR1VAL
-        mcr   CP32(r0, MAIR0)
-        mcr   CP32(r1, MAIR1)
         mcr   CP32(r0, HMAIR0)
         mcr   CP32(r1, HMAIR1)
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 11/20] xen/arm32: head: Don't set MAIR0 and MAIR1

[Julien Grall]

The co-processor registers MAIR0 and MAIR1 are managed by EL1. So there
are no need to initialize them during Xen boot.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm32/head.S | 2 --
 1 file changed, 2 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index d42a13556c..3448817aab 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -212,8 +212,6 @@ cpu_init_done:
         /* Set up memory attribute type tables */
         ldr   r0, =MAIR0VAL
         ldr   r1, =MAIR1VAL
-        mcr   CP32(r0, MAIR0)
-        mcr   CP32(r1, MAIR1)
         mcr   CP32(r0, HMAIR0)
         mcr   CP32(r1, HMAIR1)
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 12/20] xen/arm32: head: Always zero r3 before update a page-table entry

[Julien Grall]

The boot code is using r2 and r3 to hold the page-table entry value.
While r2 is always updated before storing the value, this is not always
the case for r3.

Thankfully today, r3 will always be zero when we care. But this is
difficult to track and error-prone.

So always zero r3 within the few instructions before the write the
page-table entry.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm32/head.S | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 3448817aab..0536b62aec 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -270,6 +270,7 @@ cpu_init_done:
         orr   r2, r2, #PT_UPPER(MEM) /* r2:r3 := section map */
         orr   r2, r2, #PT_LOWER(MEM)
         lsl   r1, r1, #3             /* r1 := Slot offset */
+        mov   r3, #0x0
         strd  r2, r3, [r4, r1]       /* Mapping of paddr(start) */
         mov   r6, #1                 /* r6 := identity map now in place */
 
@@ -377,6 +378,7 @@ paging:
         lsl   r2, r2, #THIRD_SHIFT   /* 4K aligned paddr of UART */
         orr   r2, r2, #PT_UPPER(DEV_L3)
         orr   r2, r2, #PT_LOWER(DEV_L3) /* r2:r3 := 4K dev map including UART */
+        mov   r3, #0
         strd  r2, r3, [r1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */
 1:
 
@@ -388,6 +390,7 @@ paging:
         orr   r2, r2, #PT_LOWER(PT)  /* r2:r3 := table map of xen_fixmap */
         ldr   r4, =FIXMAP_ADDR(0)
         mov   r4, r4, lsr #(SECOND_SHIFT - 3)   /* r4 := Slot for FIXMAP(0) */
+        mov   r3, #0
         strd  r2, r3, [r1, r4]       /* Map it in the fixmap's slot */
 
         /* Use a virtual address to access the UART. */
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 12/20] xen/arm32: head: Always zero r3 before update a page-table entry

[Julien Grall]

The boot code is using r2 and r3 to hold the page-table entry value.
While r2 is always updated before storing the value, this is not always
the case for r3.

Thankfully today, r3 will always be zero when we care. But this is
difficult to track and error-prone.

So always zero r3 within the few instructions before the write the
page-table entry.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm32/head.S | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 3448817aab..0536b62aec 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -270,6 +270,7 @@ cpu_init_done:
         orr   r2, r2, #PT_UPPER(MEM) /* r2:r3 := section map */
         orr   r2, r2, #PT_LOWER(MEM)
         lsl   r1, r1, #3             /* r1 := Slot offset */
+        mov   r3, #0x0
         strd  r2, r3, [r4, r1]       /* Mapping of paddr(start) */
         mov   r6, #1                 /* r6 := identity map now in place */
 
@@ -377,6 +378,7 @@ paging:
         lsl   r2, r2, #THIRD_SHIFT   /* 4K aligned paddr of UART */
         orr   r2, r2, #PT_UPPER(DEV_L3)
         orr   r2, r2, #PT_LOWER(DEV_L3) /* r2:r3 := 4K dev map including UART */
+        mov   r3, #0
         strd  r2, r3, [r1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */
 1:
 
@@ -388,6 +390,7 @@ paging:
         orr   r2, r2, #PT_LOWER(PT)  /* r2:r3 := table map of xen_fixmap */
         ldr   r4, =FIXMAP_ADDR(0)
         mov   r4, r4, lsr #(SECOND_SHIFT - 3)   /* r4 := Slot for FIXMAP(0) */
+        mov   r3, #0
         strd  r2, r3, [r1, r4]       /* Map it in the fixmap's slot */
 
         /* Use a virtual address to access the UART. */
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 13/20] xen/arm32: mm: Avoid to zero and clean cache for CPU0 domheap

[Julien Grall]

The page-table walker is configured to use the same shareability and
cacheability as the access performed when updating the page-tables. This
means cleaning the cache for CPU0 domheap is unnecessary.

Furthermore, CPU0 page-tables are part of Xen binary and will already be
zeroed beforehand. So it is pointless to zero the domheap again.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index e090afb976..cda2847d00 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -724,11 +724,6 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
 #ifdef CONFIG_ARM_32
     per_cpu(xen_pgtable, 0) = cpu0_pgtable;
     per_cpu(xen_dommap, 0) = cpu0_dommap;
-
-    /* Make sure it is clear */
-    memset(this_cpu(xen_dommap), 0, DOMHEAP_SECOND_PAGES*PAGE_SIZE);
-    clean_dcache_va_range(this_cpu(xen_dommap),
-                              DOMHEAP_SECOND_PAGES*PAGE_SIZE);
 #endif
 }
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 13/20] xen/arm32: mm: Avoid to zero and clean cache for CPU0 domheap

[Julien Grall]

The page-table walker is configured to use the same shareability and
cacheability as the access performed when updating the page-tables. This
means cleaning the cache for CPU0 domheap is unnecessary.

Furthermore, CPU0 page-tables are part of Xen binary and will already be
zeroed beforehand. So it is pointless to zero the domheap again.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index e090afb976..cda2847d00 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -724,11 +724,6 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
 #ifdef CONFIG_ARM_32
     per_cpu(xen_pgtable, 0) = cpu0_pgtable;
     per_cpu(xen_dommap, 0) = cpu0_dommap;
-
-    /* Make sure it is clear */
-    memset(this_cpu(xen_dommap), 0, DOMHEAP_SECOND_PAGES*PAGE_SIZE);
-    clean_dcache_va_range(this_cpu(xen_dommap),
-                              DOMHEAP_SECOND_PAGES*PAGE_SIZE);
 #endif
 }
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 14/20] xen/arm32: mm: Avoid cleaning the cache for secondary CPUs page-tables

[Julien Grall]

The page-table walker is configured to use the same shareability and
cacheability as the access performed when updating the page-tables. This
means cleaning the cache for secondary CPUs runtime page-tables is
unnecessary.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index cda2847d00..6db7dda0da 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -769,9 +769,6 @@ int init_secondary_pagetables(int cpu)
         write_pte(&first[first_table_offset(DOMHEAP_VIRT_START+i*FIRST_SIZE)], pte);
     }
 
-    clean_dcache_va_range(first, PAGE_SIZE);
-    clean_dcache_va_range(domheap, DOMHEAP_SECOND_PAGES*PAGE_SIZE);
-
     per_cpu(xen_pgtable, cpu) = first;
     per_cpu(xen_dommap, cpu) = domheap;
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 14/20] xen/arm32: mm: Avoid cleaning the cache for secondary CPUs page-tables

[Julien Grall]

The page-table walker is configured to use the same shareability and
cacheability as the access performed when updating the page-tables. This
means cleaning the cache for secondary CPUs runtime page-tables is
unnecessary.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index cda2847d00..6db7dda0da 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -769,9 +769,6 @@ int init_secondary_pagetables(int cpu)
         write_pte(&first[first_table_offset(DOMHEAP_VIRT_START+i*FIRST_SIZE)], pte);
     }
 
-    clean_dcache_va_range(first, PAGE_SIZE);
-    clean_dcache_va_range(domheap, DOMHEAP_SECOND_PAGES*PAGE_SIZE);
-
     per_cpu(xen_pgtable, cpu) = first;
     per_cpu(xen_dommap, cpu) = domheap;
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 15/20] xen/arm: mm: Use the shorter version __aligned(PAGE_SIZE) to align page-tables

[Julien Grall]

We currently use the very long version __attribute__((__aligned(4096)))
to align page-tables. Thankfully there is a shorter version to make
the code more readable.

While modifying the attribute:
    1) Move it before the variable name as we do in other part of Xen
    2) Switch to PAGE_SIZE instead of 4096 to make more future-proof
    3) Mark static page-tables not used outside the file (i.e any
       page-tables other than boot_* and xen_fixmap).

Lastly, some of the variables use __attribute__(__aligned(X * 4096)).
However this is not necessary as page-tables are only required to be
to be aligned to a page-size. So use __aligned(PAGE_SIZE).

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 6db7dda0da..fa0f41bd07 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -73,13 +73,13 @@ struct domain *dom_xen, *dom_io, *dom_cow;
  * Finally, if EARLY_PRINTK is enabled then xen_fixmap will be mapped
  * by the CPU once it has moved off the 1:1 mapping.
  */
-lpae_t boot_pgtable[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
+lpae_t __aligned(PAGE_SIZE) boot_pgtable[LPAE_ENTRIES];
 #ifdef CONFIG_ARM_64
-lpae_t boot_first[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
-lpae_t boot_first_id[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
+lpae_t __aligned(PAGE_SIZE) boot_first[LPAE_ENTRIES];
+lpae_t __aligned(PAGE_SIZE) boot_first_id[LPAE_ENTRIES];
 #endif
-lpae_t boot_second[LPAE_ENTRIES]  __attribute__((__aligned__(4096)));
-lpae_t boot_third[LPAE_ENTRIES]  __attribute__((__aligned__(4096)));
+lpae_t __aligned(PAGE_SIZE) boot_second[LPAE_ENTRIES];
+lpae_t __aligned(PAGE_SIZE) boot_third[LPAE_ENTRIES];
 
 /* Main runtime page tables */
 
@@ -93,8 +93,8 @@ lpae_t boot_third[LPAE_ENTRIES]  __attribute__((__aligned__(4096)));
 
 #ifdef CONFIG_ARM_64
 #define HYP_PT_ROOT_LEVEL 0
-lpae_t xen_pgtable[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
-lpae_t xen_first[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
+static lpae_t __aligned(PAGE_SIZE) xen_pgtable[LPAE_ENTRIES];
+static lpae_t __aligned(PAGE_SIZE) xen_first[LPAE_ENTRIES];
 #define THIS_CPU_PGTABLE xen_pgtable
 #else
 #define HYP_PT_ROOT_LEVEL 1
@@ -107,17 +107,16 @@ static DEFINE_PER_CPU(lpae_t *, xen_pgtable);
  * DOMHEAP_VIRT_START...DOMHEAP_VIRT_END in 2MB chunks. */
 static DEFINE_PER_CPU(lpae_t *, xen_dommap);
 /* Root of the trie for cpu0, other CPU's PTs are dynamically allocated */
-lpae_t cpu0_pgtable[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
+static lpae_t __aligned(PAGE_SIZE) cpu0_pgtable[LPAE_ENTRIES];
 /* cpu0's domheap page tables */
-lpae_t cpu0_dommap[LPAE_ENTRIES*DOMHEAP_SECOND_PAGES]
-    __attribute__((__aligned__(4096*DOMHEAP_SECOND_PAGES)));
+static lpae_t __aligned(PAGE_SIZE) cpu0_dommap[LPAE_ENTRIES*DOMHEAP_SECOND_PAGES];
 #endif
 
 #ifdef CONFIG_ARM_64
 /* The first page of the first level mapping of the xenheap. The
  * subsequent xenheap first level pages are dynamically allocated, but
  * we need this one to bootstrap ourselves. */
-lpae_t xenheap_first_first[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
+static lpae_t __aligned(PAGE_SIZE) xenheap_first_first[LPAE_ENTRIES];
 /* The zeroeth level slot which uses xenheap_first_first. Used because
  * setup_xenheap_mappings otherwise relies on mfn_to_virt which isn't
  * valid for a non-xenheap mapping. */
@@ -131,12 +130,12 @@ static __initdata int xenheap_first_first_slot = -1;
  * addresses from 0 to 0x7fffffff. Offsets into it are calculated
  * with second_linear_offset(), not second_table_offset().
  */
-lpae_t xen_second[LPAE_ENTRIES*2] __attribute__((__aligned__(4096*2)));
+static lpae_t __aligned(PAGE_SIZE) xen_second[LPAE_ENTRIES*2];
 /* First level page table used for fixmap */
-lpae_t xen_fixmap[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
+lpae_t __aligned(PAGE_SIZE) xen_fixmap[LPAE_ENTRIES];
 /* First level page table used to map Xen itself with the XN bit set
  * as appropriate. */
-static lpae_t xen_xenmap[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
+static lpae_t __aligned(PAGE_SIZE) xen_xenmap[LPAE_ENTRIES];
 
 /* Non-boot CPUs use this to find the correct pagetables. */
 uint64_t init_ttbr;
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 15/20] xen/arm: mm: Use the shorter version __aligned(PAGE_SIZE) to align page-tables

[Julien Grall]

We currently use the very long version __attribute__((__aligned(4096)))
to align page-tables. Thankfully there is a shorter version to make
the code more readable.

While modifying the attribute:
    1) Move it before the variable name as we do in other part of Xen
    2) Switch to PAGE_SIZE instead of 4096 to make more future-proof
    3) Mark static page-tables not used outside the file (i.e any
       page-tables other than boot_* and xen_fixmap).

Lastly, some of the variables use __attribute__(__aligned(X * 4096)).
However this is not necessary as page-tables are only required to be
to be aligned to a page-size. So use __aligned(PAGE_SIZE).

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 6db7dda0da..fa0f41bd07 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -73,13 +73,13 @@ struct domain *dom_xen, *dom_io, *dom_cow;
  * Finally, if EARLY_PRINTK is enabled then xen_fixmap will be mapped
  * by the CPU once it has moved off the 1:1 mapping.
  */
-lpae_t boot_pgtable[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
+lpae_t __aligned(PAGE_SIZE) boot_pgtable[LPAE_ENTRIES];
 #ifdef CONFIG_ARM_64
-lpae_t boot_first[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
-lpae_t boot_first_id[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
+lpae_t __aligned(PAGE_SIZE) boot_first[LPAE_ENTRIES];
+lpae_t __aligned(PAGE_SIZE) boot_first_id[LPAE_ENTRIES];
 #endif
-lpae_t boot_second[LPAE_ENTRIES]  __attribute__((__aligned__(4096)));
-lpae_t boot_third[LPAE_ENTRIES]  __attribute__((__aligned__(4096)));
+lpae_t __aligned(PAGE_SIZE) boot_second[LPAE_ENTRIES];
+lpae_t __aligned(PAGE_SIZE) boot_third[LPAE_ENTRIES];
 
 /* Main runtime page tables */
 
@@ -93,8 +93,8 @@ lpae_t boot_third[LPAE_ENTRIES]  __attribute__((__aligned__(4096)));
 
 #ifdef CONFIG_ARM_64
 #define HYP_PT_ROOT_LEVEL 0
-lpae_t xen_pgtable[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
-lpae_t xen_first[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
+static lpae_t __aligned(PAGE_SIZE) xen_pgtable[LPAE_ENTRIES];
+static lpae_t __aligned(PAGE_SIZE) xen_first[LPAE_ENTRIES];
 #define THIS_CPU_PGTABLE xen_pgtable
 #else
 #define HYP_PT_ROOT_LEVEL 1
@@ -107,17 +107,16 @@ static DEFINE_PER_CPU(lpae_t *, xen_pgtable);
  * DOMHEAP_VIRT_START...DOMHEAP_VIRT_END in 2MB chunks. */
 static DEFINE_PER_CPU(lpae_t *, xen_dommap);
 /* Root of the trie for cpu0, other CPU's PTs are dynamically allocated */
-lpae_t cpu0_pgtable[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
+static lpae_t __aligned(PAGE_SIZE) cpu0_pgtable[LPAE_ENTRIES];
 /* cpu0's domheap page tables */
-lpae_t cpu0_dommap[LPAE_ENTRIES*DOMHEAP_SECOND_PAGES]
-    __attribute__((__aligned__(4096*DOMHEAP_SECOND_PAGES)));
+static lpae_t __aligned(PAGE_SIZE) cpu0_dommap[LPAE_ENTRIES*DOMHEAP_SECOND_PAGES];
 #endif
 
 #ifdef CONFIG_ARM_64
 /* The first page of the first level mapping of the xenheap. The
  * subsequent xenheap first level pages are dynamically allocated, but
  * we need this one to bootstrap ourselves. */
-lpae_t xenheap_first_first[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
+static lpae_t __aligned(PAGE_SIZE) xenheap_first_first[LPAE_ENTRIES];
 /* The zeroeth level slot which uses xenheap_first_first. Used because
  * setup_xenheap_mappings otherwise relies on mfn_to_virt which isn't
  * valid for a non-xenheap mapping. */
@@ -131,12 +130,12 @@ static __initdata int xenheap_first_first_slot = -1;
  * addresses from 0 to 0x7fffffff. Offsets into it are calculated
  * with second_linear_offset(), not second_table_offset().
  */
-lpae_t xen_second[LPAE_ENTRIES*2] __attribute__((__aligned__(4096*2)));
+static lpae_t __aligned(PAGE_SIZE) xen_second[LPAE_ENTRIES*2];
 /* First level page table used for fixmap */
-lpae_t xen_fixmap[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
+lpae_t __aligned(PAGE_SIZE) xen_fixmap[LPAE_ENTRIES];
 /* First level page table used to map Xen itself with the XN bit set
  * as appropriate. */
-static lpae_t xen_xenmap[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
+static lpae_t __aligned(PAGE_SIZE) xen_xenmap[LPAE_ENTRIES];
 
 /* Non-boot CPUs use this to find the correct pagetables. */
 uint64_t init_ttbr;
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 16/20] xen/arm: mm: Protect Xen page-table update with a spinlock

[Julien Grall]

The function create_xen_entries may be concurrently called. So we need
to protect with a spinlock to avoid corruption the page-tables.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index fa0f41bd07..ecde4e34df 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -969,6 +969,8 @@ enum xenmap_operation {
     RESERVE
 };
 
+static DEFINE_SPINLOCK(xen_pt_lock);
+
 static int create_xen_entries(enum xenmap_operation op,
                               unsigned long virt,
                               mfn_t mfn,
@@ -980,6 +982,8 @@ static int create_xen_entries(enum xenmap_operation op,
     lpae_t pte, *entry;
     lpae_t *third = NULL;
 
+    spin_lock(&xen_pt_lock);
+
     for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
     {
         entry = &xen_second[second_linear_offset(addr)];
@@ -1054,6 +1058,8 @@ out:
      */
     flush_xen_tlb_range_va(virt, PAGE_SIZE * nr_mfns);
 
+    spin_unlock(&xen_pt_lock);
+
     return rc;
 }
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 16/20] xen/arm: mm: Protect Xen page-table update with a spinlock

[Julien Grall]

The function create_xen_entries may be concurrently called. So we need
to protect with a spinlock to avoid corruption the page-tables.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index fa0f41bd07..ecde4e34df 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -969,6 +969,8 @@ enum xenmap_operation {
     RESERVE
 };
 
+static DEFINE_SPINLOCK(xen_pt_lock);
+
 static int create_xen_entries(enum xenmap_operation op,
                               unsigned long virt,
                               mfn_t mfn,
@@ -980,6 +982,8 @@ static int create_xen_entries(enum xenmap_operation op,
     lpae_t pte, *entry;
     lpae_t *third = NULL;
 
+    spin_lock(&xen_pt_lock);
+
     for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
     {
         entry = &xen_second[second_linear_offset(addr)];
@@ -1054,6 +1058,8 @@ out:
      */
     flush_xen_tlb_range_va(virt, PAGE_SIZE * nr_mfns);
 
+    spin_unlock(&xen_pt_lock);
+
     return rc;
 }
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 17/20] xen/arm: mm: Initialize page-tables earlier

[Julien Grall]

Since commit f60658c6ae "xen/arm: Stop relocating Xen", the function
setup_page_tables() does not require any information from the FDT.

So the initialization of the page-tables can be done much earlier in the
boot process. The earliest setup_page_tables() can be called is after
traps have been initialized, so we can get backtrace if an error
occurred.

Moving the initialization of the page-tables also avoid the dance to map
the FDT again in the new set of page-tables.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c    | 12 +++---------
 xen/arch/arm/setup.c |  4 ++--
 2 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index ecde4e34df..ee541a38e3 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -545,7 +545,7 @@ static inline lpae_t pte_of_xenaddr(vaddr_t va)
     return mfn_to_xen_entry(maddr_to_mfn(ma), MT_NORMAL);
 }
 
-/* Map the FDT in the early boot page table */
+/* Map the FDT in the runtime page table */
 void * __init early_fdt_map(paddr_t fdt_paddr)
 {
     /* We are using 2MB superpage for mapping the FDT */
@@ -568,7 +568,7 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
     /* The FDT is mapped using 2MB superpage */
     BUILD_BUG_ON(BOOT_FDT_VIRT_START % SZ_2M);
 
-    create_mappings(boot_second, BOOT_FDT_VIRT_START, paddr_to_pfn(base_paddr),
+    create_mappings(xen_second, BOOT_FDT_VIRT_START, paddr_to_pfn(base_paddr),
                     SZ_2M >> PAGE_SHIFT, SZ_2M);
 
     offset = fdt_paddr % SECOND_SIZE;
@@ -583,7 +583,7 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
 
     if ( (offset + size) > SZ_2M )
     {
-        create_mappings(boot_second, BOOT_FDT_VIRT_START + SZ_2M,
+        create_mappings(xen_second, BOOT_FDT_VIRT_START + SZ_2M,
                         paddr_to_pfn(base_paddr + SZ_2M),
                         SZ_2M >> PAGE_SHIFT, SZ_2M);
     }
@@ -694,12 +694,6 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
     pte.pt.table = 1;
     xen_second[second_table_offset(FIXMAP_ADDR(0))] = pte;
 
-    /* ... DTB */
-    pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START)];
-    xen_second[second_table_offset(BOOT_FDT_VIRT_START)] = pte;
-    pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)];
-    xen_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)] = pte;
-
 #ifdef CONFIG_ARM_64
     ttbr = (uintptr_t) xen_pgtable + phys_offset;
 #else
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 6dfbba2927..f7a399ce89 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -754,6 +754,8 @@ void __init start_xen(unsigned long boot_phys_offset,
     /* Initialize traps early allow us to get backtrace when an error occurred */
     init_traps();
 
+    setup_pagetables(boot_phys_offset);
+
     smp_clear_cpu_maps();
 
     device_tree_flattened = early_fdt_map(fdt_paddr);
@@ -775,8 +777,6 @@ void __init start_xen(unsigned long boot_phys_offset,
                              (paddr_t)(uintptr_t)(_end - _start + 1), false);
     BUG_ON(!xen_bootmodule);
 
-    setup_pagetables(boot_phys_offset);
-
     setup_mm(fdt_paddr, fdt_size);
 
     /* Parse the ACPI tables for possible boot-time configuration */
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 17/20] xen/arm: mm: Initialize page-tables earlier

[Julien Grall]

Since commit f60658c6ae "xen/arm: Stop relocating Xen", the function
setup_page_tables() does not require any information from the FDT.

So the initialization of the page-tables can be done much earlier in the
boot process. The earliest setup_page_tables() can be called is after
traps have been initialized, so we can get backtrace if an error
occurred.

Moving the initialization of the page-tables also avoid the dance to map
the FDT again in the new set of page-tables.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c    | 12 +++---------
 xen/arch/arm/setup.c |  4 ++--
 2 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index ecde4e34df..ee541a38e3 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -545,7 +545,7 @@ static inline lpae_t pte_of_xenaddr(vaddr_t va)
     return mfn_to_xen_entry(maddr_to_mfn(ma), MT_NORMAL);
 }
 
-/* Map the FDT in the early boot page table */
+/* Map the FDT in the runtime page table */
 void * __init early_fdt_map(paddr_t fdt_paddr)
 {
     /* We are using 2MB superpage for mapping the FDT */
@@ -568,7 +568,7 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
     /* The FDT is mapped using 2MB superpage */
     BUILD_BUG_ON(BOOT_FDT_VIRT_START % SZ_2M);
 
-    create_mappings(boot_second, BOOT_FDT_VIRT_START, paddr_to_pfn(base_paddr),
+    create_mappings(xen_second, BOOT_FDT_VIRT_START, paddr_to_pfn(base_paddr),
                     SZ_2M >> PAGE_SHIFT, SZ_2M);
 
     offset = fdt_paddr % SECOND_SIZE;
@@ -583,7 +583,7 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
 
     if ( (offset + size) > SZ_2M )
     {
-        create_mappings(boot_second, BOOT_FDT_VIRT_START + SZ_2M,
+        create_mappings(xen_second, BOOT_FDT_VIRT_START + SZ_2M,
                         paddr_to_pfn(base_paddr + SZ_2M),
                         SZ_2M >> PAGE_SHIFT, SZ_2M);
     }
@@ -694,12 +694,6 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
     pte.pt.table = 1;
     xen_second[second_table_offset(FIXMAP_ADDR(0))] = pte;
 
-    /* ... DTB */
-    pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START)];
-    xen_second[second_table_offset(BOOT_FDT_VIRT_START)] = pte;
-    pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)];
-    xen_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)] = pte;
-
 #ifdef CONFIG_ARM_64
     ttbr = (uintptr_t) xen_pgtable + phys_offset;
 #else
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 6dfbba2927..f7a399ce89 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -754,6 +754,8 @@ void __init start_xen(unsigned long boot_phys_offset,
     /* Initialize traps early allow us to get backtrace when an error occurred */
     init_traps();
 
+    setup_pagetables(boot_phys_offset);
+
     smp_clear_cpu_maps();
 
     device_tree_flattened = early_fdt_map(fdt_paddr);
@@ -775,8 +777,6 @@ void __init start_xen(unsigned long boot_phys_offset,
                              (paddr_t)(uintptr_t)(_end - _start + 1), false);
     BUG_ON(!xen_bootmodule);
 
-    setup_pagetables(boot_phys_offset);
-
     setup_mm(fdt_paddr, fdt_size);
 
     /* Parse the ACPI tables for possible boot-time configuration */
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 18/20] xen/arm: mm: Check start is always before end in {destroy, modify}_xen_mappings

[Julien Grall]

The two helpers {destroy, modify}_xen_mappings don't check that the
start is always before the end. This should never happen but if it
happens, it will result to unexpected behavior.

Catch such issues earlier on by adding an ASSERT in destroy_xen_mappings
and modify_xen_mappings.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index ee541a38e3..d6157c35d6 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1072,11 +1072,13 @@ int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
 
 int destroy_xen_mappings(unsigned long v, unsigned long e)
 {
+    ASSERT(v <= e);
     return create_xen_entries(REMOVE, v, INVALID_MFN, (e - v) >> PAGE_SHIFT, 0);
 }
 
 int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
 {
+    ASSERT(s <= e);
     return create_xen_entries(MODIFY, s, INVALID_MFN, (e - s) >> PAGE_SHIFT,
                               flags);
 }
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 18/20] xen/arm: mm: Check start is always before end in {destroy, modify}_xen_mappings

[Julien Grall]

The two helpers {destroy, modify}_xen_mappings don't check that the
start is always before the end. This should never happen but if it
happens, it will result to unexpected behavior.

Catch such issues earlier on by adding an ASSERT in destroy_xen_mappings
and modify_xen_mappings.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index ee541a38e3..d6157c35d6 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1072,11 +1072,13 @@ int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
 
 int destroy_xen_mappings(unsigned long v, unsigned long e)
 {
+    ASSERT(v <= e);
     return create_xen_entries(REMOVE, v, INVALID_MFN, (e - v) >> PAGE_SHIFT, 0);
 }
 
 int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
 {
+    ASSERT(s <= e);
     return create_xen_entries(MODIFY, s, INVALID_MFN, (e - s) >> PAGE_SHIFT,
                               flags);
 }
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 19/20] xen/arm: Pair call to set_fixmap with call to clear_fixmap in copy_from_paddr

[Julien Grall]

At the moment, set_fixmap may replace a valid entry without following
the break-before-make sequence. This may result to TLB conflict abort.

Rather than dealing with Break-Before-Make in set_fixmap, every call to
set_fixmap is paired with a call to clear_fixmap.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/kernel.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index e3ffdb2fa1..389bef2afa 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -58,13 +58,12 @@ void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
         set_fixmap(FIXMAP_MISC, maddr_to_mfn(paddr), PAGE_HYPERVISOR_WC);
         memcpy(dst, src + s, l);
         clean_dcache_va_range(dst, l);
+        clear_fixmap(FIXMAP_MISC);
 
         paddr += l;
         dst += l;
         len -= l;
     }
-
-    clear_fixmap(FIXMAP_MISC);
 }
 
 static void __init place_modules(struct kernel_info *info,
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 19/20] xen/arm: Pair call to set_fixmap with call to clear_fixmap in copy_from_paddr

[Julien Grall]

At the moment, set_fixmap may replace a valid entry without following
the break-before-make sequence. This may result to TLB conflict abort.

Rather than dealing with Break-Before-Make in set_fixmap, every call to
set_fixmap is paired with a call to clear_fixmap.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/kernel.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index e3ffdb2fa1..389bef2afa 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -58,13 +58,12 @@ void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
         set_fixmap(FIXMAP_MISC, maddr_to_mfn(paddr), PAGE_HYPERVISOR_WC);
         memcpy(dst, src + s, l);
         clean_dcache_va_range(dst, l);
+        clear_fixmap(FIXMAP_MISC);
 
         paddr += l;
         dst += l;
         len -= l;
     }
-
-    clear_fixmap(FIXMAP_MISC);
 }
 
 static void __init place_modules(struct kernel_info *info,
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 20/20] xen/arm: Allow cleaning the directory even when CONFIG_EARLY_PRINTK is set

[Julien Grall]

CONFIG_EARLY_PRINTK can only be set when CONFIG_DEBUG is enabled. It can
be quite convenient to only modify the target.

However, the target clean will not include the .config. This means
CONFIG_DEBUG is not enabled and therefore make will throw an error
preventing clean to continue.

The check is not moved at linking time.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---

This code is pretty nasty, but I haven't found a better way for avoiding
to check if CONFIG_DEBUG is enabled when the target clean is called.

Ideally we will want to move CONFIG_EARLY_PRINTK in Kconfig. I haven't
had time yet to look at it properly so far.
---
 xen/arch/arm/Makefile | 5 +++++
 xen/arch/arm/Rules.mk | 7 -------
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index cb902cb6fe..fef508c836 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -101,6 +101,11 @@ prelink.o: $(ALL_OBJS)
 endif
 
 $(TARGET)-syms: prelink.o xen.lds
+ifneq ($(CONFIG_EARLY_PRINTK), )
+ifneq ($(CONFIG_DEBUG), y)
+	$(error CONFIG_EARLY_PRINTK enabled for non-debug build)
+endif
+endif
 	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
 	    $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
 	$(NM) -pa --format=sysv $(@D)/.$(@F).0 \
diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
index f264592aef..12150986c5 100644
--- a/xen/arch/arm/Rules.mk
+++ b/xen/arch/arm/Rules.mk
@@ -80,11 +80,4 @@ CFLAGS-$(EARLY_PRINTK) += -DEARLY_PRINTK_BAUD=$(EARLY_PRINTK_BAUD)
 CFLAGS-$(EARLY_PRINTK) += -DEARLY_UART_BASE_ADDRESS=$(EARLY_UART_BASE_ADDRESS)
 CFLAGS-$(EARLY_PRINTK) += -DEARLY_UART_REG_SHIFT=$(EARLY_UART_REG_SHIFT)
 
-else # !CONFIG_DEBUG
-
-ifneq ($(CONFIG_EARLY_PRINTK),)
-# Early printk is dependant on a debug build.
-$(error CONFIG_EARLY_PRINTK enabled for non-debug build)
-endif
-
 endif
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 20/20] xen/arm: Allow cleaning the directory even when CONFIG_EARLY_PRINTK is set

[Julien Grall]

CONFIG_EARLY_PRINTK can only be set when CONFIG_DEBUG is enabled. It can
be quite convenient to only modify the target.

However, the target clean will not include the .config. This means
CONFIG_DEBUG is not enabled and therefore make will throw an error
preventing clean to continue.

The check is not moved at linking time.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---

This code is pretty nasty, but I haven't found a better way for avoiding
to check if CONFIG_DEBUG is enabled when the target clean is called.

Ideally we will want to move CONFIG_EARLY_PRINTK in Kconfig. I haven't
had time yet to look at it properly so far.
---
 xen/arch/arm/Makefile | 5 +++++
 xen/arch/arm/Rules.mk | 7 -------
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index cb902cb6fe..fef508c836 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -101,6 +101,11 @@ prelink.o: $(ALL_OBJS)
 endif
 
 $(TARGET)-syms: prelink.o xen.lds
+ifneq ($(CONFIG_EARLY_PRINTK), )
+ifneq ($(CONFIG_DEBUG), y)
+	$(error CONFIG_EARLY_PRINTK enabled for non-debug build)
+endif
+endif
 	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
 	    $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
 	$(NM) -pa --format=sysv $(@D)/.$(@F).0 \
diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
index f264592aef..12150986c5 100644
--- a/xen/arch/arm/Rules.mk
+++ b/xen/arch/arm/Rules.mk
@@ -80,11 +80,4 @@ CFLAGS-$(EARLY_PRINTK) += -DEARLY_PRINTK_BAUD=$(EARLY_PRINTK_BAUD)
 CFLAGS-$(EARLY_PRINTK) += -DEARLY_UART_BASE_ADDRESS=$(EARLY_UART_BASE_ADDRESS)
 CFLAGS-$(EARLY_PRINTK) += -DEARLY_UART_REG_SHIFT=$(EARLY_UART_REG_SHIFT)
 
-else # !CONFIG_DEBUG
-
-ifneq ($(CONFIG_EARLY_PRINTK),)
-# Early printk is dependant on a debug build.
-$(error CONFIG_EARLY_PRINTK enabled for non-debug build)
-endif
-
 endif
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 20/20] xen/arm: Allow cleaning the directory even when CONFIG_EARLY_PRINTK is set

[Julien Grall]

Hi,

Please ignore this patch. This was already sent separately.

Cheers,

On 22/04/2019 17:49, Julien Grall wrote:
> CONFIG_EARLY_PRINTK can only be set when CONFIG_DEBUG is enabled. It can
> be quite convenient to only modify the target.
> 
> However, the target clean will not include the .config. This means
> CONFIG_DEBUG is not enabled and therefore make will throw an error
> preventing clean to continue.
> 
> The check is not moved at linking time.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
> 
> This code is pretty nasty, but I haven't found a better way for avoiding
> to check if CONFIG_DEBUG is enabled when the target clean is called.
> 
> Ideally we will want to move CONFIG_EARLY_PRINTK in Kconfig. I haven't
> had time yet to look at it properly so far.
> ---
>   xen/arch/arm/Makefile | 5 +++++
>   xen/arch/arm/Rules.mk | 7 -------
>   2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index cb902cb6fe..fef508c836 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -101,6 +101,11 @@ prelink.o: $(ALL_OBJS)
>   endif
>   
>   $(TARGET)-syms: prelink.o xen.lds
> +ifneq ($(CONFIG_EARLY_PRINTK), )
> +ifneq ($(CONFIG_DEBUG), y)
> +	$(error CONFIG_EARLY_PRINTK enabled for non-debug build)
> +endif
> +endif
>   	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
>   	    $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
>   	$(NM) -pa --format=sysv $(@D)/.$(@F).0 \
> diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
> index f264592aef..12150986c5 100644
> --- a/xen/arch/arm/Rules.mk
> +++ b/xen/arch/arm/Rules.mk
> @@ -80,11 +80,4 @@ CFLAGS-$(EARLY_PRINTK) += -DEARLY_PRINTK_BAUD=$(EARLY_PRINTK_BAUD)
>   CFLAGS-$(EARLY_PRINTK) += -DEARLY_UART_BASE_ADDRESS=$(EARLY_UART_BASE_ADDRESS)
>   CFLAGS-$(EARLY_PRINTK) += -DEARLY_UART_REG_SHIFT=$(EARLY_UART_REG_SHIFT)
>   
> -else # !CONFIG_DEBUG
> -
> -ifneq ($(CONFIG_EARLY_PRINTK),)
> -# Early printk is dependant on a debug build.
> -$(error CONFIG_EARLY_PRINTK enabled for non-debug build)
> -endif
> -
>   endif
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 20/20] xen/arm: Allow cleaning the directory even when CONFIG_EARLY_PRINTK is set

[Julien Grall]

Hi,

Please ignore this patch. This was already sent separately.

Cheers,

On 22/04/2019 17:49, Julien Grall wrote:
> CONFIG_EARLY_PRINTK can only be set when CONFIG_DEBUG is enabled. It can
> be quite convenient to only modify the target.
> 
> However, the target clean will not include the .config. This means
> CONFIG_DEBUG is not enabled and therefore make will throw an error
> preventing clean to continue.
> 
> The check is not moved at linking time.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
> 
> This code is pretty nasty, but I haven't found a better way for avoiding
> to check if CONFIG_DEBUG is enabled when the target clean is called.
> 
> Ideally we will want to move CONFIG_EARLY_PRINTK in Kconfig. I haven't
> had time yet to look at it properly so far.
> ---
>   xen/arch/arm/Makefile | 5 +++++
>   xen/arch/arm/Rules.mk | 7 -------
>   2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index cb902cb6fe..fef508c836 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -101,6 +101,11 @@ prelink.o: $(ALL_OBJS)
>   endif
>   
>   $(TARGET)-syms: prelink.o xen.lds
> +ifneq ($(CONFIG_EARLY_PRINTK), )
> +ifneq ($(CONFIG_DEBUG), y)
> +	$(error CONFIG_EARLY_PRINTK enabled for non-debug build)
> +endif
> +endif
>   	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
>   	    $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
>   	$(NM) -pa --format=sysv $(@D)/.$(@F).0 \
> diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
> index f264592aef..12150986c5 100644
> --- a/xen/arch/arm/Rules.mk
> +++ b/xen/arch/arm/Rules.mk
> @@ -80,11 +80,4 @@ CFLAGS-$(EARLY_PRINTK) += -DEARLY_PRINTK_BAUD=$(EARLY_PRINTK_BAUD)
>   CFLAGS-$(EARLY_PRINTK) += -DEARLY_UART_BASE_ADDRESS=$(EARLY_UART_BASE_ADDRESS)
>   CFLAGS-$(EARLY_PRINTK) += -DEARLY_UART_REG_SHIFT=$(EARLY_UART_REG_SHIFT)
>   
> -else # !CONFIG_DEBUG
> -
> -ifneq ($(CONFIG_EARLY_PRINTK),)
> -# Early printk is dependant on a debug build.
> -$(error CONFIG_EARLY_PRINTK enabled for non-debug build)
> -endif
> -
>   endif
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 01/20] xen/const: Introduce _BITUL and _BITULL

[Jan Beulich]

>>> On 22.04.19 at 18:49, <julien.grall@arm.com> wrote:
> The pattern _AC(1, UL{,L}) << X is commonly used in the headers to make
> define usuable in both assembly and C.
> 
> So introduce _BITUL and _BITULL to make the code slightly more readable.

I don't particularly like the names, and I specifically object to
the leading underscores. I'm afraid I don't have better
suggestions for the names, but what I'd like to ask for is that
at least the UL / ULL be somehow separated from BIT. One
option might be something like

#define BIT(pos, sfx) (_AC(1, sfx) << (pos))

albeit BIT may be a little too generic a name, yet something
like DEFINE_BIT looks a little longish. But at least it would also
allow e.g. plain unsigned (non-long) constants to be defined
without yet another new construct.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 01/20] xen/const: Introduce _BITUL and _BITULL

[Jan Beulich]

>>> On 22.04.19 at 18:49, <julien.grall@arm.com> wrote:
> The pattern _AC(1, UL{,L}) << X is commonly used in the headers to make
> define usuable in both assembly and C.
> 
> So introduce _BITUL and _BITULL to make the code slightly more readable.

I don't particularly like the names, and I specifically object to
the leading underscores. I'm afraid I don't have better
suggestions for the names, but what I'd like to ask for is that
at least the UL / ULL be somehow separated from BIT. One
option might be something like

#define BIT(pos, sfx) (_AC(1, sfx) << (pos))

albeit BIT may be a little too generic a name, yet something
like DEFINE_BIT looks a little longish. But at least it would also
allow e.g. plain unsigned (non-long) constants to be defined
without yet another new construct.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 01/20] xen/const: Introduce _BITUL and _BITULL

[Julien Grall]

Hi,

On 25/04/2019 13:15, Jan Beulich wrote:
>>>> On 22.04.19 at 18:49, <julien.grall@arm.com> wrote:
>> The pattern _AC(1, UL{,L}) << X is commonly used in the headers to make
>> define usuable in both assembly and C.
>>
>> So introduce _BITUL and _BITULL to make the code slightly more readable.
> 
> I don't particularly like the names, and I specifically object to
> the leading underscores. I'm afraid I don't have better
> suggestions for the names, but what I'd like to ask for is that
> at least the UL / ULL be somehow separated from BIT. One
> option might be something like

The _ match the other assembly macro we have defined in const.h. I understand 
you don't like the leading underscores, but I think consistency is better here.

The more keeping the same generic naming lower down the churn to import code 
from Linux.

> 
> #define BIT(pos, sfx) (_AC(1, sfx) << (pos))

BIT() is already define in Xen and used in code coming from Linux. I would 
rather not change the prototype for this reason.

> 
> albeit BIT may be a little too generic a name, yet something
> like DEFINE_BIT looks a little longish. But at least it would also
> allow e.g. plain unsigned (non-long) constants to be defined
> without yet another new construct.

See above the reason why those names.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 01/20] xen/const: Introduce _BITUL and _BITULL

[Julien Grall]

Hi,

On 25/04/2019 13:15, Jan Beulich wrote:
>>>> On 22.04.19 at 18:49, <julien.grall@arm.com> wrote:
>> The pattern _AC(1, UL{,L}) << X is commonly used in the headers to make
>> define usuable in both assembly and C.
>>
>> So introduce _BITUL and _BITULL to make the code slightly more readable.
> 
> I don't particularly like the names, and I specifically object to
> the leading underscores. I'm afraid I don't have better
> suggestions for the names, but what I'd like to ask for is that
> at least the UL / ULL be somehow separated from BIT. One
> option might be something like

The _ match the other assembly macro we have defined in const.h. I understand 
you don't like the leading underscores, but I think consistency is better here.

The more keeping the same generic naming lower down the churn to import code 
from Linux.

> 
> #define BIT(pos, sfx) (_AC(1, sfx) << (pos))

BIT() is already define in Xen and used in code coming from Linux. I would 
rather not change the prototype for this reason.

> 
> albeit BIT may be a little too generic a name, yet something
> like DEFINE_BIT looks a little longish. But at least it would also
> allow e.g. plain unsigned (non-long) constants to be defined
> without yet another new construct.

See above the reason why those names.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 01/20] xen/const: Introduce _BITUL and _BITULL

[Jan Beulich]

>>> On 29.04.19 at 18:47, <julien.grall@arm.com> wrote:
> On 25/04/2019 13:15, Jan Beulich wrote:
>>>>> On 22.04.19 at 18:49, <julien.grall@arm.com> wrote:
>>> The pattern _AC(1, UL{,L}) << X is commonly used in the headers to make
>>> define usuable in both assembly and C.
>>>
>>> So introduce _BITUL and _BITULL to make the code slightly more readable.
>> 
>> I don't particularly like the names, and I specifically object to
>> the leading underscores. I'm afraid I don't have better
>> suggestions for the names, but what I'd like to ask for is that
>> at least the UL / ULL be somehow separated from BIT. One
>> option might be something like
> 
> The _ match the other assembly macro we have defined in const.h. I understand 
> you don't like the leading underscores, but I think consistency is better here.

Well, "don't like" sounds like personal taste, but it goes beyond
that: It's a name space violation. I'm all for consistency, but not
at the expense of violating what the language standard demands.
If consistency is a goal here, the other macro names should be
changed rather than introducing more offenders.

> The more keeping the same generic naming lower down the churn to import code 
> from Linux.

While this is a desirable goal, as long as Linux doesn't care about
name space violations we shouldn't follow them slavishly (or
establish firmly that we don#t care about name space violations
either).

>> #define BIT(pos, sfx) (_AC(1, sfx) << (pos))
> 
> BIT() is already define in Xen and used in code coming from Linux. I would 
> rather not change the prototype for this reason.

That's up for debate. My proposal is more flexible than what we
currently have. Whether that outweighs becoming incompatible
with Linux'es similarly named macro.

Talking of Linux: Now that I've looked, I can't really figure why
they have both BIT_ULL() (linux/bits.h) and BITULL (linux/const.h).
The former is clearly redundant with the latter (and BIT() with
BITUL()). I don't think we should follow that model. In fact I think
BIT() as proposed above is then really the best solution, covering
everything that's needed in one go.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 01/20] xen/const: Introduce _BITUL and _BITULL

[Jan Beulich]

>>> On 29.04.19 at 18:47, <julien.grall@arm.com> wrote:
> On 25/04/2019 13:15, Jan Beulich wrote:
>>>>> On 22.04.19 at 18:49, <julien.grall@arm.com> wrote:
>>> The pattern _AC(1, UL{,L}) << X is commonly used in the headers to make
>>> define usuable in both assembly and C.
>>>
>>> So introduce _BITUL and _BITULL to make the code slightly more readable.
>> 
>> I don't particularly like the names, and I specifically object to
>> the leading underscores. I'm afraid I don't have better
>> suggestions for the names, but what I'd like to ask for is that
>> at least the UL / ULL be somehow separated from BIT. One
>> option might be something like
> 
> The _ match the other assembly macro we have defined in const.h. I understand 
> you don't like the leading underscores, but I think consistency is better here.

Well, "don't like" sounds like personal taste, but it goes beyond
that: It's a name space violation. I'm all for consistency, but not
at the expense of violating what the language standard demands.
If consistency is a goal here, the other macro names should be
changed rather than introducing more offenders.

> The more keeping the same generic naming lower down the churn to import code 
> from Linux.

While this is a desirable goal, as long as Linux doesn't care about
name space violations we shouldn't follow them slavishly (or
establish firmly that we don#t care about name space violations
either).

>> #define BIT(pos, sfx) (_AC(1, sfx) << (pos))
> 
> BIT() is already define in Xen and used in code coming from Linux. I would 
> rather not change the prototype for this reason.

That's up for debate. My proposal is more flexible than what we
currently have. Whether that outweighs becoming incompatible
with Linux'es similarly named macro.

Talking of Linux: Now that I've looked, I can't really figure why
they have both BIT_ULL() (linux/bits.h) and BITULL (linux/const.h).
The former is clearly redundant with the latter (and BIT() with
BITUL()). I don't think we should follow that model. In fact I think
BIT() as proposed above is then really the best solution, covering
everything that's needed in one go.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 02/20] xen/arm: Rename SCTLR_* defines and remove unused one

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> The SCTLR_* are currently used for SCTLR/HSCTLR (arm32) and
> SCTLR_EL1/SCTLR_EL2 (arm64).
> 
> The naming scheme is actually quite confusing because they may only be
> defined for an archicture (or even an exception level). So it is not easy
> for the developer to know which one to use.
> 
> The naming scheme is reworked by adding Axx_ELx in each define:
>      * xx is replaced by 32 or 64 if specific to an architecture
>      * x is replaced by 2 (hypervisor) or 1 (kernel) if specific to an
>      exception level
> 
> While doing the renaming, remove the unused defines (or at least the ones
> that are unlikely going to be used).
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>


-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 02/20] xen/arm: Rename SCTLR_* defines and remove unused one

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> The SCTLR_* are currently used for SCTLR/HSCTLR (arm32) and
> SCTLR_EL1/SCTLR_EL2 (arm64).
> 
> The naming scheme is actually quite confusing because they may only be
> defined for an archicture (or even an exception level). So it is not easy
> for the developer to know which one to use.
> 
> The naming scheme is reworked by adding Axx_ELx in each define:
>      * xx is replaced by 32 or 64 if specific to an architecture
>      * x is replaced by 2 (hypervisor) or 1 (kernel) if specific to an
>      exception level
> 
> While doing the renaming, remove the unused defines (or at least the ones
> that are unlikely going to be used).
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>


-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 03/20] xen/arm: processor: Use _BITUL instead of _AC(1, U) in SCTLR_ defines

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> The newly introduced macro _BITUL makes the code more readable.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>   xen/include/asm-arm/processor.h | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index c6f56490b3..1a143fb6a3 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -115,20 +115,20 @@
>   
>   /* Bits specific to SCTLR_EL1 for Arm32 */
>   
> -#define SCTLR_A32_EL1_V     (_AC(1,U)<<13)
> +#define SCTLR_A32_EL1_V     _BITUL(13)
>   
>   /* Common bits for SCTLR_ELx for Arm32 */
>   
> -#define SCTLR_A32_ELx_TE    (_AC(1,U)<<30)
> -#define SCTLR_A32_ELx_FI    (_AC(1,U)<<21)
> +#define SCTLR_A32_ELx_TE    _BITUL(30)
> +#define SCTLR_A32_ELx_FI    _BITUL(21)
>   
>   /* Common bits for SCTLR_ELx on all architectures */
> -#define SCTLR_Axx_ELx_EE    (_AC(1,U)<<25)
> -#define SCTLR_Axx_ELx_WXN   (_AC(1,U)<<19)
> -#define SCTLR_Axx_ELx_I     (_AC(1,U)<<12)
> -#define SCTLR_Axx_ELx_C     (_AC(1,U)<<2)
> -#define SCTLR_Axx_ELx_A     (_AC(1,U)<<1)
> -#define SCTLR_Axx_ELx_M     (_AC(1,U)<<0)
> +#define SCTLR_Axx_ELx_EE    _BITUL(25)
> +#define SCTLR_Axx_ELx_WXN   _BITUL(19)
> +#define SCTLR_Axx_ELx_I     _BITUL(12)
> +#define SCTLR_Axx_ELx_C     _BITUL(2)
> +#define SCTLR_Axx_ELx_A     _BITUL(1)
> +#define SCTLR_Axx_ELx_M     _BITUL(0)
>   
>   #define HSCTLR_BASE     _AC(0x30c51878,U)
>   
> 

Resolution of the dispute with Jan about [PATCH 01/20] is required first.

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 03/20] xen/arm: processor: Use _BITUL instead of _AC(1, U) in SCTLR_ defines

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> The newly introduced macro _BITUL makes the code more readable.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>   xen/include/asm-arm/processor.h | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index c6f56490b3..1a143fb6a3 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -115,20 +115,20 @@
>   
>   /* Bits specific to SCTLR_EL1 for Arm32 */
>   
> -#define SCTLR_A32_EL1_V     (_AC(1,U)<<13)
> +#define SCTLR_A32_EL1_V     _BITUL(13)
>   
>   /* Common bits for SCTLR_ELx for Arm32 */
>   
> -#define SCTLR_A32_ELx_TE    (_AC(1,U)<<30)
> -#define SCTLR_A32_ELx_FI    (_AC(1,U)<<21)
> +#define SCTLR_A32_ELx_TE    _BITUL(30)
> +#define SCTLR_A32_ELx_FI    _BITUL(21)
>   
>   /* Common bits for SCTLR_ELx on all architectures */
> -#define SCTLR_Axx_ELx_EE    (_AC(1,U)<<25)
> -#define SCTLR_Axx_ELx_WXN   (_AC(1,U)<<19)
> -#define SCTLR_Axx_ELx_I     (_AC(1,U)<<12)
> -#define SCTLR_Axx_ELx_C     (_AC(1,U)<<2)
> -#define SCTLR_Axx_ELx_A     (_AC(1,U)<<1)
> -#define SCTLR_Axx_ELx_M     (_AC(1,U)<<0)
> +#define SCTLR_Axx_ELx_EE    _BITUL(25)
> +#define SCTLR_Axx_ELx_WXN   _BITUL(19)
> +#define SCTLR_Axx_ELx_I     _BITUL(12)
> +#define SCTLR_Axx_ELx_C     _BITUL(2)
> +#define SCTLR_Axx_ELx_A     _BITUL(1)
> +#define SCTLR_Axx_ELx_M     _BITUL(0)
>   
>   #define HSCTLR_BASE     _AC(0x30c51878,U)
>   
> 

Resolution of the dispute with Jan about [PATCH 01/20] is required first.

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 04/20] xen/arm: Rework HSCTLR_BASE

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> The current value of HSCTLR_BASE for Arm64 is pretty wrong. It would
> actually turn on SCTLR_EL2.nAA (bit 6) on hardware implementing
> ARMv8.4-LSE.
> 
> Furthermore, the documentation of what is cleared/set in SCTLR_EL2 is
> also not correct and looks like to be a verbatim copy from Arm32.
> 
> HSCTLR_BASE is replaced with a bunch of per-architecture new defines
> helping to understand better what is the initialie value for
> SCTLR_EL2/HSCTLR.
> 
> Note the defines *_CLEAR are only used to check the state of each bits
> are known.
> 
> Lastly, the documentation is dropped from arm{32,64}/head.S as it would
> be pretty easy to get out-of-sync with the definitions.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>   xen/arch/arm/arm32/head.S       | 12 +---------
>   xen/arch/arm/arm64/head.S       | 10 +-------
>   xen/include/asm-arm/processor.h | 52 ++++++++++++++++++++++++++++++++++++++++-
>   3 files changed, 53 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 5ef7e5a2f3..8a98607459 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -234,17 +234,7 @@ cpu_init_done:
>           ldr   r0, =(TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(0))
>           mcr   CP32(r0, HTCR)
>   
> -        /*
> -         * Set up the HSCTLR:
> -         * Exceptions in LE ARM,
> -         * Low-latency IRQs disabled,
> -         * Write-implies-XN disabled (for now),
> -         * D-cache disabled (for now),
> -         * I-cache enabled,
> -         * Alignment checking enabled,
> -         * MMU translation disabled (for now).
> -         */
> -        ldr   r0, =(HSCTLR_BASE|SCTLR_AXX_A)
> +        ldr   r0, =HSCTLR_SET
>           mcr   CP32(r0, HSCTLR)
>   
>           /*
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 8a6be3352e..4fe904c51d 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -363,15 +363,7 @@ skip_bss:
>   
>           msr   tcr_el2, x0
>   
> -        /* Set up the SCTLR_EL2:
> -         * Exceptions in LE ARM,
> -         * Low-latency IRQs disabled,
> -         * Write-implies-XN disabled (for now),
> -         * D-cache disabled (for now),
> -         * I-cache enabled,
> -         * Alignment checking disabled,
> -         * MMU translation disabled (for now). */
> -        ldr   x0, =(HSCTLR_BASE)
> +        ldr   x0, =SCTLR_EL2_SET
>           msr   SCTLR_EL2, x0
>   
>           /* Ensure that any exceptions encountered at EL2
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index 1a143fb6a3..6dac500e40 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -122,6 +122,9 @@
>   #define SCTLR_A32_ELx_TE    _BITUL(30)
>   #define SCTLR_A32_ELx_FI    _BITUL(21)
>   
> +/* Common bits for SCTLR_ELx for Arm64 */
> +#define SCTLR_A64_ELx_SA    _BITUL(3)
> +
>   /* Common bits for SCTLR_ELx on all architectures */
>   #define SCTLR_Axx_ELx_EE    _BITUL(25)
>   #define SCTLR_Axx_ELx_WXN   _BITUL(19)
> @@ -130,7 +133,54 @@
>   #define SCTLR_Axx_ELx_A     _BITUL(1)
>   #define SCTLR_Axx_ELx_M     _BITUL(0)
>   
> -#define HSCTLR_BASE     _AC(0x30c51878,U)
> +#ifdef CONFIG_ARM_32
> +
> +#define HSCTLR_RES1     (_BITUL(3)  | _BITUL(4)  | _BITUL(5)  | _BITUL(6)  |\
> +                         _BITUL(11) | _BITUL(16) | _BITUL(18) | _BITUL(22) |\
> +                         _BITUL(23) | _BITUL(28) | _BITUL(29))
> +
> +#define HSCTLR_RES0     (_BITUL(7)  | _BITUL(8)  | _BITUL(9)  | _BITUL(10) |\
> +                         _BITUL(13) | _BITUL(14) | _BITUL(15) | _BITUL(17) |\
> +                         _BITUL(20) | _BITUL(24) | _BITUL(26) | _BITUL(27) |\
> +                         _BITUL(31))
> +
> +/* Initial value for HSCTLR */
> +#define HSCTLR_SET      (HSCTLR_RES1    | SCTLR_Axx_ELx_A   | SCTLR_Axx_ELx_I)
> +
> +#define HSCTLR_CLEAR    (HSCTLR_RES0        | SCTLR_Axx_ELx_M   |\
> +                         SCTLR_Axx_ELx_C    | SCTLR_Axx_ELx_WXN |\
> +                         SCTLR_A32_ELx_FI   | SCTLR_Axx_ELx_EE  |\
> +                         SCTLR_A32_ELx_TE)
> +
> +#if (HSCTLR_SET ^ HSCTLR_CLEAR) != 0xffffffffU
> +#error "Inconsistent HSCTLR set/clear bits"
> +#endif
> +
> +#else
> +
> +#define SCTLR_EL2_RES1  (_BITUL(4)  | _BITUL(5)  | _BITUL(11) | _BITUL(16) |\
> +                         _BITUL(18) | _BITUL(22) | _BITUL(23) | _BITUL(28) |\
> +                         _BITUL(29))
> +
> +#define SCTLR_EL2_RES0  (_BITUL(6)  | _BITUL(7)  | _BITUL(8)  | _BITUL(9)  |\
> +                         _BITUL(10) | _BITUL(13) | _BITUL(14) | _BITUL(15) |\
> +                         _BITUL(17) | _BITUL(20) | _BITUL(21) | _BITUL(24) |\
> +                         _BITUL(26) | _BITUL(27) | _BITUL(30) | _BITUL(31) |\
> +                         (0xffffffffULL << 32))
> +
> +/* Initial value for SCTLR_EL2 */
> +#define SCTLR_EL2_SET   (SCTLR_EL2_RES1     | SCTLR_A64_ELx_SA  |\
> +                         SCTLR_Axx_ELx_I)
> +
> +#define SCTLR_EL2_CLEAR (SCTLR_EL2_RES0     | SCTLR_Axx_ELx_M   |\
> +                         SCTLR_Axx_ELx_A    | SCTLR_Axx_ELx_C   |\
> +                         SCTLR_Axx_ELx_WXN  | SCTLR_Axx_ELx_EE)
> +
> +#if (SCTLR_EL2_SET ^ SCTLR_EL2_CLEAR) != 0xffffffffffffffffUL
> +#error "Inconsistent SCTLR_EL2 set/clear bits"
> +#endif
> +
> +#endif
>   
>   /* HCR Hyp Configuration Register */
>   #define HCR_RW          (_AC(1,UL)<<31) /* Register Width, ARM64 only */
> 

Resolution of the dispute with Jan about [PATCH 01/20] is required first.

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 04/20] xen/arm: Rework HSCTLR_BASE

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> The current value of HSCTLR_BASE for Arm64 is pretty wrong. It would
> actually turn on SCTLR_EL2.nAA (bit 6) on hardware implementing
> ARMv8.4-LSE.
> 
> Furthermore, the documentation of what is cleared/set in SCTLR_EL2 is
> also not correct and looks like to be a verbatim copy from Arm32.
> 
> HSCTLR_BASE is replaced with a bunch of per-architecture new defines
> helping to understand better what is the initialie value for
> SCTLR_EL2/HSCTLR.
> 
> Note the defines *_CLEAR are only used to check the state of each bits
> are known.
> 
> Lastly, the documentation is dropped from arm{32,64}/head.S as it would
> be pretty easy to get out-of-sync with the definitions.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>   xen/arch/arm/arm32/head.S       | 12 +---------
>   xen/arch/arm/arm64/head.S       | 10 +-------
>   xen/include/asm-arm/processor.h | 52 ++++++++++++++++++++++++++++++++++++++++-
>   3 files changed, 53 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 5ef7e5a2f3..8a98607459 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -234,17 +234,7 @@ cpu_init_done:
>           ldr   r0, =(TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(0))
>           mcr   CP32(r0, HTCR)
>   
> -        /*
> -         * Set up the HSCTLR:
> -         * Exceptions in LE ARM,
> -         * Low-latency IRQs disabled,
> -         * Write-implies-XN disabled (for now),
> -         * D-cache disabled (for now),
> -         * I-cache enabled,
> -         * Alignment checking enabled,
> -         * MMU translation disabled (for now).
> -         */
> -        ldr   r0, =(HSCTLR_BASE|SCTLR_AXX_A)
> +        ldr   r0, =HSCTLR_SET
>           mcr   CP32(r0, HSCTLR)
>   
>           /*
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 8a6be3352e..4fe904c51d 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -363,15 +363,7 @@ skip_bss:
>   
>           msr   tcr_el2, x0
>   
> -        /* Set up the SCTLR_EL2:
> -         * Exceptions in LE ARM,
> -         * Low-latency IRQs disabled,
> -         * Write-implies-XN disabled (for now),
> -         * D-cache disabled (for now),
> -         * I-cache enabled,
> -         * Alignment checking disabled,
> -         * MMU translation disabled (for now). */
> -        ldr   x0, =(HSCTLR_BASE)
> +        ldr   x0, =SCTLR_EL2_SET
>           msr   SCTLR_EL2, x0
>   
>           /* Ensure that any exceptions encountered at EL2
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index 1a143fb6a3..6dac500e40 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -122,6 +122,9 @@
>   #define SCTLR_A32_ELx_TE    _BITUL(30)
>   #define SCTLR_A32_ELx_FI    _BITUL(21)
>   
> +/* Common bits for SCTLR_ELx for Arm64 */
> +#define SCTLR_A64_ELx_SA    _BITUL(3)
> +
>   /* Common bits for SCTLR_ELx on all architectures */
>   #define SCTLR_Axx_ELx_EE    _BITUL(25)
>   #define SCTLR_Axx_ELx_WXN   _BITUL(19)
> @@ -130,7 +133,54 @@
>   #define SCTLR_Axx_ELx_A     _BITUL(1)
>   #define SCTLR_Axx_ELx_M     _BITUL(0)
>   
> -#define HSCTLR_BASE     _AC(0x30c51878,U)
> +#ifdef CONFIG_ARM_32
> +
> +#define HSCTLR_RES1     (_BITUL(3)  | _BITUL(4)  | _BITUL(5)  | _BITUL(6)  |\
> +                         _BITUL(11) | _BITUL(16) | _BITUL(18) | _BITUL(22) |\
> +                         _BITUL(23) | _BITUL(28) | _BITUL(29))
> +
> +#define HSCTLR_RES0     (_BITUL(7)  | _BITUL(8)  | _BITUL(9)  | _BITUL(10) |\
> +                         _BITUL(13) | _BITUL(14) | _BITUL(15) | _BITUL(17) |\
> +                         _BITUL(20) | _BITUL(24) | _BITUL(26) | _BITUL(27) |\
> +                         _BITUL(31))
> +
> +/* Initial value for HSCTLR */
> +#define HSCTLR_SET      (HSCTLR_RES1    | SCTLR_Axx_ELx_A   | SCTLR_Axx_ELx_I)
> +
> +#define HSCTLR_CLEAR    (HSCTLR_RES0        | SCTLR_Axx_ELx_M   |\
> +                         SCTLR_Axx_ELx_C    | SCTLR_Axx_ELx_WXN |\
> +                         SCTLR_A32_ELx_FI   | SCTLR_Axx_ELx_EE  |\
> +                         SCTLR_A32_ELx_TE)
> +
> +#if (HSCTLR_SET ^ HSCTLR_CLEAR) != 0xffffffffU
> +#error "Inconsistent HSCTLR set/clear bits"
> +#endif
> +
> +#else
> +
> +#define SCTLR_EL2_RES1  (_BITUL(4)  | _BITUL(5)  | _BITUL(11) | _BITUL(16) |\
> +                         _BITUL(18) | _BITUL(22) | _BITUL(23) | _BITUL(28) |\
> +                         _BITUL(29))
> +
> +#define SCTLR_EL2_RES0  (_BITUL(6)  | _BITUL(7)  | _BITUL(8)  | _BITUL(9)  |\
> +                         _BITUL(10) | _BITUL(13) | _BITUL(14) | _BITUL(15) |\
> +                         _BITUL(17) | _BITUL(20) | _BITUL(21) | _BITUL(24) |\
> +                         _BITUL(26) | _BITUL(27) | _BITUL(30) | _BITUL(31) |\
> +                         (0xffffffffULL << 32))
> +
> +/* Initial value for SCTLR_EL2 */
> +#define SCTLR_EL2_SET   (SCTLR_EL2_RES1     | SCTLR_A64_ELx_SA  |\
> +                         SCTLR_Axx_ELx_I)
> +
> +#define SCTLR_EL2_CLEAR (SCTLR_EL2_RES0     | SCTLR_Axx_ELx_M   |\
> +                         SCTLR_Axx_ELx_A    | SCTLR_Axx_ELx_C   |\
> +                         SCTLR_Axx_ELx_WXN  | SCTLR_Axx_ELx_EE)
> +
> +#if (SCTLR_EL2_SET ^ SCTLR_EL2_CLEAR) != 0xffffffffffffffffUL
> +#error "Inconsistent SCTLR_EL2 set/clear bits"
> +#endif
> +
> +#endif
>   
>   /* HCR Hyp Configuration Register */
>   #define HCR_RW          (_AC(1,UL)<<31) /* Register Width, ARM64 only */
> 

Resolution of the dispute with Jan about [PATCH 01/20] is required first.

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 05/20] xen/arm: Rework secondary_start prototype

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> None of the parameters of secondary_start are actually used. So turn
> secondary_start to a function with no parameters.
> 
> Also modify the assembly code to avoid setting-up the registers before
> calling secondary_start.

What is not really mandatory.

> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Though:

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 05/20] xen/arm: Rework secondary_start prototype

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> None of the parameters of secondary_start are actually used. So turn
> secondary_start to a function with no parameters.
> 
> Also modify the assembly code to avoid setting-up the registers before
> calling secondary_start.

What is not really mandatory.

> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Though:

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 06/20] xen/arm: Remove parameter cpuid from start_xen

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> The parameter cpuid is not used by start_xen. So remove it.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>   xen/arch/arm/arm32/head.S | 1 -
>   xen/arch/arm/arm64/head.S | 1 -
>   xen/arch/arm/setup.c      | 3 +--
>   3 files changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index b71d7fb11d..9f40face98 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -448,7 +448,6 @@ launch:
>           teq   r12, #0
>           moveq r0, r10                /* Marshal args: - phys_offset */
>           moveq r1, r8                 /*               - DTB address */
> -        moveq r2, r7                 /*               - CPU ID */

I don't really like making changes which are then fixed in next patches.
I'd like to see it coupled this with the previous patch.

>           beq   start_xen              /* and disappear into the land of C */
>           b     start_secondary        /* (to the appropriate entry point) */
>   
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index b26126de53..cb30d6f22e 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -586,7 +586,6 @@ launch:
>   
>           mov   x0, x20                /* Marshal args: - phys_offset */
>           mov   x1, x21                /*               - FDT */
> -        mov   x2, x24                /*               - CPU ID */
>           b     start_xen              /* and disappear into the land of C */
>   1:
>           b     start_secondary        /* (to the appropriate entry point) */
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index ccb0f181ea..6dfbba2927 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -728,8 +728,7 @@ size_t __read_mostly dcache_line_bytes;
>   
>   /* C entry point for boot CPU */
>   void __init start_xen(unsigned long boot_phys_offset,
> -                      unsigned long fdt_paddr,
> -                      unsigned long cpuid)
> +                      unsigned long fdt_paddr)
>   {
>       size_t fdt_size;
>       int cpus, i;
> 

Though:

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 06/20] xen/arm: Remove parameter cpuid from start_xen

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> The parameter cpuid is not used by start_xen. So remove it.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>   xen/arch/arm/arm32/head.S | 1 -
>   xen/arch/arm/arm64/head.S | 1 -
>   xen/arch/arm/setup.c      | 3 +--
>   3 files changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index b71d7fb11d..9f40face98 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -448,7 +448,6 @@ launch:
>           teq   r12, #0
>           moveq r0, r10                /* Marshal args: - phys_offset */
>           moveq r1, r8                 /*               - DTB address */
> -        moveq r2, r7                 /*               - CPU ID */

I don't really like making changes which are then fixed in next patches.
I'd like to see it coupled this with the previous patch.

>           beq   start_xen              /* and disappear into the land of C */
>           b     start_secondary        /* (to the appropriate entry point) */
>   
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index b26126de53..cb30d6f22e 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -586,7 +586,6 @@ launch:
>   
>           mov   x0, x20                /* Marshal args: - phys_offset */
>           mov   x1, x21                /*               - FDT */
> -        mov   x2, x24                /*               - CPU ID */
>           b     start_xen              /* and disappear into the land of C */
>   1:
>           b     start_secondary        /* (to the appropriate entry point) */
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index ccb0f181ea..6dfbba2927 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -728,8 +728,7 @@ size_t __read_mostly dcache_line_bytes;
>   
>   /* C entry point for boot CPU */
>   void __init start_xen(unsigned long boot_phys_offset,
> -                      unsigned long fdt_paddr,
> -                      unsigned long cpuid)
> +                      unsigned long fdt_paddr)
>   {
>       size_t fdt_size;
>       int cpus, i;
> 

Though:

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 07/20] xen/arm64: head: Remove unnecessary comment

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> So far, we don't init specific core initialization at boot. So remove
> the comment.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 07/20] xen/arm64: head: Remove unnecessary comment

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> So far, we don't init specific core initialization at boot. So remove
> the comment.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 08/20] xen/arm64: head: Move earlyprintk messages in .rodata.str

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> At the moment, the earlyprintk messages are interleaved with the
> instructions. This makes more difficult to read the objdump output.
> 
> Introduce a new macro to add a string in .rodata.str and use it for all
> the earlyprintk messages.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>


Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 08/20] xen/arm64: head: Move earlyprintk messages in .rodata.str

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> At the moment, the earlyprintk messages are interleaved with the
> instructions. This makes more difficult to read the objdump output.
> 
> Introduce a new macro to add a string in .rodata.str and use it for all
> the earlyprintk messages.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>


Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 09/20] xen/arm64: head: Correctly report the HW CPU ID

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> There are no reason to consider the HW CPU ID will be 0 when the
> processor is part of a uniprocessor system. At best, this will result to
> conflicting output as the rest of Xen use the value directly read from
> MPIDR_EL1.
> 
> So remove the zeroing and logic to check if the CPU is part of a
> uniprocessor system.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>


Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 09/20] xen/arm64: head: Correctly report the HW CPU ID

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> There are no reason to consider the HW CPU ID will be 0 when the
> processor is part of a uniprocessor system. At best, this will result to
> conflicting output as the rest of Xen use the value directly read from
> MPIDR_EL1.
> 
> So remove the zeroing and logic to check if the CPU is part of a
> uniprocessor system.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>


Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 10/20] xen/arm32: head: Correctly report the HW CPU ID

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> There are no reason to consider the HW CPU ID will be 0 when the
> processor is part of a uniprocessor system. At best, this will result to
> conflicting output as the rest of Xen use the value directly read from
> MPIDR.
> 
> So remove the zeroing and logic to check if the CPU is part of a
> uniprocessor system.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>


Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 10/20] xen/arm32: head: Correctly report the HW CPU ID

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> There are no reason to consider the HW CPU ID will be 0 when the
> processor is part of a uniprocessor system. At best, this will result to
> conflicting output as the rest of Xen use the value directly read from
> MPIDR.
> 
> So remove the zeroing and logic to check if the CPU is part of a
> uniprocessor system.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>


Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 11/20] xen/arm32: head: Don't set MAIR0 and MAIR1

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> The co-processor registers MAIR0 and MAIR1 are managed by EL1. So there
> are no need to initialize them during Xen boot.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>


Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 11/20] xen/arm32: head: Don't set MAIR0 and MAIR1

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> The co-processor registers MAIR0 and MAIR1 are managed by EL1. So there
> are no need to initialize them during Xen boot.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>


Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 12/20] xen/arm32: head: Always zero r3 before update a page-table entry

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> The boot code is using r2 and r3 to hold the page-table entry value.
> While r2 is always updated before storing the value, this is not always
> the case for r3.
> 
> Thankfully today, r3 will always be zero when we care. But this is
> difficult to track and error-prone.
> 
> So always zero r3 within the few instructions before the write the
> page-table entry.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>   xen/arch/arm/arm32/head.S | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 3448817aab..0536b62aec 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -270,6 +270,7 @@ cpu_init_done:
>           orr   r2, r2, #PT_UPPER(MEM) /* r2:r3 := section map */
>           orr   r2, r2, #PT_LOWER(MEM)
>           lsl   r1, r1, #3             /* r1 := Slot offset */
> +        mov   r3, #0x0>           strd  r2, r3, [r4, r1]       /* Mapping of paddr(start) */
>           mov   r6, #1                 /* r6 := identity map now in place */
>   
> @@ -377,6 +378,7 @@ paging:
>           lsl   r2, r2, #THIRD_SHIFT   /* 4K aligned paddr of UART */
>           orr   r2, r2, #PT_UPPER(DEV_L3)
>           orr   r2, r2, #PT_LOWER(DEV_L3) /* r2:r3 := 4K dev map including UART */
> +        mov   r3, #0

What's the difference between `#0x0` and `#0`? I've seen the usage is mixed across the file, but not sure why. Could it be unified?

>           strd  r2, r3, [r1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */
>   1:
>   
> @@ -388,6 +390,7 @@ paging:
>           orr   r2, r2, #PT_LOWER(PT)  /* r2:r3 := table map of xen_fixmap */
>           ldr   r4, =FIXMAP_ADDR(0)
>           mov   r4, r4, lsr #(SECOND_SHIFT - 3)   /* r4 := Slot for FIXMAP(0) */
> +        mov   r3, #0
Ditto.

>           strd  r2, r3, [r1, r4]       /* Map it in the fixmap's slot */
>   
>           /* Use a virtual address to access the UART. */
> 

With the minor comments:

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 12/20] xen/arm32: head: Always zero r3 before update a page-table entry

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> The boot code is using r2 and r3 to hold the page-table entry value.
> While r2 is always updated before storing the value, this is not always
> the case for r3.
> 
> Thankfully today, r3 will always be zero when we care. But this is
> difficult to track and error-prone.
> 
> So always zero r3 within the few instructions before the write the
> page-table entry.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>   xen/arch/arm/arm32/head.S | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 3448817aab..0536b62aec 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -270,6 +270,7 @@ cpu_init_done:
>           orr   r2, r2, #PT_UPPER(MEM) /* r2:r3 := section map */
>           orr   r2, r2, #PT_LOWER(MEM)
>           lsl   r1, r1, #3             /* r1 := Slot offset */
> +        mov   r3, #0x0>           strd  r2, r3, [r4, r1]       /* Mapping of paddr(start) */
>           mov   r6, #1                 /* r6 := identity map now in place */
>   
> @@ -377,6 +378,7 @@ paging:
>           lsl   r2, r2, #THIRD_SHIFT   /* 4K aligned paddr of UART */
>           orr   r2, r2, #PT_UPPER(DEV_L3)
>           orr   r2, r2, #PT_LOWER(DEV_L3) /* r2:r3 := 4K dev map including UART */
> +        mov   r3, #0

What's the difference between `#0x0` and `#0`? I've seen the usage is mixed across the file, but not sure why. Could it be unified?

>           strd  r2, r3, [r1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */
>   1:
>   
> @@ -388,6 +390,7 @@ paging:
>           orr   r2, r2, #PT_LOWER(PT)  /* r2:r3 := table map of xen_fixmap */
>           ldr   r4, =FIXMAP_ADDR(0)
>           mov   r4, r4, lsr #(SECOND_SHIFT - 3)   /* r4 := Slot for FIXMAP(0) */
> +        mov   r3, #0
Ditto.

>           strd  r2, r3, [r1, r4]       /* Map it in the fixmap's slot */
>   
>           /* Use a virtual address to access the UART. */
> 

With the minor comments:

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 13/20] xen/arm32: mm: Avoid to zero and clean cache for CPU0 domheap

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> The page-table walker is configured to use the same shareability and
> cacheability as the access performed when updating the page-tables. This
> means cleaning the cache for CPU0 domheap is unnecessary.
> 
> Furthermore, CPU0 page-tables are part of Xen binary and will already be
> zeroed beforehand.

IMO it is a bit confusing.
As I understand, `cpu0_dommap` resides in BSS which is not a part of the binary unlike initialized data. Yet it is unconditionally cleared during the boot on ARM32.

>  So it is pointless to zero the domheap again.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---

Though:

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 13/20] xen/arm32: mm: Avoid to zero and clean cache for CPU0 domheap

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> The page-table walker is configured to use the same shareability and
> cacheability as the access performed when updating the page-tables. This
> means cleaning the cache for CPU0 domheap is unnecessary.
> 
> Furthermore, CPU0 page-tables are part of Xen binary and will already be
> zeroed beforehand.

IMO it is a bit confusing.
As I understand, `cpu0_dommap` resides in BSS which is not a part of the binary unlike initialized data. Yet it is unconditionally cleared during the boot on ARM32.

>  So it is pointless to zero the domheap again.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---

Though:

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 14/20] xen/arm32: mm: Avoid cleaning the cache for secondary CPUs page-tables

[Andrii Anisov]

On 22.04.19 19:49, Julien Grall wrote:
> The page-table walker is configured to use the same shareability and
> cacheability as the access performed when updating the page-tables. This
> means cleaning the cache for secondary CPUs runtime page-tables is
> unnecessary.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>


-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 14/20] xen/arm32: mm: Avoid cleaning the cache for secondary CPUs page-tables

[Andrii Anisov]

On 22.04.19 19:49, Julien Grall wrote:
> The page-table walker is configured to use the same shareability and
> cacheability as the access performed when updating the page-tables. This
> means cleaning the cache for secondary CPUs runtime page-tables is
> unnecessary.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>


-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 15/20] xen/arm: mm: Use the shorter version __aligned(PAGE_SIZE) to align page-tables

[Andrii Anisov]

On 22.04.19 19:49, Julien Grall wrote:
> We currently use the very long version __attribute__((__aligned(4096)))
> to align page-tables. Thankfully there is a shorter version to make

IMO it is better to change `version` to `macro`. In order to specify it is not a compiler specific but specific to XEN.

> the code more readable.
> 
> While modifying the attribute:
>      1) Move it before the variable name as we do in other part of Xen
>      2) Switch to PAGE_SIZE instead of 4096 to make more future-proof
>      3) Mark static page-tables not used outside the file (i.e any
>         page-tables other than boot_* and xen_fixmap).
> 
> Lastly, some of the variables use __attribute__(__aligned(X * 4096)).
> However this is not necessary as page-tables are only required to be
> to be aligned to a page-size. So use __aligned(PAGE_SIZE).
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Though:

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 15/20] xen/arm: mm: Use the shorter version __aligned(PAGE_SIZE) to align page-tables

[Andrii Anisov]

On 22.04.19 19:49, Julien Grall wrote:
> We currently use the very long version __attribute__((__aligned(4096)))
> to align page-tables. Thankfully there is a shorter version to make

IMO it is better to change `version` to `macro`. In order to specify it is not a compiler specific but specific to XEN.

> the code more readable.
> 
> While modifying the attribute:
>      1) Move it before the variable name as we do in other part of Xen
>      2) Switch to PAGE_SIZE instead of 4096 to make more future-proof
>      3) Mark static page-tables not used outside the file (i.e any
>         page-tables other than boot_* and xen_fixmap).
> 
> Lastly, some of the variables use __attribute__(__aligned(X * 4096)).
> However this is not necessary as page-tables are only required to be
> to be aligned to a page-size. So use __aligned(PAGE_SIZE).
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Though:

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 16/20] xen/arm: mm: Protect Xen page-table update with a spinlock

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> The function create_xen_entries may be concurrently called. So we need
> to protect with a spinlock to avoid corruption the page-tables.

The question from me is why didn't we step into this issue before?

> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>   xen/arch/arm/mm.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index fa0f41bd07..ecde4e34df 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -969,6 +969,8 @@ enum xenmap_operation {
>       RESERVE
>   };
>   
> +static DEFINE_SPINLOCK(xen_pt_lock);
> +
>   static int create_xen_entries(enum xenmap_operation op,
>                                 unsigned long virt,
>                                 mfn_t mfn,
> @@ -980,6 +982,8 @@ static int create_xen_entries(enum xenmap_operation op,
>       lpae_t pte, *entry;
>       lpae_t *third = NULL;
>   
> +    spin_lock(&xen_pt_lock);
> +
>       for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
>       {
>           entry = &xen_second[second_linear_offset(addr)];
> @@ -1054,6 +1058,8 @@ out:
>        */
>       flush_xen_tlb_range_va(virt, PAGE_SIZE * nr_mfns);
>   
> +    spin_unlock(&xen_pt_lock);
> +
>       return rc;
>   }
>   
> 


-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 16/20] xen/arm: mm: Protect Xen page-table update with a spinlock

[Andrii Anisov]

Hello Julien,

On 22.04.19 19:49, Julien Grall wrote:
> The function create_xen_entries may be concurrently called. So we need
> to protect with a spinlock to avoid corruption the page-tables.

The question from me is why didn't we step into this issue before?

> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>   xen/arch/arm/mm.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index fa0f41bd07..ecde4e34df 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -969,6 +969,8 @@ enum xenmap_operation {
>       RESERVE
>   };
>   
> +static DEFINE_SPINLOCK(xen_pt_lock);
> +
>   static int create_xen_entries(enum xenmap_operation op,
>                                 unsigned long virt,
>                                 mfn_t mfn,
> @@ -980,6 +982,8 @@ static int create_xen_entries(enum xenmap_operation op,
>       lpae_t pte, *entry;
>       lpae_t *third = NULL;
>   
> +    spin_lock(&xen_pt_lock);
> +
>       for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
>       {
>           entry = &xen_second[second_linear_offset(addr)];
> @@ -1054,6 +1058,8 @@ out:
>        */
>       flush_xen_tlb_range_va(virt, PAGE_SIZE * nr_mfns);
>   
> +    spin_unlock(&xen_pt_lock);
> +
>       return rc;
>   }
>   
> 


-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 17/20] xen/arm: mm: Initialize page-tables earlier

[Andrii Anisov]

On 22.04.19 19:49, Julien Grall wrote:
> Since commit f60658c6ae "xen/arm: Stop relocating Xen", the function
> setup_page_tables() does not require any information from the FDT.
> 
> So the initialization of the page-tables can be done much earlier in the
> boot process. The earliest setup_page_tables() can be called is after
> traps have been initialized, so we can get backtrace if an error
> occurred.
> 
> Moving the initialization of the page-tables also avoid the dance to map
> the FDT again in the new set of page-tables.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 17/20] xen/arm: mm: Initialize page-tables earlier

[Andrii Anisov]

On 22.04.19 19:49, Julien Grall wrote:
> Since commit f60658c6ae "xen/arm: Stop relocating Xen", the function
> setup_page_tables() does not require any information from the FDT.
> 
> So the initialization of the page-tables can be done much earlier in the
> boot process. The earliest setup_page_tables() can be called is after
> traps have been initialized, so we can get backtrace if an error
> occurred.
> 
> Moving the initialization of the page-tables also avoid the dance to map
> the FDT again in the new set of page-tables.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 18/20] xen/arm: mm: Check start is always before end in {destroy, modify}_xen_mappings

[Andrii Anisov]

On 22.04.19 19:49, Julien Grall wrote:
> The two helpers {destroy, modify}_xen_mappings don't check that the
> start is always before the end. This should never happen but if it
> happens, it will result to unexpected behavior.
> 
> Catch such issues earlier on by adding an ASSERT in destroy_xen_mappings
> and modify_xen_mappings.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>


Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 18/20] xen/arm: mm: Check start is always before end in {destroy, modify}_xen_mappings

[Andrii Anisov]

On 22.04.19 19:49, Julien Grall wrote:
> The two helpers {destroy, modify}_xen_mappings don't check that the
> start is always before the end. This should never happen but if it
> happens, it will result to unexpected behavior.
> 
> Catch such issues earlier on by adding an ASSERT in destroy_xen_mappings
> and modify_xen_mappings.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>


Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 19/20] xen/arm: Pair call to set_fixmap with call to clear_fixmap in copy_from_paddr

[Andrii Anisov]

On 22.04.19 19:49, Julien Grall wrote:
> At the moment, set_fixmap may replace a valid entry without following
> the break-before-make sequence. This may result to TLB conflict abort.
> 
> Rather than dealing with Break-Before-Make in set_fixmap, every call to
> set_fixmap is paired with a call to clear_fixmap.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 19/20] xen/arm: Pair call to set_fixmap with call to clear_fixmap in copy_from_paddr

[Andrii Anisov]

On 22.04.19 19:49, Julien Grall wrote:
> At the moment, set_fixmap may replace a valid entry without following
> the break-before-make sequence. This may result to TLB conflict abort.
> 
> Rather than dealing with Break-Before-Make in set_fixmap, every call to
> set_fixmap is paired with a call to clear_fixmap.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 03/20] xen/arm: processor: Use _BITUL instead of _AC(1, U) in SCTLR_ defines

[Julien Grall]

On 03/05/2019 16:56, Andrii Anisov wrote:
> Hello Julien,
> 
> On 22.04.19 19:49, Julien Grall wrote:
>> The newly introduced macro _BITUL makes the code more readable.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/include/asm-arm/processor.h | 18 +++++++++---------
>>   1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
>> index c6f56490b3..1a143fb6a3 100644
>> --- a/xen/include/asm-arm/processor.h
>> +++ b/xen/include/asm-arm/processor.h
>> @@ -115,20 +115,20 @@
>>   /* Bits specific to SCTLR_EL1 for Arm32 */
>> -#define SCTLR_A32_EL1_V     (_AC(1,U)<<13)
>> +#define SCTLR_A32_EL1_V     _BITUL(13)
>>   /* Common bits for SCTLR_ELx for Arm32 */
>> -#define SCTLR_A32_ELx_TE    (_AC(1,U)<<30)
>> -#define SCTLR_A32_ELx_FI    (_AC(1,U)<<21)
>> +#define SCTLR_A32_ELx_TE    _BITUL(30)
>> +#define SCTLR_A32_ELx_FI    _BITUL(21)
>>   /* Common bits for SCTLR_ELx on all architectures */
>> -#define SCTLR_Axx_ELx_EE    (_AC(1,U)<<25)
>> -#define SCTLR_Axx_ELx_WXN   (_AC(1,U)<<19)
>> -#define SCTLR_Axx_ELx_I     (_AC(1,U)<<12)
>> -#define SCTLR_Axx_ELx_C     (_AC(1,U)<<2)
>> -#define SCTLR_Axx_ELx_A     (_AC(1,U)<<1)
>> -#define SCTLR_Axx_ELx_M     (_AC(1,U)<<0)
>> +#define SCTLR_Axx_ELx_EE    _BITUL(25)
>> +#define SCTLR_Axx_ELx_WXN   _BITUL(19)
>> +#define SCTLR_Axx_ELx_I     _BITUL(12)
>> +#define SCTLR_Axx_ELx_C     _BITUL(2)
>> +#define SCTLR_Axx_ELx_A     _BITUL(1)
>> +#define SCTLR_Axx_ELx_M     _BITUL(0)
>>   #define HSCTLR_BASE     _AC(0x30c51878,U)
>>
> 
> Resolution of the dispute with Jan about [PATCH 01/20] is required first.

I don't understand what is your "second". Does it mean you are happy with the 
idea of the patch but we should agree on the naming first?

Cheers,

> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 03/20] xen/arm: processor: Use _BITUL instead of _AC(1, U) in SCTLR_ defines

[Julien Grall]

On 03/05/2019 16:56, Andrii Anisov wrote:
> Hello Julien,
> 
> On 22.04.19 19:49, Julien Grall wrote:
>> The newly introduced macro _BITUL makes the code more readable.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/include/asm-arm/processor.h | 18 +++++++++---------
>>   1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
>> index c6f56490b3..1a143fb6a3 100644
>> --- a/xen/include/asm-arm/processor.h
>> +++ b/xen/include/asm-arm/processor.h
>> @@ -115,20 +115,20 @@
>>   /* Bits specific to SCTLR_EL1 for Arm32 */
>> -#define SCTLR_A32_EL1_V     (_AC(1,U)<<13)
>> +#define SCTLR_A32_EL1_V     _BITUL(13)
>>   /* Common bits for SCTLR_ELx for Arm32 */
>> -#define SCTLR_A32_ELx_TE    (_AC(1,U)<<30)
>> -#define SCTLR_A32_ELx_FI    (_AC(1,U)<<21)
>> +#define SCTLR_A32_ELx_TE    _BITUL(30)
>> +#define SCTLR_A32_ELx_FI    _BITUL(21)
>>   /* Common bits for SCTLR_ELx on all architectures */
>> -#define SCTLR_Axx_ELx_EE    (_AC(1,U)<<25)
>> -#define SCTLR_Axx_ELx_WXN   (_AC(1,U)<<19)
>> -#define SCTLR_Axx_ELx_I     (_AC(1,U)<<12)
>> -#define SCTLR_Axx_ELx_C     (_AC(1,U)<<2)
>> -#define SCTLR_Axx_ELx_A     (_AC(1,U)<<1)
>> -#define SCTLR_Axx_ELx_M     (_AC(1,U)<<0)
>> +#define SCTLR_Axx_ELx_EE    _BITUL(25)
>> +#define SCTLR_Axx_ELx_WXN   _BITUL(19)
>> +#define SCTLR_Axx_ELx_I     _BITUL(12)
>> +#define SCTLR_Axx_ELx_C     _BITUL(2)
>> +#define SCTLR_Axx_ELx_A     _BITUL(1)
>> +#define SCTLR_Axx_ELx_M     _BITUL(0)
>>   #define HSCTLR_BASE     _AC(0x30c51878,U)
>>
> 
> Resolution of the dispute with Jan about [PATCH 01/20] is required first.

I don't understand what is your "second". Does it mean you are happy with the 
idea of the patch but we should agree on the naming first?

Cheers,

> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 04/20] xen/arm: Rework HSCTLR_BASE

[Julien Grall]

Hi,

On 03/05/2019 16:56, Andrii Anisov wrote:
> On 22.04.19 19:49, Julien Grall wrote:
>>   /* HCR Hyp Configuration Register */
>>   #define HCR_RW          (_AC(1,UL)<<31) /* Register Width, ARM64 only */
>>
> 
> Resolution of the dispute with Jan about [PATCH 01/20] is required first.

I don't understand what is your "second". Does it mean you are happy with the 
idea of the patch but we should agree on the naming first?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 04/20] xen/arm: Rework HSCTLR_BASE

[Julien Grall]

Hi,

On 03/05/2019 16:56, Andrii Anisov wrote:
> On 22.04.19 19:49, Julien Grall wrote:
>>   /* HCR Hyp Configuration Register */
>>   #define HCR_RW          (_AC(1,UL)<<31) /* Register Width, ARM64 only */
>>
> 
> Resolution of the dispute with Jan about [PATCH 01/20] is required first.

I don't understand what is your "second". Does it mean you are happy with the 
idea of the patch but we should agree on the naming first?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 03/20] xen/arm: processor: Use _BITUL instead of _AC(1, U) in SCTLR_ defines

[Andrii Anisov]

On 03.05.19 19:09, Julien Grall wrote:
> I don't understand what is your "second". Does it mean you are happy with the idea of the patch but we should agree on the naming first?

Yes, right you are.
Sorry for the mess.

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 03/20] xen/arm: processor: Use _BITUL instead of _AC(1, U) in SCTLR_ defines

[Andrii Anisov]

On 03.05.19 19:09, Julien Grall wrote:
> I don't understand what is your "second". Does it mean you are happy with the idea of the patch but we should agree on the naming first?

Yes, right you are.
Sorry for the mess.

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 05/20] xen/arm: Rework secondary_start prototype

[Julien Grall]

On 03/05/2019 16:56, Andrii Anisov wrote:
> Hello Julien,
> 
> On 22.04.19 19:49, Julien Grall wrote:
>> None of the parameters of secondary_start are actually used. So turn
>> secondary_start to a function with no parameters.
>>
>> Also modify the assembly code to avoid setting-up the registers before
>> calling secondary_start.
> 
> What is not really mandatory.

So...? You just don't setup parameter when it is not necessary. The more it is 
quite confusing for a reader to see the registers are setup but not used by the 
caller.

> 
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> Though:
> 
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

Thank you.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 05/20] xen/arm: Rework secondary_start prototype

[Julien Grall]

On 03/05/2019 16:56, Andrii Anisov wrote:
> Hello Julien,
> 
> On 22.04.19 19:49, Julien Grall wrote:
>> None of the parameters of secondary_start are actually used. So turn
>> secondary_start to a function with no parameters.
>>
>> Also modify the assembly code to avoid setting-up the registers before
>> calling secondary_start.
> 
> What is not really mandatory.

So...? You just don't setup parameter when it is not necessary. The more it is 
quite confusing for a reader to see the registers are setup but not used by the 
caller.

> 
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> Though:
> 
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

Thank you.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 06/20] xen/arm: Remove parameter cpuid from start_xen

[Julien Grall]

Hi,

On 03/05/2019 16:56, Andrii Anisov wrote:
> On 22.04.19 19:49, Julien Grall wrote:
>> The parameter cpuid is not used by start_xen. So remove it.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/arm32/head.S | 1 -
>>   xen/arch/arm/arm64/head.S | 1 -
>>   xen/arch/arm/setup.c      | 3 +--
>>   3 files changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>> index b71d7fb11d..9f40face98 100644
>> --- a/xen/arch/arm/arm32/head.S
>> +++ b/xen/arch/arm/arm32/head.S
>> @@ -448,7 +448,6 @@ launch:
>>           teq   r12, #0
>>           moveq r0, r10                /* Marshal args: - phys_offset */
>>           moveq r1, r8                 /*               - DTB address */
>> -        moveq r2, r7                 /*               - CPU ID */
> 
> I don't really like making changes which are then fixed in next patches.
> I'd like to see it coupled this with the previous patch.

They are two different changes... one deal with start_xen the other deal with 
secondary_start.

I can offer to reshuffle the patches so this one is before #5, but not merge them.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 06/20] xen/arm: Remove parameter cpuid from start_xen

[Julien Grall]

Hi,

On 03/05/2019 16:56, Andrii Anisov wrote:
> On 22.04.19 19:49, Julien Grall wrote:
>> The parameter cpuid is not used by start_xen. So remove it.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/arm32/head.S | 1 -
>>   xen/arch/arm/arm64/head.S | 1 -
>>   xen/arch/arm/setup.c      | 3 +--
>>   3 files changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>> index b71d7fb11d..9f40face98 100644
>> --- a/xen/arch/arm/arm32/head.S
>> +++ b/xen/arch/arm/arm32/head.S
>> @@ -448,7 +448,6 @@ launch:
>>           teq   r12, #0
>>           moveq r0, r10                /* Marshal args: - phys_offset */
>>           moveq r1, r8                 /*               - DTB address */
>> -        moveq r2, r7                 /*               - CPU ID */
> 
> I don't really like making changes which are then fixed in next patches.
> I'd like to see it coupled this with the previous patch.

They are two different changes... one deal with start_xen the other deal with 
secondary_start.

I can offer to reshuffle the patches so this one is before #5, but not merge them.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 04/20] xen/arm: Rework HSCTLR_BASE

[Andrii Anisov]

On 03.05.19 19:10, Julien Grall wrote:
> I don't understand what is your "second". Does it mean you are happy with the idea of the patch but we should agree on the naming first?

Yes, I'm ok with the change.
Actually I like the part with HSCTLR_CLEAR as a compilation time guard.
But naming should be agreed first.

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 04/20] xen/arm: Rework HSCTLR_BASE

[Andrii Anisov]

On 03.05.19 19:10, Julien Grall wrote:
> I don't understand what is your "second". Does it mean you are happy with the idea of the patch but we should agree on the naming first?

Yes, I'm ok with the change.
Actually I like the part with HSCTLR_CLEAR as a compilation time guard.
But naming should be agreed first.

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 06/20] xen/arm: Remove parameter cpuid from start_xen

[Andrii Anisov]

On 03.05.19 19:17, Julien Grall wrote:
> I can offer to reshuffle the patches so this one is before #5, but not merge them.

Good option. I like it.

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 06/20] xen/arm: Remove parameter cpuid from start_xen

[Andrii Anisov]

On 03.05.19 19:17, Julien Grall wrote:
> I can offer to reshuffle the patches so this one is before #5, but not merge them.

Good option. I like it.

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 12/20] xen/arm32: head: Always zero r3 before update a page-table entry

[Julien Grall]

On 03/05/2019 16:57, Andrii Anisov wrote:
> Hello Julien,
> 
> On 22.04.19 19:49, Julien Grall wrote:
>> The boot code is using r2 and r3 to hold the page-table entry value.
>> While r2 is always updated before storing the value, this is not always
>> the case for r3.
>>
>> Thankfully today, r3 will always be zero when we care. But this is
>> difficult to track and error-prone.
>>
>> So always zero r3 within the few instructions before the write the
>> page-table entry.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/arm32/head.S | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>> index 3448817aab..0536b62aec 100644
>> --- a/xen/arch/arm/arm32/head.S
>> +++ b/xen/arch/arm/arm32/head.S
>> @@ -270,6 +270,7 @@ cpu_init_done:
>>           orr   r2, r2, #PT_UPPER(MEM) /* r2:r3 := section map */
>>           orr   r2, r2, #PT_LOWER(MEM)
>>           lsl   r1, r1, #3             /* r1 := Slot offset */
>> +        mov   r3, #0x0>           strd  r2, r3, [r4, r1]       /* Mapping of 
>> paddr(start) */
>>           mov   r6, #1                 /* r6 := identity map now in place */
>> @@ -377,6 +378,7 @@ paging:
>>           lsl   r2, r2, #THIRD_SHIFT   /* 4K aligned paddr of UART */
>>           orr   r2, r2, #PT_UPPER(DEV_L3)
>>           orr   r2, r2, #PT_LOWER(DEV_L3) /* r2:r3 := 4K dev map including 
>> UART */
>> +        mov   r3, #0
> 
> What's the difference between `#0x0` and `#0`? I've seen the usage is mixed 
> across the file, but not sure why. Could it be unified?

No difference, matter of taste. The file seems to use 0x0 in more places, so I 
will use 0x0 in this patch as well.

> 
>>           strd  r2, r3, [r1, #(FIXMAP_CONSOLE*8)] /* Map it in the first 
>> fixmap's slot */
>>   1:
>> @@ -388,6 +390,7 @@ paging:
>>           orr   r2, r2, #PT_LOWER(PT)  /* r2:r3 := table map of xen_fixmap */
>>           ldr   r4, =FIXMAP_ADDR(0)
>>           mov   r4, r4, lsr #(SECOND_SHIFT - 3)   /* r4 := Slot for FIXMAP(0) */
>> +        mov   r3, #0
> Ditto.
> 
>>           strd  r2, r3, [r1, r4]       /* Map it in the fixmap's slot */
>>           /* Use a virtual address to access the UART. */
>>
> 
> With the minor comments:
> 
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

Thank you.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 12/20] xen/arm32: head: Always zero r3 before update a page-table entry

[Julien Grall]

On 03/05/2019 16:57, Andrii Anisov wrote:
> Hello Julien,
> 
> On 22.04.19 19:49, Julien Grall wrote:
>> The boot code is using r2 and r3 to hold the page-table entry value.
>> While r2 is always updated before storing the value, this is not always
>> the case for r3.
>>
>> Thankfully today, r3 will always be zero when we care. But this is
>> difficult to track and error-prone.
>>
>> So always zero r3 within the few instructions before the write the
>> page-table entry.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/arm32/head.S | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>> index 3448817aab..0536b62aec 100644
>> --- a/xen/arch/arm/arm32/head.S
>> +++ b/xen/arch/arm/arm32/head.S
>> @@ -270,6 +270,7 @@ cpu_init_done:
>>           orr   r2, r2, #PT_UPPER(MEM) /* r2:r3 := section map */
>>           orr   r2, r2, #PT_LOWER(MEM)
>>           lsl   r1, r1, #3             /* r1 := Slot offset */
>> +        mov   r3, #0x0>           strd  r2, r3, [r4, r1]       /* Mapping of 
>> paddr(start) */
>>           mov   r6, #1                 /* r6 := identity map now in place */
>> @@ -377,6 +378,7 @@ paging:
>>           lsl   r2, r2, #THIRD_SHIFT   /* 4K aligned paddr of UART */
>>           orr   r2, r2, #PT_UPPER(DEV_L3)
>>           orr   r2, r2, #PT_LOWER(DEV_L3) /* r2:r3 := 4K dev map including 
>> UART */
>> +        mov   r3, #0
> 
> What's the difference between `#0x0` and `#0`? I've seen the usage is mixed 
> across the file, but not sure why. Could it be unified?

No difference, matter of taste. The file seems to use 0x0 in more places, so I 
will use 0x0 in this patch as well.

> 
>>           strd  r2, r3, [r1, #(FIXMAP_CONSOLE*8)] /* Map it in the first 
>> fixmap's slot */
>>   1:
>> @@ -388,6 +390,7 @@ paging:
>>           orr   r2, r2, #PT_LOWER(PT)  /* r2:r3 := table map of xen_fixmap */
>>           ldr   r4, =FIXMAP_ADDR(0)
>>           mov   r4, r4, lsr #(SECOND_SHIFT - 3)   /* r4 := Slot for FIXMAP(0) */
>> +        mov   r3, #0
> Ditto.
> 
>>           strd  r2, r3, [r1, r4]       /* Map it in the fixmap's slot */
>>           /* Use a virtual address to access the UART. */
>>
> 
> With the minor comments:
> 
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

Thank you.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 13/20] xen/arm32: mm: Avoid to zero and clean cache for CPU0 domheap

[Julien Grall]

On 03/05/2019 16:57, Andrii Anisov wrote:
> Hello Julien,

Hi,

> 
> On 22.04.19 19:49, Julien Grall wrote:
>> The page-table walker is configured to use the same shareability and
>> cacheability as the access performed when updating the page-tables. This
>> means cleaning the cache for CPU0 domheap is unnecessary.
>>
>> Furthermore, CPU0 page-tables are part of Xen binary and will already be
>> zeroed beforehand.
> 
> IMO it is a bit confusing.
> As I understand, `cpu0_dommap` resides in BSS which is not a part of the binary 
> unlike initialized data. Yet it is unconditionally cleared during the boot on 
> ARM32.

In C, uninitialized global variable will be zero by default. It is a bit of 
waste to allocate space in the binary for them. So the compiler will commonly 
put them in a section BSS that are going to be zeroed when at launch.

On Arm32, this is always done in CPU0 at early boot. For Arm64, UEFI will do it 
for us, so we don't want to do it when using UEFI as we may override global

The reason I chose to say "will always be zeroed beforehand" than specifically 
mention "BSS" is I wasn't entirely convinced the compiler will always put in BSS.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 13/20] xen/arm32: mm: Avoid to zero and clean cache for CPU0 domheap

[Julien Grall]

On 03/05/2019 16:57, Andrii Anisov wrote:
> Hello Julien,

Hi,

> 
> On 22.04.19 19:49, Julien Grall wrote:
>> The page-table walker is configured to use the same shareability and
>> cacheability as the access performed when updating the page-tables. This
>> means cleaning the cache for CPU0 domheap is unnecessary.
>>
>> Furthermore, CPU0 page-tables are part of Xen binary and will already be
>> zeroed beforehand.
> 
> IMO it is a bit confusing.
> As I understand, `cpu0_dommap` resides in BSS which is not a part of the binary 
> unlike initialized data. Yet it is unconditionally cleared during the boot on 
> ARM32.

In C, uninitialized global variable will be zero by default. It is a bit of 
waste to allocate space in the binary for them. So the compiler will commonly 
put them in a section BSS that are going to be zeroed when at launch.

On Arm32, this is always done in CPU0 at early boot. For Arm64, UEFI will do it 
for us, so we don't want to do it when using UEFI as we may override global

The reason I chose to say "will always be zeroed beforehand" than specifically 
mention "BSS" is I wasn't entirely convinced the compiler will always put in BSS.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 15/20] xen/arm: mm: Use the shorter version __aligned(PAGE_SIZE) to align page-tables

[Julien Grall]

On 03/05/2019 16:58, Andrii Anisov wrote:
> 
> 
> On 22.04.19 19:49, Julien Grall wrote:
>> We currently use the very long version __attribute__((__aligned(4096)))
>> to align page-tables. Thankfully there is a shorter version to make
> 
> IMO it is better to change `version` to `macro`. In order to specify it is not a 
> compiler specific but specific to XEN.

I will reword to "Thankfully Xen provides an handy macro that will make the code 
more readable".

> 
>> the code more readable.
>>
>> While modifying the attribute:
>>      1) Move it before the variable name as we do in other part of Xen
>>      2) Switch to PAGE_SIZE instead of 4096 to make more future-proof
>>      3) Mark static page-tables not used outside the file (i.e any
>>         page-tables other than boot_* and xen_fixmap).
>>
>> Lastly, some of the variables use __attribute__(__aligned(X * 4096)).
>> However this is not necessary as page-tables are only required to be
>> to be aligned to a page-size. So use __aligned(PAGE_SIZE).
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Though:
> 
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 15/20] xen/arm: mm: Use the shorter version __aligned(PAGE_SIZE) to align page-tables

[Julien Grall]

On 03/05/2019 16:58, Andrii Anisov wrote:
> 
> 
> On 22.04.19 19:49, Julien Grall wrote:
>> We currently use the very long version __attribute__((__aligned(4096)))
>> to align page-tables. Thankfully there is a shorter version to make
> 
> IMO it is better to change `version` to `macro`. In order to specify it is not a 
> compiler specific but specific to XEN.

I will reword to "Thankfully Xen provides an handy macro that will make the code 
more readable".

> 
>> the code more readable.
>>
>> While modifying the attribute:
>>      1) Move it before the variable name as we do in other part of Xen
>>      2) Switch to PAGE_SIZE instead of 4096 to make more future-proof
>>      3) Mark static page-tables not used outside the file (i.e any
>>         page-tables other than boot_* and xen_fixmap).
>>
>> Lastly, some of the variables use __attribute__(__aligned(X * 4096)).
>> However this is not necessary as page-tables are only required to be
>> to be aligned to a page-size. So use __aligned(PAGE_SIZE).
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Though:
> 
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 16/20] xen/arm: mm: Protect Xen page-table update with a spinlock

[Julien Grall]

On 03/05/2019 16:59, Andrii Anisov wrote:
> Hello Julien,

Hi,

> 
> On 22.04.19 19:49, Julien Grall wrote:
>> The function create_xen_entries may be concurrently called. So we need
>> to protect with a spinlock to avoid corruption the page-tables.
> 
> The question from me is why didn't we step into this issue before?

TLDR; because xen page-tables are not that often modified after boot. Yet it is 
still possible to race.

At the moment, create_xen_entries() can only modify the VA range 0 - 2GB. In 
that range, we can modify at runtime the VMAP area. One potential issue is
a vmap issued at the same time.

While the range allocation is protected by a lock (see vm_alloc), the mapping is 
not. So it would be possible to end up modifying the page-table at the same. 
That could blow up if for instance, the second-level entry is invalid as we 
would need to allocate memory (only one can win that race).

In general, it is a saner approach to try to serialize the modifications in the 
page-tables. So you can safely read an entry, check it and then update it.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 16/20] xen/arm: mm: Protect Xen page-table update with a spinlock

[Julien Grall]

On 03/05/2019 16:59, Andrii Anisov wrote:
> Hello Julien,

Hi,

> 
> On 22.04.19 19:49, Julien Grall wrote:
>> The function create_xen_entries may be concurrently called. So we need
>> to protect with a spinlock to avoid corruption the page-tables.
> 
> The question from me is why didn't we step into this issue before?

TLDR; because xen page-tables are not that often modified after boot. Yet it is 
still possible to race.

At the moment, create_xen_entries() can only modify the VA range 0 - 2GB. In 
that range, we can modify at runtime the VMAP area. One potential issue is
a vmap issued at the same time.

While the range allocation is protected by a lock (see vm_alloc), the mapping is 
not. So it would be possible to end up modifying the page-table at the same. 
That could blow up if for instance, the second-level entry is invalid as we 
would need to allocate memory (only one can win that race).

In general, it is a saner approach to try to serialize the modifications in the 
page-tables. So you can safely read an entry, check it and then update it.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 15/20] xen/arm: mm: Use the shorter version __aligned(PAGE_SIZE) to align page-tables

[Andrii Anisov]

On 03.05.19 20:09, Julien Grall wrote:
> I will reword to "Thankfully Xen provides an handy macro that will make the code more readable".

Good.

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 15/20] xen/arm: mm: Use the shorter version __aligned(PAGE_SIZE) to align page-tables

[Andrii Anisov]

On 03.05.19 20:09, Julien Grall wrote:
> I will reword to "Thankfully Xen provides an handy macro that will make the code more readable".

Good.

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 16/20] xen/arm: mm: Protect Xen page-table update with a spinlock

[Andrii Anisov]

Hello Julien,

On 03.05.19 20:19, Julien Grall wrote:
> TLDR; because xen page-tables are not that often modified after boot. Yet it is still possible to race.
> 
> At the moment, create_xen_entries() can only modify the VA range 0 - 2GB. In that range, we can modify at runtime the VMAP area. One potential issue is
> a vmap issued at the same time.
> 
> While the range allocation is protected by a lock (see vm_alloc), the mapping is not. So it would be possible to end up modifying the page-table at the same. That could blow up if for instance, the second-level entry is invalid as we would need to allocate memory (only one can win that race).

I understand the potential race, but still wondering why didn't we see those issues. Maybe we are too lucky.

> In general, it is a saner approach to try to serialize the modifications in the page-tables. So you can safely read an entry, check it and then update it.

Yet, I think we would stick at these locks for now.

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 16/20] xen/arm: mm: Protect Xen page-table update with a spinlock

[Andrii Anisov]

Hello Julien,

On 03.05.19 20:19, Julien Grall wrote:
> TLDR; because xen page-tables are not that often modified after boot. Yet it is still possible to race.
> 
> At the moment, create_xen_entries() can only modify the VA range 0 - 2GB. In that range, we can modify at runtime the VMAP area. One potential issue is
> a vmap issued at the same time.
> 
> While the range allocation is protected by a lock (see vm_alloc), the mapping is not. So it would be possible to end up modifying the page-table at the same. That could blow up if for instance, the second-level entry is invalid as we would need to allocate memory (only one can win that race).

I understand the potential race, but still wondering why didn't we see those issues. Maybe we are too lucky.

> In general, it is a saner approach to try to serialize the modifications in the page-tables. So you can safely read an entry, check it and then update it.

Yet, I think we would stick at these locks for now.

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 16/20] xen/arm: mm: Protect Xen page-table update with a spinlock

[Andrii Anisov]

On 22.04.19 19:49, Julien Grall wrote:
> The function create_xen_entries may be concurrently called. So we need
> to protect with a spinlock to avoid corruption the page-tables.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 16/20] xen/arm: mm: Protect Xen page-table update with a spinlock

[Andrii Anisov]

On 22.04.19 19:49, Julien Grall wrote:
> The function create_xen_entries may be concurrently called. So we need
> to protect with a spinlock to avoid corruption the page-tables.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 13/20] xen/arm32: mm: Avoid to zero and clean cache for CPU0 domheap

[Andrii Anisov]

On 03.05.19 20:06, Julien Grall wrote:
> In C, uninitialized global variable will be zero by default. It is a bit of waste to allocate space in the binary for them. So the compiler will commonly put them in a section BSS that are going to be zeroed when at launch.
> 
> On Arm32, this is always done in CPU0 at early boot. For Arm64, UEFI will do it for us, so we don't want to do it when using UEFI as we may override global
> 
> The reason I chose to say "will always be zeroed beforehand" than specifically mention "BSS" is I wasn't entirely convinced the compiler will always put in BSS.

OK.

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 13/20] xen/arm32: mm: Avoid to zero and clean cache for CPU0 domheap

[Andrii Anisov]

On 03.05.19 20:06, Julien Grall wrote:
> In C, uninitialized global variable will be zero by default. It is a bit of waste to allocate space in the binary for them. So the compiler will commonly put them in a section BSS that are going to be zeroed when at launch.
> 
> On Arm32, this is always done in CPU0 at early boot. For Arm64, UEFI will do it for us, so we don't want to do it when using UEFI as we may override global
> 
> The reason I chose to say "will always be zeroed beforehand" than specifically mention "BSS" is I wasn't entirely convinced the compiler will always put in BSS.

OK.

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 16/20] xen/arm: mm: Protect Xen page-table update with a spinlock

[Julien Grall]

On 5/6/19 9:20 AM, Andrii Anisov wrote:
> Hello Julien,
> 
> On 03.05.19 20:19, Julien Grall wrote:
>> TLDR; because xen page-tables are not that often modified after boot. 
>> Yet it is still possible to race.
>>
>> At the moment, create_xen_entries() can only modify the VA range 0 - 
>> 2GB. In that range, we can modify at runtime the VMAP area. One 
>> potential issue is
>> a vmap issued at the same time.
>>
>> While the range allocation is protected by a lock (see vm_alloc), the 
>> mapping is not. So it would be possible to end up modifying the 
>> page-table at the same. That could blow up if for instance, the 
>> second-level entry is invalid as we would need to allocate memory 
>> (only one can win that race).
> 
> I understand the potential race, but still wondering why didn't we see 
> those issues. Maybe we are too lucky.

As I pointed out above, the VMAP area is not often updated after boot. 
Furthermore, to hit the race, you need to insert 2 entries covered by 
the same unallocated 3rd-level table at the same time. As the 3rd-level 
table are only allocated once and never released, you probably have more 
chance to win at the lottery over hitting the race.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 16/20] xen/arm: mm: Protect Xen page-table update with a spinlock

[Julien Grall]

On 5/6/19 9:20 AM, Andrii Anisov wrote:
> Hello Julien,
> 
> On 03.05.19 20:19, Julien Grall wrote:
>> TLDR; because xen page-tables are not that often modified after boot. 
>> Yet it is still possible to race.
>>
>> At the moment, create_xen_entries() can only modify the VA range 0 - 
>> 2GB. In that range, we can modify at runtime the VMAP area. One 
>> potential issue is
>> a vmap issued at the same time.
>>
>> While the range allocation is protected by a lock (see vm_alloc), the 
>> mapping is not. So it would be possible to end up modifying the 
>> page-table at the same. That could blow up if for instance, the 
>> second-level entry is invalid as we would need to allocate memory 
>> (only one can win that race).
> 
> I understand the potential race, but still wondering why didn't we see 
> those issues. Maybe we are too lucky.

As I pointed out above, the VMAP area is not often updated after boot. 
Furthermore, to hit the race, you need to insert 2 entries covered by 
the same unallocated 3rd-level table at the same time. As the 3rd-level 
table are only allocated once and never released, you probably have more 
chance to win at the lottery over hitting the race.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel