[Xen-devel] [PATCH 0/7] xen/arm: Xen hardening for newer Armv8

[Xen-devel] [PATCH 0/7] xen/arm: Xen hardening for newer Armv8

[Julien Grall]

Hi all,

This is a not-yet complete series to harden Xen for later revision of
Armv8. The main goals are:
    - Reducing the number of BUG_ON() to check guest state
    - Fix system registers size as they are always 64-bit on AArch64
    (not 32-bit!).

There are more work to do. I will send them in smaller batch as I find
spare time to rework bits of Xen.

Note that patch #1 was already sent separately but added here for convenience.

Cheers,

Julien Grall (7):
  xen/public: arch-arm: Restrict the visibility of struct
    vcpu_guest_core_regs
  xen/arm: SCTLR_EL1 is a 64-bit register on Arm64
  xen/arm: Rework psr_mode_is_32bit()
  xen/arm: traps: Avoid using BUG_ON() in _show_registers()
  xen/arm: traps: Avoid BUG_ON() in do_trap_brk()
  xen/arm: vsmc: The function identifier is always 32-bit
  xen/arm: types: Specify the zero padding in the definition of
    PRIregister

 tools/xentrace/xenctx.c       |  4 ++-
 xen/arch/arm/guest_walk.c     |  2 +-
 xen/arch/arm/traps.c          | 73 ++++++++++++++++++++-----------------------
 xen/arch/arm/vsmc.c           |  4 +--
 xen/include/asm-arm/domain.h  |  3 +-
 xen/include/asm-arm/p2m.h     |  4 +--
 xen/include/asm-arm/regs.h    |  9 +++++-
 xen/include/asm-arm/types.h   |  4 +--
 xen/include/public/arch-arm.h | 28 ++++++++---------
 9 files changed, 68 insertions(+), 63 deletions(-)

-- 
2.11.0


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

[Xen-devel] [PATCH 1/7] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Julien Grall]

Currently, the structure vcpu_guest_core_regs is part of the public API.
This implies that any change in the structure should be backward
compatible.

However, the structure is only needed by the tools and Xen. It is also
not expected to be ever used outside of that context. So we could save us
some headache by only declaring the structure for Xen and tools.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>

---
    This is a follow-up of the discussion [1].

    [1] <3c245c5b-51c6-1d0e-ad6c-42414573166f@arm.com>

    Changes in v3:
        - Avoid introduce a new #ifdef in the header by moving the
        definitions later on.
---
 xen/include/public/arch-arm.h | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 3e8cdc151d..7ce139a0f5 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -197,6 +197,18 @@
     } while ( 0 )
 #define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
 
+typedef uint64_t xen_pfn_t;
+#define PRI_xen_pfn PRIx64
+#define PRIu_xen_pfn PRIu64
+
+/* Maximum number of virtual CPUs in legacy multi-processor guests. */
+/* Only one. All other VCPUS must use VCPUOP_register_vcpu_info */
+#define XEN_LEGACY_MAX_VCPUS 1
+
+typedef uint64_t xen_ulong_t;
+#define PRI_xen_ulong PRIx64
+
+#if defined(__XEN__) || defined(__XEN_TOOLS__)
 #if defined(__GNUC__) && !defined(__STRICT_ANSI__)
 /* Anonymous union includes both 32- and 64-bit names (e.g., r0/x0). */
 # define __DECL_REG(n64, n32) union {          \
@@ -272,18 +284,6 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_core_regs_t);
 
 #undef __DECL_REG
 
-typedef uint64_t xen_pfn_t;
-#define PRI_xen_pfn PRIx64
-#define PRIu_xen_pfn PRIu64
-
-/* Maximum number of virtual CPUs in legacy multi-processor guests. */
-/* Only one. All other VCPUS must use VCPUOP_register_vcpu_info */
-#define XEN_LEGACY_MAX_VCPUS 1
-
-typedef uint64_t xen_ulong_t;
-#define PRI_xen_ulong PRIx64
-
-#if defined(__XEN__) || defined(__XEN_TOOLS__)
 struct vcpu_guest_context {
 #define _VGCF_online                   0
 #define VGCF_online                    (1<<_VGCF_online)
-- 
2.11.0


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

[Xen-devel] [PATCH 2/7] xen/arm: SCTLR_EL1 is a 64-bit register on Arm64

[Julien Grall]

On Arm64, system registers are always 64-bit including SCTLR_EL1.
However, Xen is assuming this is 32-bit because earlier revision of
Armv8 had the top 32-bit RES0 (see ARM DDI0595.b).

From Armv8.5, some bits in [63:32] will be defined and allowed to be
modified by the guest. So we would effectively reset those bits to 0
after each context switch. This means the guest may not function
correctly afterwards.

Rather than resetting to 0 the bits [63:32], preserve them acxcross
context switch.

Note that the corresponding register on Arm32 (i.e SCTLR) is always
32-bit. So we need to use register_t anywhere we deal the SCTLR{,_EL1}.

Outside interface is switched to use 64-bit to allow ABI compatibility
between 32-bit and 64-bit.

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

---
    All the other system registers should be switched to 64-bit. This is
    done separatly as this is the only system register that currently
    not save/restore correctly.

    I would consider to backport it as we would end up to disable
    features behind the back of the guest.
---
 tools/xentrace/xenctx.c       |  4 +++-
 xen/arch/arm/guest_walk.c     |  2 +-
 xen/arch/arm/traps.c          | 10 +++++-----
 xen/include/asm-arm/domain.h  |  3 ++-
 xen/include/asm-arm/p2m.h     |  4 ++--
 xen/include/public/arch-arm.h |  4 ++--
 6 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
index e647179e19..2fa864f867 100644
--- a/tools/xentrace/xenctx.c
+++ b/tools/xentrace/xenctx.c
@@ -598,6 +598,8 @@ static void print_ctx_32(vcpu_guest_context_t *ctx)
 
     printf("r12_fiq: %08"PRIx32"\n", regs->r12_fiq);
     printf("\n");
+    /* SCTLR is always 32-bit */
+    printf("SCTLR: %08"PRIx32"\n", (uint32_t)ctx->sctlr);
 }
 
 #ifdef __aarch64__
@@ -659,6 +661,7 @@ static void print_ctx_64(vcpu_guest_context_t *ctx)
     printf("x28: %016"PRIx64"\t", regs->x28);
     printf("x29: %016"PRIx64"\n", regs->x29);
     printf("\n");
+    printf("SCTLR_EL1: %016"PRIx64"\n", ctx->sctlr);
 }
 #endif /* __aarch64__ */
 
@@ -675,7 +678,6 @@ static void print_ctx(vcpu_guest_context_any_t *ctx_any)
     print_ctx_32(ctx);
 #endif
 
-    printf("SCTLR: %08"PRIx32"\n", ctx->sctlr);
     printf("TTBCR: %016"PRIx64"\n", ctx->ttbcr);
     printf("TTBR0: %016"PRIx64"\n", ctx->ttbr0);
     printf("TTBR1: %016"PRIx64"\n", ctx->ttbr1);
diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index c6d6e23bf5..a1cdd7f4af 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -589,7 +589,7 @@ static bool guest_walk_ld(const struct vcpu *v,
 bool guest_walk_tables(const struct vcpu *v, vaddr_t gva,
                        paddr_t *ipa, unsigned int *perms)
 {
-    uint32_t sctlr = READ_SYSREG(SCTLR_EL1);
+    register_t sctlr = READ_SYSREG(SCTLR_EL1);
     register_t tcr = READ_SYSREG(TCR_EL1);
     unsigned int _perms;
 
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 3103620323..111a2029e6 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -384,7 +384,7 @@ void panic_PAR(uint64_t par)
 
 static void cpsr_switch_mode(struct cpu_user_regs *regs, int mode)
 {
-    uint32_t sctlr = READ_SYSREG32(SCTLR_EL1);
+    register_t sctlr = READ_SYSREG(SCTLR_EL1);
 
     regs->cpsr &= ~(PSR_MODE_MASK|PSR_IT_MASK|PSR_JAZELLE|PSR_BIG_ENDIAN|PSR_THUMB);
 
@@ -400,7 +400,7 @@ static void cpsr_switch_mode(struct cpu_user_regs *regs, int mode)
 
 static vaddr_t exception_handler32(vaddr_t offset)
 {
-    uint32_t sctlr = READ_SYSREG32(SCTLR_EL1);
+    register_t sctlr = READ_SYSREG(SCTLR_EL1);
 
     if ( sctlr & SCTLR_A32_EL1_V )
         return 0xffff0000 + offset;
@@ -719,7 +719,7 @@ crash_system:
 
 struct reg_ctxt {
     /* Guest-side state */
-    uint32_t sctlr_el1;
+    register_t sctlr_el1;
     register_t tcr_el1;
     uint64_t ttbr0_el1, ttbr1_el1;
 #ifdef CONFIG_ARM_32
@@ -822,7 +822,7 @@ static void show_registers_32(const struct cpu_user_regs *regs,
 
     if ( guest_mode )
     {
-        printk("     SCTLR: %08"PRIx32"\n", ctxt->sctlr_el1);
+        printk("     SCTLR: %"PRIregister"\n", ctxt->sctlr_el1);
         printk("       TCR: %08"PRIregister"\n", ctxt->tcr_el1);
         printk("     TTBR0: %016"PRIx64"\n", ctxt->ttbr0_el1);
         printk("     TTBR1: %016"PRIx64"\n", ctxt->ttbr1_el1);
@@ -894,7 +894,7 @@ static void show_registers_64(const struct cpu_user_regs *regs,
         printk("   ESR_EL1: %08"PRIx32"\n", ctxt->esr_el1);
         printk("   FAR_EL1: %016"PRIx64"\n", ctxt->far);
         printk("\n");
-        printk(" SCTLR_EL1: %08"PRIx32"\n", ctxt->sctlr_el1);
+        printk(" SCTLR_EL1: %"PRIregister"\n", ctxt->sctlr_el1);
         printk("   TCR_EL1: %08"PRIregister"\n", ctxt->tcr_el1);
         printk(" TTBR0_EL1: %016"PRIx64"\n", ctxt->ttbr0_el1);
         printk(" TTBR1_EL1: %016"PRIx64"\n", ctxt->ttbr1_el1);
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 2960a53e69..86ebdd2bcf 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -167,7 +167,8 @@ struct arch_vcpu
 #endif
 
     /* Control Registers */
-    uint32_t actlr, sctlr;
+    register_t sctlr;
+    uint32_t actlr;
     uint32_t cpacr;
 
     uint32_t contextidr;
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 2f89bb00c3..03f2ee75c1 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -391,12 +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;
+    const register_t mask = SCTLR_Axx_ELx_C | SCTLR_Axx_ELx_M;
 
     /* Only works with the current vCPU */
     ASSERT(current == v);
 
-    return (READ_SYSREG32(SCTLR_EL1) & mask) == mask;
+    return (READ_SYSREG(SCTLR_EL1) & mask) == mask;
 }
 
 #endif /* _XEN_P2M_H */
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 7ce139a0f5..d9a06efbd8 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -291,7 +291,7 @@ struct vcpu_guest_context {
 
     struct vcpu_guest_core_regs user_regs;  /* Core CPU registers */
 
-    uint32_t sctlr;
+    uint64_t sctlr;
     uint64_t ttbcr, ttbr0, ttbr1;
 };
 typedef struct vcpu_guest_context vcpu_guest_context_t;
@@ -380,7 +380,7 @@ typedef uint64_t xen_callback_t;
 #define PSR_GUEST32_INIT  (PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_SVC)
 #define PSR_GUEST64_INIT (PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_EL1h)
 
-#define SCTLR_GUEST_INIT    0x00c50078
+#define SCTLR_GUEST_INIT    xen_mk_ullong(0x00c50078)
 
 /*
  * Virtual machine platform (memory layout, interrupts)
-- 
2.11.0


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

[Xen-devel] [PATCH 3/7] xen/arm: Rework psr_mode_is_32bit()

[Julien Grall]

psr_mode_is_32bit() prototype does not match the rest of the helpers for
the process state. Looking at the callers, most of them will access
struct cpu_user_regs just for calling psr_mode_is_32bit().

The macro is now reworked to take a struct cpu_user_regs in parameter.
At the same time take the opportunity to switch to a static inline
helper.

Lastly, when compiled for 32-bit, Xen will only support 32-bit guest. So
it is pointless to check whether the register state correspond to 64-bit
or not.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/traps.c       | 28 ++++++++++++++--------------
 xen/include/asm-arm/regs.h |  9 ++++++++-
 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 111a2029e6..54e66a86d0 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -919,7 +919,7 @@ static void _show_registers(const struct cpu_user_regs *regs,
 #ifdef CONFIG_ARM_64
         else if ( is_64bit_domain(v->domain) )
         {
-            if ( psr_mode_is_32bit(regs->cpsr) )
+            if ( psr_mode_is_32bit(regs) )
             {
                 BUG_ON(!usr_mode(regs));
                 show_registers_32(regs, ctxt, guest_mode, v);
@@ -1625,7 +1625,7 @@ int check_conditional_instr(struct cpu_user_regs *regs, const union hsr hsr)
     {
         unsigned long it;
 
-        BUG_ON( !psr_mode_is_32bit(regs->cpsr) || !(cpsr&PSR_THUMB) );
+        BUG_ON( !psr_mode_is_32bit(regs) || !(cpsr & PSR_THUMB) );
 
         it = ( (cpsr >> (10-2)) & 0xfc) | ((cpsr >> 25) & 0x3 );
 
@@ -1650,7 +1650,7 @@ int check_conditional_instr(struct cpu_user_regs *regs, const union hsr hsr)
 void advance_pc(struct cpu_user_regs *regs, const union hsr hsr)
 {
     unsigned long itbits, cond, cpsr = regs->cpsr;
-    bool is_thumb = psr_mode_is_32bit(cpsr) && (cpsr & PSR_THUMB);
+    bool is_thumb = psr_mode_is_32bit(regs) && (cpsr & PSR_THUMB);
 
     if ( is_thumb && (cpsr & PSR_IT_MASK) )
     {
@@ -2078,32 +2078,32 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
         advance_pc(regs, hsr);
         break;
     case HSR_EC_CP15_32:
-        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
+        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
         perfc_incr(trap_cp15_32);
         do_cp15_32(regs, hsr);
         break;
     case HSR_EC_CP15_64:
-        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
+        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
         perfc_incr(trap_cp15_64);
         do_cp15_64(regs, hsr);
         break;
     case HSR_EC_CP14_32:
-        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
+        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
         perfc_incr(trap_cp14_32);
         do_cp14_32(regs, hsr);
         break;
     case HSR_EC_CP14_64:
-        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
+        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
         perfc_incr(trap_cp14_64);
         do_cp14_64(regs, hsr);
         break;
     case HSR_EC_CP14_DBG:
-        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
+        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
         perfc_incr(trap_cp14_dbg);
         do_cp14_dbg(regs, hsr);
         break;
     case HSR_EC_CP:
-        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
+        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
         perfc_incr(trap_cp);
         do_cp(regs, hsr);
         break;
@@ -2114,7 +2114,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
          * ARMv7 (DDI 0406C.b): B1.14.8
          * ARMv8 (DDI 0487A.d): D1-1501 Table D1-44
          */
-        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
+        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
         perfc_incr(trap_smc32);
         do_trap_smc(regs, hsr);
         break;
@@ -2122,7 +2122,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
     {
         register_t nr;
 
-        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
+        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
         perfc_incr(trap_hvc32);
 #ifndef NDEBUG
         if ( (hsr.iss & 0xff00) == 0xff00 )
@@ -2137,7 +2137,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
     }
 #ifdef CONFIG_ARM_64
     case HSR_EC_HVC64:
-        GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
+        GUEST_BUG_ON(psr_mode_is_32bit(regs));
         perfc_incr(trap_hvc64);
 #ifndef NDEBUG
         if ( (hsr.iss & 0xff00) == 0xff00 )
@@ -2153,12 +2153,12 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
          *
          * ARMv8 (DDI 0487A.d): D1-1501 Table D1-44
          */
-        GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
+        GUEST_BUG_ON(psr_mode_is_32bit(regs));
         perfc_incr(trap_smc64);
         do_trap_smc(regs, hsr);
         break;
     case HSR_EC_SYSREG:
-        GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
+        GUEST_BUG_ON(psr_mode_is_32bit(regs));
         perfc_incr(trap_sysreg);
         do_sysreg(regs, hsr);
         break;
diff --git a/xen/include/asm-arm/regs.h b/xen/include/asm-arm/regs.h
index ddc6eba9ce..0e3e56b452 100644
--- a/xen/include/asm-arm/regs.h
+++ b/xen/include/asm-arm/regs.h
@@ -13,7 +13,14 @@
 
 #define psr_mode(psr,m) (((psr) & PSR_MODE_MASK) == m)
 
-#define psr_mode_is_32bit(psr) !!((psr) & PSR_MODE_BIT)
+static inline bool psr_mode_is_32bit(const struct cpu_user_regs *regs)
+{
+#ifdef CONFIG_ARM_32
+    return true;
+#else
+    return !!(regs->cpsr & PSR_MODE_BIT);
+#endif
+}
 
 #define usr_mode(r)     psr_mode((r)->cpsr,PSR_MODE_USR)
 #define fiq_mode(r)     psr_mode((r)->cpsr,PSR_MODE_FIQ)
-- 
2.11.0


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

[Xen-devel] [PATCH 4/7] xen/arm: traps: Avoid using BUG_ON() in _show_registers()

[Julien Grall]

At the moment, _show_registers() is using a BUG_ON() to assert only
userspace will run 32-bit code in a 64-bit domain.

Such extra precaution is not necessary and could be avoided by only
checking the CPU mode to decide whether show_registers_64() or
show_reigsters_32() should be called.

This has also the nice advantage to avoid nested if in the code.

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 54e66a86d0..132686ee0f 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -914,21 +914,11 @@ static void _show_registers(const struct cpu_user_regs *regs,
 
     if ( guest_mode )
     {
-        if ( is_32bit_domain(v->domain) )
+        if ( psr_mode_is_32bit(regs) )
             show_registers_32(regs, ctxt, guest_mode, v);
 #ifdef CONFIG_ARM_64
-        else if ( is_64bit_domain(v->domain) )
-        {
-            if ( psr_mode_is_32bit(regs) )
-            {
-                BUG_ON(!usr_mode(regs));
-                show_registers_32(regs, ctxt, guest_mode, v);
-            }
-            else
-            {
-                show_registers_64(regs, ctxt, guest_mode, v);
-            }
-        }
+        else
+            show_registers_64(regs, ctxt, guest_mode, v);
 #endif
     }
     else
-- 
2.11.0


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

[Xen-devel] [PATCH 5/7] xen/arm: traps: Avoid BUG_ON() in do_trap_brk()

[Julien Grall]

At the moment, do_trap_brk() is using a BUG_ON() to check the hardware
has been correctly configured during boot.

Any error when configuring the hardware could result to a guest 'brk'
trapping in the hypervisor and crash it.

This is pretty harsh to kill Xen when actually killing the guest would
be enough as misconfiguring this trap would not lead to exposing
sensitive data. Replace the BUG_ON() with crashing the guest.

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 132686ee0f..ef37ca6bde 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1304,10 +1304,15 @@ int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
 #ifdef CONFIG_ARM_64
 static void do_trap_brk(struct cpu_user_regs *regs, const union hsr hsr)
 {
-    /* HCR_EL2.TGE and MDCR_EL2.TDE are not set so we never receive
-     * software breakpoint exception for EL1 and EL0 here.
+    /*
+     * HCR_EL2.TGE and MDCR_EL2.TDR are currently not set. So we should
+     * never receive software breakpoing exception for EL1 and EL0 here.
      */
-    BUG_ON(!hyp_mode(regs));
+    if ( !hyp_mode(regs) )
+    {
+        domain_crash(current->domain);
+        return;
+    }
 
     switch ( hsr.brk.comment )
     {
-- 
2.11.0


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

[Xen-devel] [PATCH 6/7] xen/arm: vsmc: The function identifier is always 32-bit

[Julien Grall]

On Arm64, the SMCCC function identifier is always stored in the first 32-bit
of x0 register. The rest of the bits are not defined and should be
ignored.

This means the variable funcid should be an uint32_t rather than
register_t.

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

diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index f8e350311d..a36db15fff 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -220,7 +220,7 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
 {
     bool handled = false;
     const union hsr hsr = { .bits = regs->hsr };
-    register_t funcid = get_user_reg(regs, 0);
+    uint32_t funcid = get_user_reg(regs, 0);
 
     /*
      * Check immediate value for HVC32, HVC64 and SMC64.
@@ -286,7 +286,7 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
 
     if ( !handled )
     {
-        gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n", funcid);
+        gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %#x\n", funcid);
 
         /* Inform caller that function is not supported. */
         set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
-- 
2.11.0


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

[Xen-devel] [PATCH 7/7] xen/arm: types: Specify the zero padding in the definition of PRIregister

[Julien Grall]

The definition of PRIregister varies between Arm32 and Arm64 (32-bit vs
64-bit). However, some of the users uses the wrong padding.

For more consistency, the padding is now moved into the PRIregister and
varies depending on the architecture.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/traps.c        | 10 +++++-----
 xen/include/asm-arm/types.h |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index ef37ca6bde..f062ae6f6a 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -797,7 +797,7 @@ static void show_registers_32(const struct cpu_user_regs *regs,
 
     if ( guest_mode )
     {
-        printk("USR: SP: %08"PRIx32" LR: %08"PRIregister"\n",
+        printk("USR: SP: %08"PRIx32" LR: %"PRIregister"\n",
                regs->sp_usr, regs->lr);
         printk("SVC: SP: %08"PRIx32" LR: %08"PRIx32" SPSR:%08"PRIx32"\n",
                regs->sp_svc, regs->lr_svc, regs->spsr_svc);
@@ -815,7 +815,7 @@ static void show_registers_32(const struct cpu_user_regs *regs,
 #ifndef CONFIG_ARM_64
     else
     {
-        printk("HYP: SP: %08"PRIx32" LR: %08"PRIregister"\n", regs->sp, regs->lr);
+        printk("HYP: SP: %08"PRIx32" LR: %"PRIregister"\n", regs->sp, regs->lr);
     }
 #endif
     printk("\n");
@@ -823,7 +823,7 @@ static void show_registers_32(const struct cpu_user_regs *regs,
     if ( guest_mode )
     {
         printk("     SCTLR: %"PRIregister"\n", ctxt->sctlr_el1);
-        printk("       TCR: %08"PRIregister"\n", ctxt->tcr_el1);
+        printk("       TCR: %"PRIregister"\n", ctxt->tcr_el1);
         printk("     TTBR0: %016"PRIx64"\n", ctxt->ttbr0_el1);
         printk("     TTBR1: %016"PRIx64"\n", ctxt->ttbr1_el1);
         printk("      IFAR: %08"PRIx32", IFSR: %08"PRIx32"\n"
@@ -895,7 +895,7 @@ static void show_registers_64(const struct cpu_user_regs *regs,
         printk("   FAR_EL1: %016"PRIx64"\n", ctxt->far);
         printk("\n");
         printk(" SCTLR_EL1: %"PRIregister"\n", ctxt->sctlr_el1);
-        printk("   TCR_EL1: %08"PRIregister"\n", ctxt->tcr_el1);
+        printk("   TCR_EL1: %"PRIregister"\n", ctxt->tcr_el1);
         printk(" TTBR0_EL1: %016"PRIx64"\n", ctxt->ttbr0_el1);
         printk(" TTBR1_EL1: %016"PRIx64"\n", ctxt->ttbr1_el1);
         printk("\n");
@@ -934,7 +934,7 @@ static void _show_registers(const struct cpu_user_regs *regs,
     printk("\n");
 
     printk(" SCTLR_EL2: %08"PRIx32"\n", READ_SYSREG32(SCTLR_EL2));
-    printk("   HCR_EL2: %016"PRIregister"\n", READ_SYSREG(HCR_EL2));
+    printk("   HCR_EL2: %"PRIregister"\n", READ_SYSREG(HCR_EL2));
     printk(" TTBR0_EL2: %016"PRIx64"\n", READ_SYSREG64(TTBR0_EL2));
     printk("\n");
     printk("   ESR_EL2: %08"PRIx32"\n", regs->hsr);
diff --git a/xen/include/asm-arm/types.h b/xen/include/asm-arm/types.h
index 30f95078cb..89aae25ffe 100644
--- a/xen/include/asm-arm/types.h
+++ b/xen/include/asm-arm/types.h
@@ -41,7 +41,7 @@ typedef u64 paddr_t;
 #define INVALID_PADDR (~0ULL)
 #define PRIpaddr "016llx"
 typedef u32 register_t;
-#define PRIregister "x"
+#define PRIregister "08x"
 #elif defined (CONFIG_ARM_64)
 typedef signed long s64;
 typedef unsigned long u64;
@@ -51,7 +51,7 @@ typedef u64 paddr_t;
 #define INVALID_PADDR (~0UL)
 #define PRIpaddr "016lx"
 typedef u64 register_t;
-#define PRIregister "lx"
+#define PRIregister "016lx"
 #endif
 
 #if defined(__SIZE_TYPE__)
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 1/7] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Volodymyr Babchuk]

Hi Julien,

Julien Grall writes:

> Currently, the structure vcpu_guest_core_regs is part of the public API.
> This implies that any change in the structure should be backward
> compatible.
>
> However, the structure is only needed by the tools and Xen. It is also
> not expected to be ever used outside of that context. So we could save us
> some headache by only declaring the structure for Xen and tools.
>
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>     This is a follow-up of the discussion [1].
>
>     [1] <3c245c5b-51c6-1d0e-ad6c-42414573166f@arm.com>
>
>     Changes in v3:
>         - Avoid introduce a new #ifdef in the header by moving the
>         definitions later on.
> ---
>  xen/include/public/arch-arm.h | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 3e8cdc151d..7ce139a0f5 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -197,6 +197,18 @@
>      } while ( 0 )
>  #define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
>  
> +typedef uint64_t xen_pfn_t;
> +#define PRI_xen_pfn PRIx64
> +#define PRIu_xen_pfn PRIu64
> +
> +/* Maximum number of virtual CPUs in legacy multi-processor guests. */
> +/* Only one. All other VCPUS must use VCPUOP_register_vcpu_info */
Just a suggestion: you already touching this part. Maybe you'll fix this
comment as well?

> +#define XEN_LEGACY_MAX_VCPUS 1
> +
> +typedef uint64_t xen_ulong_t;
> +#define PRI_xen_ulong PRIx64
> +
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
>  #if defined(__GNUC__) && !defined(__STRICT_ANSI__)
>  /* Anonymous union includes both 32- and 64-bit names (e.g., r0/x0). */
>  # define __DECL_REG(n64, n32) union {          \
> @@ -272,18 +284,6 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_core_regs_t);
>  
>  #undef __DECL_REG
>  
> -typedef uint64_t xen_pfn_t;
> -#define PRI_xen_pfn PRIx64
> -#define PRIu_xen_pfn PRIu64
> -
> -/* Maximum number of virtual CPUs in legacy multi-processor guests. */
> -/* Only one. All other VCPUS must use VCPUOP_register_vcpu_info */
> -#define XEN_LEGACY_MAX_VCPUS 1
> -
> -typedef uint64_t xen_ulong_t;
> -#define PRI_xen_ulong PRIx64
> -
> -#if defined(__XEN__) || defined(__XEN_TOOLS__)
>  struct vcpu_guest_context {
>  #define _VGCF_online                   0
>  #define VGCF_online                    (1<<_VGCF_online)


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

Re: [Xen-devel] [PATCH 2/7] xen/arm: SCTLR_EL1 is a 64-bit register on Arm64

[Volodymyr Babchuk]

Hi,

Julien Grall writes:

> On Arm64, system registers are always 64-bit including SCTLR_EL1.
> However, Xen is assuming this is 32-bit because earlier revision of
> Armv8 had the top 32-bit RES0 (see ARM DDI0595.b).
>
> From Armv8.5, some bits in [63:32] will be defined and allowed to be
> modified by the guest. So we would effectively reset those bits to 0
> after each context switch. This means the guest may not function
> correctly afterwards.
>
> Rather than resetting to 0 the bits [63:32], preserve them acxcross
typo: across
> context switch.
>
> Note that the corresponding register on Arm32 (i.e SCTLR) is always
> 32-bit. So we need to use register_t anywhere we deal the SCTLR{,_EL1}.
>
> Outside interface is switched to use 64-bit to allow ABI compatibility
> between 32-bit and 64-bit.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Providing that typo will be fixed:
Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com>
>
> ---
>     All the other system registers should be switched to 64-bit. This is
>     done separatly as this is the only system register that currently
>     not save/restore correctly.
>
>     I would consider to backport it as we would end up to disable
>     features behind the back of the guest.
> ---
>  tools/xentrace/xenctx.c       |  4 +++-
>  xen/arch/arm/guest_walk.c     |  2 +-
>  xen/arch/arm/traps.c          | 10 +++++-----
>  xen/include/asm-arm/domain.h  |  3 ++-
>  xen/include/asm-arm/p2m.h     |  4 ++--
>  xen/include/public/arch-arm.h |  4 ++--
>  6 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
> index e647179e19..2fa864f867 100644
> --- a/tools/xentrace/xenctx.c
> +++ b/tools/xentrace/xenctx.c
> @@ -598,6 +598,8 @@ static void print_ctx_32(vcpu_guest_context_t *ctx)
>  
>      printf("r12_fiq: %08"PRIx32"\n", regs->r12_fiq);
>      printf("\n");
> +    /* SCTLR is always 32-bit */
> +    printf("SCTLR: %08"PRIx32"\n", (uint32_t)ctx->sctlr);
>  }
>  
>  #ifdef __aarch64__
> @@ -659,6 +661,7 @@ static void print_ctx_64(vcpu_guest_context_t *ctx)
>      printf("x28: %016"PRIx64"\t", regs->x28);
>      printf("x29: %016"PRIx64"\n", regs->x29);
>      printf("\n");
> +    printf("SCTLR_EL1: %016"PRIx64"\n", ctx->sctlr);
>  }
>  #endif /* __aarch64__ */
>  
> @@ -675,7 +678,6 @@ static void print_ctx(vcpu_guest_context_any_t *ctx_any)
>      print_ctx_32(ctx);
>  #endif
>  
> -    printf("SCTLR: %08"PRIx32"\n", ctx->sctlr);
>      printf("TTBCR: %016"PRIx64"\n", ctx->ttbcr);
>      printf("TTBR0: %016"PRIx64"\n", ctx->ttbr0);
>      printf("TTBR1: %016"PRIx64"\n", ctx->ttbr1);
> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> index c6d6e23bf5..a1cdd7f4af 100644
> --- a/xen/arch/arm/guest_walk.c
> +++ b/xen/arch/arm/guest_walk.c
> @@ -589,7 +589,7 @@ static bool guest_walk_ld(const struct vcpu *v,
>  bool guest_walk_tables(const struct vcpu *v, vaddr_t gva,
>                         paddr_t *ipa, unsigned int *perms)
>  {
> -    uint32_t sctlr = READ_SYSREG(SCTLR_EL1);
> +    register_t sctlr = READ_SYSREG(SCTLR_EL1);
>      register_t tcr = READ_SYSREG(TCR_EL1);
>      unsigned int _perms;
>  
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 3103620323..111a2029e6 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -384,7 +384,7 @@ void panic_PAR(uint64_t par)
>  
>  static void cpsr_switch_mode(struct cpu_user_regs *regs, int mode)
>  {
> -    uint32_t sctlr = READ_SYSREG32(SCTLR_EL1);
> +    register_t sctlr = READ_SYSREG(SCTLR_EL1);
>  
>      regs->cpsr &= ~(PSR_MODE_MASK|PSR_IT_MASK|PSR_JAZELLE|PSR_BIG_ENDIAN|PSR_THUMB);
>  
> @@ -400,7 +400,7 @@ static void cpsr_switch_mode(struct cpu_user_regs *regs, int mode)
>  
>  static vaddr_t exception_handler32(vaddr_t offset)
>  {
> -    uint32_t sctlr = READ_SYSREG32(SCTLR_EL1);
> +    register_t sctlr = READ_SYSREG(SCTLR_EL1);
>  
>      if ( sctlr & SCTLR_A32_EL1_V )
>          return 0xffff0000 + offset;
> @@ -719,7 +719,7 @@ crash_system:
>  
>  struct reg_ctxt {
>      /* Guest-side state */
> -    uint32_t sctlr_el1;
> +    register_t sctlr_el1;
>      register_t tcr_el1;
>      uint64_t ttbr0_el1, ttbr1_el1;
>  #ifdef CONFIG_ARM_32
> @@ -822,7 +822,7 @@ static void show_registers_32(const struct cpu_user_regs *regs,
>  
>      if ( guest_mode )
>      {
> -        printk("     SCTLR: %08"PRIx32"\n", ctxt->sctlr_el1);
> +        printk("     SCTLR: %"PRIregister"\n", ctxt->sctlr_el1);
>          printk("       TCR: %08"PRIregister"\n", ctxt->tcr_el1);
>          printk("     TTBR0: %016"PRIx64"\n", ctxt->ttbr0_el1);
>          printk("     TTBR1: %016"PRIx64"\n", ctxt->ttbr1_el1);
> @@ -894,7 +894,7 @@ static void show_registers_64(const struct cpu_user_regs *regs,
>          printk("   ESR_EL1: %08"PRIx32"\n", ctxt->esr_el1);
>          printk("   FAR_EL1: %016"PRIx64"\n", ctxt->far);
>          printk("\n");
> -        printk(" SCTLR_EL1: %08"PRIx32"\n", ctxt->sctlr_el1);
> +        printk(" SCTLR_EL1: %"PRIregister"\n", ctxt->sctlr_el1);
>          printk("   TCR_EL1: %08"PRIregister"\n", ctxt->tcr_el1);
>          printk(" TTBR0_EL1: %016"PRIx64"\n", ctxt->ttbr0_el1);
>          printk(" TTBR1_EL1: %016"PRIx64"\n", ctxt->ttbr1_el1);
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 2960a53e69..86ebdd2bcf 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -167,7 +167,8 @@ struct arch_vcpu
>  #endif
>  
>      /* Control Registers */
> -    uint32_t actlr, sctlr;
> +    register_t sctlr;
> +    uint32_t actlr;
>      uint32_t cpacr;
>  
>      uint32_t contextidr;
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 2f89bb00c3..03f2ee75c1 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -391,12 +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;
> +    const register_t mask = SCTLR_Axx_ELx_C | SCTLR_Axx_ELx_M;
>  
>      /* Only works with the current vCPU */
>      ASSERT(current == v);
>  
> -    return (READ_SYSREG32(SCTLR_EL1) & mask) == mask;
> +    return (READ_SYSREG(SCTLR_EL1) & mask) == mask;
>  }
>  
>  #endif /* _XEN_P2M_H */
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 7ce139a0f5..d9a06efbd8 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -291,7 +291,7 @@ struct vcpu_guest_context {
>  
>      struct vcpu_guest_core_regs user_regs;  /* Core CPU registers */
>  
> -    uint32_t sctlr;
> +    uint64_t sctlr;
>      uint64_t ttbcr, ttbr0, ttbr1;
>  };
>  typedef struct vcpu_guest_context vcpu_guest_context_t;
> @@ -380,7 +380,7 @@ typedef uint64_t xen_callback_t;
>  #define PSR_GUEST32_INIT  (PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_SVC)
>  #define PSR_GUEST64_INIT (PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_EL1h)
>  
> -#define SCTLR_GUEST_INIT    0x00c50078
> +#define SCTLR_GUEST_INIT    xen_mk_ullong(0x00c50078)
>  
>  /*
>   * Virtual machine platform (memory layout, interrupts)


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

Re: [Xen-devel] [PATCH 3/7] xen/arm: Rework psr_mode_is_32bit()

[Volodymyr Babchuk]

Julien Grall writes:

> psr_mode_is_32bit() prototype does not match the rest of the helpers for
> the process state. Looking at the callers, most of them will access
> struct cpu_user_regs just for calling psr_mode_is_32bit().
>
> The macro is now reworked to take a struct cpu_user_regs in parameter.
> At the same time take the opportunity to switch to a static inline
> helper.
I'm a bit concerned about naming now. As psr_mode_is_32bit() is now have
no psr parameter, and ARM ARM uses term "state" instead of "mode", maybe
it is worth to rename this helper to something like "is_32bit_state"?

> Lastly, when compiled for 32-bit, Xen will only support 32-bit guest. So
> it is pointless to check whether the register state correspond to 64-bit
> or not.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/traps.c       | 28 ++++++++++++++--------------
>  xen/include/asm-arm/regs.h |  9 ++++++++-
>  2 files changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 111a2029e6..54e66a86d0 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -919,7 +919,7 @@ static void _show_registers(const struct cpu_user_regs *regs,
>  #ifdef CONFIG_ARM_64
>          else if ( is_64bit_domain(v->domain) )
>          {
> -            if ( psr_mode_is_32bit(regs->cpsr) )
> +            if ( psr_mode_is_32bit(regs) )
>              {
>                  BUG_ON(!usr_mode(regs));
>                  show_registers_32(regs, ctxt, guest_mode, v);
> @@ -1625,7 +1625,7 @@ int check_conditional_instr(struct cpu_user_regs *regs, const union hsr hsr)
>      {
>          unsigned long it;
>  
> -        BUG_ON( !psr_mode_is_32bit(regs->cpsr) || !(cpsr&PSR_THUMB) );
> +        BUG_ON( !psr_mode_is_32bit(regs) || !(cpsr & PSR_THUMB) );
>  
>          it = ( (cpsr >> (10-2)) & 0xfc) | ((cpsr >> 25) & 0x3 );
>  
> @@ -1650,7 +1650,7 @@ int check_conditional_instr(struct cpu_user_regs *regs, const union hsr hsr)
>  void advance_pc(struct cpu_user_regs *regs, const union hsr hsr)
>  {
>      unsigned long itbits, cond, cpsr = regs->cpsr;
> -    bool is_thumb = psr_mode_is_32bit(cpsr) && (cpsr & PSR_THUMB);
> +    bool is_thumb = psr_mode_is_32bit(regs) && (cpsr & PSR_THUMB);
>  
>      if ( is_thumb && (cpsr & PSR_IT_MASK) )
>      {
> @@ -2078,32 +2078,32 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>          advance_pc(regs, hsr);
>          break;
>      case HSR_EC_CP15_32:
> -        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> +        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
>          perfc_incr(trap_cp15_32);
>          do_cp15_32(regs, hsr);
>          break;
>      case HSR_EC_CP15_64:
> -        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> +        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
>          perfc_incr(trap_cp15_64);
>          do_cp15_64(regs, hsr);
>          break;
>      case HSR_EC_CP14_32:
> -        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> +        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
>          perfc_incr(trap_cp14_32);
>          do_cp14_32(regs, hsr);
>          break;
>      case HSR_EC_CP14_64:
> -        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> +        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
>          perfc_incr(trap_cp14_64);
>          do_cp14_64(regs, hsr);
>          break;
>      case HSR_EC_CP14_DBG:
> -        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> +        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
>          perfc_incr(trap_cp14_dbg);
>          do_cp14_dbg(regs, hsr);
>          break;
>      case HSR_EC_CP:
> -        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> +        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
>          perfc_incr(trap_cp);
>          do_cp(regs, hsr);
>          break;
> @@ -2114,7 +2114,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>           * ARMv7 (DDI 0406C.b): B1.14.8
>           * ARMv8 (DDI 0487A.d): D1-1501 Table D1-44
>           */
> -        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> +        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
>          perfc_incr(trap_smc32);
>          do_trap_smc(regs, hsr);
>          break;
> @@ -2122,7 +2122,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>      {
>          register_t nr;
>  
> -        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> +        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
>          perfc_incr(trap_hvc32);
>  #ifndef NDEBUG
>          if ( (hsr.iss & 0xff00) == 0xff00 )
> @@ -2137,7 +2137,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>      }
>  #ifdef CONFIG_ARM_64
>      case HSR_EC_HVC64:
> -        GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
> +        GUEST_BUG_ON(psr_mode_is_32bit(regs));
>          perfc_incr(trap_hvc64);
>  #ifndef NDEBUG
>          if ( (hsr.iss & 0xff00) == 0xff00 )
> @@ -2153,12 +2153,12 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>           *
>           * ARMv8 (DDI 0487A.d): D1-1501 Table D1-44
>           */
> -        GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
> +        GUEST_BUG_ON(psr_mode_is_32bit(regs));
>          perfc_incr(trap_smc64);
>          do_trap_smc(regs, hsr);
>          break;
>      case HSR_EC_SYSREG:
> -        GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
> +        GUEST_BUG_ON(psr_mode_is_32bit(regs));
>          perfc_incr(trap_sysreg);
>          do_sysreg(regs, hsr);
>          break;
> diff --git a/xen/include/asm-arm/regs.h b/xen/include/asm-arm/regs.h
> index ddc6eba9ce..0e3e56b452 100644
> --- a/xen/include/asm-arm/regs.h
> +++ b/xen/include/asm-arm/regs.h
> @@ -13,7 +13,14 @@
>  
>  #define psr_mode(psr,m) (((psr) & PSR_MODE_MASK) == m)
>  
> -#define psr_mode_is_32bit(psr) !!((psr) & PSR_MODE_BIT)
> +static inline bool psr_mode_is_32bit(const struct cpu_user_regs *regs)
> +{
> +#ifdef CONFIG_ARM_32
> +    return true;
> +#else
> +    return !!(regs->cpsr & PSR_MODE_BIT);
> +#endif
> +}
>  
>  #define usr_mode(r)     psr_mode((r)->cpsr,PSR_MODE_USR)
>  #define fiq_mode(r)     psr_mode((r)->cpsr,PSR_MODE_FIQ)


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

Re: [Xen-devel] [PATCH 4/7] xen/arm: traps: Avoid using BUG_ON() in _show_registers()

[Volodymyr Babchuk]

Julien Grall writes:

> At the moment, _show_registers() is using a BUG_ON() to assert only
> userspace will run 32-bit code in a 64-bit domain.
>
> Such extra precaution is not necessary and could be avoided by only
> checking the CPU mode to decide whether show_registers_64() or
> show_reigsters_32() should be called.
>
> This has also the nice advantage to avoid nested if in the code.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

> ---
>  xen/arch/arm/traps.c | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 54e66a86d0..132686ee0f 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -914,21 +914,11 @@ static void _show_registers(const struct cpu_user_regs *regs,
>  
>      if ( guest_mode )
>      {
> -        if ( is_32bit_domain(v->domain) )
> +        if ( psr_mode_is_32bit(regs) )
>              show_registers_32(regs, ctxt, guest_mode, v);
>  #ifdef CONFIG_ARM_64
> -        else if ( is_64bit_domain(v->domain) )
> -        {
> -            if ( psr_mode_is_32bit(regs) )
> -            {
> -                BUG_ON(!usr_mode(regs));
> -                show_registers_32(regs, ctxt, guest_mode, v);
> -            }
> -            else
> -            {
> -                show_registers_64(regs, ctxt, guest_mode, v);
> -            }
> -        }
> +        else
> +            show_registers_64(regs, ctxt, guest_mode, v);
>  #endif
>      }
>      else


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

Re: [Xen-devel] [PATCH 5/7] xen/arm: traps: Avoid BUG_ON() in do_trap_brk()

[Volodymyr Babchuk]

Julien Grall writes:

> At the moment, do_trap_brk() is using a BUG_ON() to check the hardware
> has been correctly configured during boot.
>
> Any error when configuring the hardware could result to a guest 'brk'
> trapping in the hypervisor and crash it.
>
> This is pretty harsh to kill Xen when actually killing the guest would
> be enough as misconfiguring this trap would not lead to exposing
> sensitive data. Replace the BUG_ON() with crashing the guest.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

> ---
>  xen/arch/arm/traps.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 132686ee0f..ef37ca6bde 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1304,10 +1304,15 @@ int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
>  #ifdef CONFIG_ARM_64
>  static void do_trap_brk(struct cpu_user_regs *regs, const union hsr hsr)
>  {
> -    /* HCR_EL2.TGE and MDCR_EL2.TDE are not set so we never receive
> -     * software breakpoint exception for EL1 and EL0 here.
> +    /*
> +     * HCR_EL2.TGE and MDCR_EL2.TDR are currently not set. So we should
> +     * never receive software breakpoing exception for EL1 and EL0 here.
>       */
> -    BUG_ON(!hyp_mode(regs));
> +    if ( !hyp_mode(regs) )
> +    {
> +        domain_crash(current->domain);
> +        return;
> +    }
>  
>      switch ( hsr.brk.comment )
>      {


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

Re: [Xen-devel] [PATCH 6/7] xen/arm: vsmc: The function identifier is always 32-bit

[Volodymyr Babchuk]

Julien Grall writes:

> On Arm64, the SMCCC function identifier is always stored in the first 32-bit
> of x0 register. The rest of the bits are not defined and should be
> ignored.
>
> This means the variable funcid should be an uint32_t rather than
> register_t.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

> ---
>  xen/arch/arm/vsmc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index f8e350311d..a36db15fff 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -220,7 +220,7 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
>  {
>      bool handled = false;
>      const union hsr hsr = { .bits = regs->hsr };
> -    register_t funcid = get_user_reg(regs, 0);
> +    uint32_t funcid = get_user_reg(regs, 0);
>  
>      /*
>       * Check immediate value for HVC32, HVC64 and SMC64.
> @@ -286,7 +286,7 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
>  
>      if ( !handled )
>      {
> -        gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n", funcid);
> +        gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %#x\n", funcid);
>  
>          /* Inform caller that function is not supported. */
>          set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);


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

Re: [Xen-devel] [PATCH 7/7] xen/arm: types: Specify the zero padding in the definition of PRIregister

[Volodymyr Babchuk]

Julien Grall writes:

> The definition of PRIregister varies between Arm32 and Arm64 (32-bit vs
> 64-bit). However, some of the users uses the wrong padding.
type: "users use"

> For more consistency, the padding is now moved into the PRIregister and
> varies depending on the architecture.
I'm not sure this is the right thing to do. There are lots of code
(especially in vgic) that does not use padding at all. Now it will print
padding, even if original author does not wanted to. And, honestly it is
hard to parse 15-16 zeroes in a row.

I am suggesting to add another macro like PRIregister_pad or something
like that.

> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/traps.c        | 10 +++++-----
>  xen/include/asm-arm/types.h |  4 ++--
>  2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index ef37ca6bde..f062ae6f6a 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -797,7 +797,7 @@ static void show_registers_32(const struct cpu_user_regs *regs,
>
>      if ( guest_mode )
>      {
> -        printk("USR: SP: %08"PRIx32" LR: %08"PRIregister"\n",
> +        printk("USR: SP: %08"PRIx32" LR: %"PRIregister"\n",
>                 regs->sp_usr, regs->lr);
>          printk("SVC: SP: %08"PRIx32" LR: %08"PRIx32" SPSR:%08"PRIx32"\n",
>                 regs->sp_svc, regs->lr_svc, regs->spsr_svc);
> @@ -815,7 +815,7 @@ static void show_registers_32(const struct cpu_user_regs *regs,
>  #ifndef CONFIG_ARM_64
>      else
>      {
> -        printk("HYP: SP: %08"PRIx32" LR: %08"PRIregister"\n", regs->sp, regs->lr);
> +        printk("HYP: SP: %08"PRIx32" LR: %"PRIregister"\n", regs->sp, regs->lr);
>      }
>  #endif
>      printk("\n");
> @@ -823,7 +823,7 @@ static void show_registers_32(const struct cpu_user_regs *regs,
>      if ( guest_mode )
>      {
>          printk("     SCTLR: %"PRIregister"\n", ctxt->sctlr_el1);
> -        printk("       TCR: %08"PRIregister"\n", ctxt->tcr_el1);
> +        printk("       TCR: %"PRIregister"\n", ctxt->tcr_el1);
>          printk("     TTBR0: %016"PRIx64"\n", ctxt->ttbr0_el1);
>          printk("     TTBR1: %016"PRIx64"\n", ctxt->ttbr1_el1);
>          printk("      IFAR: %08"PRIx32", IFSR: %08"PRIx32"\n"
> @@ -895,7 +895,7 @@ static void show_registers_64(const struct cpu_user_regs *regs,
>          printk("   FAR_EL1: %016"PRIx64"\n", ctxt->far);
>          printk("\n");
>          printk(" SCTLR_EL1: %"PRIregister"\n", ctxt->sctlr_el1);
> -        printk("   TCR_EL1: %08"PRIregister"\n", ctxt->tcr_el1);
> +        printk("   TCR_EL1: %"PRIregister"\n", ctxt->tcr_el1);
>          printk(" TTBR0_EL1: %016"PRIx64"\n", ctxt->ttbr0_el1);
>          printk(" TTBR1_EL1: %016"PRIx64"\n", ctxt->ttbr1_el1);
>          printk("\n");
> @@ -934,7 +934,7 @@ static void _show_registers(const struct cpu_user_regs *regs,
>      printk("\n");
>
>      printk(" SCTLR_EL2: %08"PRIx32"\n", READ_SYSREG32(SCTLR_EL2));
> -    printk("   HCR_EL2: %016"PRIregister"\n", READ_SYSREG(HCR_EL2));
> +    printk("   HCR_EL2: %"PRIregister"\n", READ_SYSREG(HCR_EL2));
>      printk(" TTBR0_EL2: %016"PRIx64"\n", READ_SYSREG64(TTBR0_EL2));
>      printk("\n");
>      printk("   ESR_EL2: %08"PRIx32"\n", regs->hsr);
> diff --git a/xen/include/asm-arm/types.h b/xen/include/asm-arm/types.h
> index 30f95078cb..89aae25ffe 100644
> --- a/xen/include/asm-arm/types.h
> +++ b/xen/include/asm-arm/types.h
> @@ -41,7 +41,7 @@ typedef u64 paddr_t;
>  #define INVALID_PADDR (~0ULL)
>  #define PRIpaddr "016llx"
>  typedef u32 register_t;
> -#define PRIregister "x"
> +#define PRIregister "08x"
>  #elif defined (CONFIG_ARM_64)
>  typedef signed long s64;
>  typedef unsigned long u64;
> @@ -51,7 +51,7 @@ typedef u64 paddr_t;
>  #define INVALID_PADDR (~0UL)
>  #define PRIpaddr "016lx"
>  typedef u64 register_t;
> -#define PRIregister "lx"
> +#define PRIregister "016lx"
>  #endif
>
>  #if defined(__SIZE_TYPE__)


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

Re: [Xen-devel] [PATCH 1/7] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Julien Grall]

On 26/07/2019 13:14, Volodymyr Babchuk wrote:
> 
> Hi Julien,

Hi Volodymyr,

> Julien Grall writes:
> 
>> Currently, the structure vcpu_guest_core_regs is part of the public API.
>> This implies that any change in the structure should be backward
>> compatible.
>>
>> However, the structure is only needed by the tools and Xen. It is also
>> not expected to be ever used outside of that context. So we could save us
>> some headache by only declaring the structure for Xen and tools.
>>
>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>      This is a follow-up of the discussion [1].
>>
>>      [1] <3c245c5b-51c6-1d0e-ad6c-42414573166f@arm.com>
>>
>>      Changes in v3:
>>          - Avoid introduce a new #ifdef in the header by moving the
>>          definitions later on.
>> ---
>>   xen/include/public/arch-arm.h | 24 ++++++++++++------------
>>   1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>> index 3e8cdc151d..7ce139a0f5 100644
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -197,6 +197,18 @@
>>       } while ( 0 )
>>   #define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
>>   
>> +typedef uint64_t xen_pfn_t;
>> +#define PRI_xen_pfn PRIx64
>> +#define PRIu_xen_pfn PRIu64
>> +
>> +/* Maximum number of virtual CPUs in legacy multi-processor guests. */
>> +/* Only one. All other VCPUS must use VCPUOP_register_vcpu_info */
> Just a suggestion: you already touching this part. Maybe you'll fix this
> comment as well?

I am not sure what's wrong with the current comment. Can you expand your 
thoughts please?

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 3/7] xen/arm: Rework psr_mode_is_32bit()

[Julien Grall]

Hi,

On 26/07/2019 13:31, Volodymyr Babchuk wrote:
> 
> Julien Grall writes:
> 
>> psr_mode_is_32bit() prototype does not match the rest of the helpers for
>> the process state. Looking at the callers, most of them will access
>> struct cpu_user_regs just for calling psr_mode_is_32bit().
>>
>> The macro is now reworked to take a struct cpu_user_regs in parameter.
>> At the same time take the opportunity to switch to a static inline
>> helper.
> I'm a bit concerned about naming now. As psr_mode_is_32bit() is now have
> no psr parameter, and ARM ARM uses term "state" instead of "mode", maybe
> it is worth to rename this helper to something like "is_32bit_state"?

It really depends how you see it. The bit is part of the "mode" field, so 
technically we are checking whether the mode corresponds to a 32-bit one or not. 
This is also inline with the rest of the helpers within this header.

I would be willing to consider renaming the helper to regs_mode_is_32bit().

On a side note, Linux is using exactly the same term (see vcpu_mode_is_32bit).

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 1/7] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Volodymyr Babchuk]

Julien Grall writes:

> On 26/07/2019 13:14, Volodymyr Babchuk wrote:
>>
>> Hi Julien,
>
> Hi Volodymyr,
>
>> Julien Grall writes:
>>
>>> Currently, the structure vcpu_guest_core_regs is part of the public API.
>>> This implies that any change in the structure should be backward
>>> compatible.
>>>
>>> However, the structure is only needed by the tools and Xen. It is also
>>> not expected to be ever used outside of that context. So we could save us
>>> some headache by only declaring the structure for Xen and tools.
>>>
>>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>> ---
>>>      This is a follow-up of the discussion [1].
>>>
>>>      [1] <3c245c5b-51c6-1d0e-ad6c-42414573166f@arm.com>
>>>
>>>      Changes in v3:
>>>          - Avoid introduce a new #ifdef in the header by moving the
>>>          definitions later on.
>>> ---
>>>   xen/include/public/arch-arm.h | 24 ++++++++++++------------
>>>   1 file changed, 12 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>>> index 3e8cdc151d..7ce139a0f5 100644
>>> --- a/xen/include/public/arch-arm.h
>>> +++ b/xen/include/public/arch-arm.h
>>> @@ -197,6 +197,18 @@
>>>       } while ( 0 )
>>>   #define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
>>>   +typedef uint64_t xen_pfn_t;
>>> +#define PRI_xen_pfn PRIx64
>>> +#define PRIu_xen_pfn PRIu64
>>> +
>>> +/* Maximum number of virtual CPUs in legacy multi-processor guests. */
>>> +/* Only one. All other VCPUS must use VCPUOP_register_vcpu_info */
>> Just a suggestion: you already touching this part. Maybe you'll fix this
>> comment as well?
>
> I am not sure what's wrong with the current comment. Can you expand
> your thoughts please?
Sure. It does not conform to CODING_STYLE:

   Comments containing a single sentence may end with a full
   stop; comments containing several sentences must have a full stop
   after each sentence.

The second comment misses full stop at the end. Also, maybe we should
consider this as s multi-line comment:

   Multi-line comment blocks should start and end with comment markers on
   separate lines and each line should begin with a leading '*'.


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

Re: [Xen-devel] [PATCH 7/7] xen/arm: types: Specify the zero padding in the definition of PRIregister

[Julien Grall]

Hi,

On 26/07/2019 13:47, Volodymyr Babchuk wrote:
> 
> Julien Grall writes:
> 
>> The definition of PRIregister varies between Arm32 and Arm64 (32-bit vs
>> 64-bit). However, some of the users uses the wrong padding.
> type: "users use"
> 
>> For more consistency, the padding is now moved into the PRIregister and
>> varies depending on the architecture.
> I'm not sure this is the right thing to do. There are lots of code
> (especially in vgic) that does not use padding at all. Now it will print
> padding, even if original author does not wanted to. And, honestly it is
> hard to parse 15-16 zeroes in a row.

Well, I am usually starting to read from the right to left. So, for me, 15-16 
zeroes are easy to ignore ;).

> 
> I am suggesting to add another macro like PRIregister_pad or something
> like that.

No, we should print register the same way everywhere. I am clearly against
providing two different formats here for the same type. Otherwise this will lead 
to endless debate on which one you will chose in the code.

Looking at the vGIC, they are mostly print for debug. If you reach them then 
there are probably already something wrong happening. So I think we can leave 
the extra zeroes there.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 3/7] xen/arm: Rework psr_mode_is_32bit()

[Volodymyr Babchuk]

Julien Grall writes:

> Hi,
>
> On 26/07/2019 13:31, Volodymyr Babchuk wrote:
>>
>> Julien Grall writes:
>>
>>> psr_mode_is_32bit() prototype does not match the rest of the helpers for
>>> the process state. Looking at the callers, most of them will access
>>> struct cpu_user_regs just for calling psr_mode_is_32bit().
>>>
>>> The macro is now reworked to take a struct cpu_user_regs in parameter.
>>> At the same time take the opportunity to switch to a static inline
>>> helper.
>> I'm a bit concerned about naming now. As psr_mode_is_32bit() is now have
>> no psr parameter, and ARM ARM uses term "state" instead of "mode", maybe
>> it is worth to rename this helper to something like "is_32bit_state"?
>
> It really depends how you see it. The bit is part of the "mode" field,
> so technically we are checking whether the mode corresponds to a
> 32-bit one or not. This is also inline with the rest of the helpers
> within this header.
>
> I would be willing to consider renaming the helper to regs_mode_is_32bit().
I'm fine with this name.

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

Re: [Xen-devel] [PATCH 7/7] xen/arm: types: Specify the zero padding in the definition of PRIregister

[Volodymyr Babchuk]

Julien Grall writes:

> Hi,
>
> On 26/07/2019 13:47, Volodymyr Babchuk wrote:
>>
>> Julien Grall writes:
>>
>>> The definition of PRIregister varies between Arm32 and Arm64 (32-bit vs
>>> 64-bit). However, some of the users uses the wrong padding.
>> type: "users use"
>>
>>> For more consistency, the padding is now moved into the PRIregister and
>>> varies depending on the architecture.
>> I'm not sure this is the right thing to do. There are lots of code
>> (especially in vgic) that does not use padding at all. Now it will print
>> padding, even if original author does not wanted to. And, honestly it is
>> hard to parse 15-16 zeroes in a row.
>
> Well, I am usually starting to read from the right to left. So, for
> me, 15-16 zeroes are easy to ignore ;).
And what if there only one bit set on position 31 or 35? :)
Personally, I'd like to see such number grouped like "FEDCBA98 76543210"
Anyways, this is matter of personal taste. I'm okay with padding.

>>
>> I am suggesting to add another macro like PRIregister_pad or something
>> like that.
>
> No, we should print register the same way everywhere. I am clearly against
> providing two different formats here for the same type. Otherwise this
> will lead to endless debate on which one you will chose in the code.
Okay then. But at least you should mention in the commit message, that
this change will affect other prints, not only the ones in the diff.

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

Re: [Xen-devel] [PATCH 7/7] xen/arm: types: Specify the zero padding in the definition of PRIregister

[Julien Grall]

On 26/07/2019 15:21, Volodymyr Babchuk wrote:
> 
> Julien Grall writes:
> 
>> Hi,
>>
>> On 26/07/2019 13:47, Volodymyr Babchuk wrote:
>>>
>>> Julien Grall writes:
>>>
>>>> The definition of PRIregister varies between Arm32 and Arm64 (32-bit vs
>>>> 64-bit). However, some of the users uses the wrong padding.
>>> type: "users use"
>>>
>>>> For more consistency, the padding is now moved into the PRIregister and
>>>> varies depending on the architecture.
>>> I'm not sure this is the right thing to do. There are lots of code
>>> (especially in vgic) that does not use padding at all. Now it will print
>>> padding, even if original author does not wanted to. And, honestly it is
>>> hard to parse 15-16 zeroes in a row.
>>
>> Well, I am usually starting to read from the right to left. So, for
>> me, 15-16 zeroes are easy to ignore ;).
> And what if there only one bit set on position 31 or 35? :)

That's why you have tools to help you figuring out the position of a bit...

> Personally, I'd like to see such number grouped like "FEDCBA98 76543210"
> Anyways, this is matter of personal taste. I'm okay with padding.

Patch are welcomed ;).

> 
>>>
>>> I am suggesting to add another macro like PRIregister_pad or something
>>> like that.
>>
>> No, we should print register the same way everywhere. I am clearly against
>> providing two different formats here for the same type. Otherwise this
>> will lead to endless debate on which one you will chose in the code.
> Okay then. But at least you should mention in the commit message, that
> this change will affect other prints, not only the ones in the diff.

This was kind of implied with "consistency". But I am not going to argue here, 
there are more important stuff to focus.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 7/7] xen/arm: types: Specify the zero padding in the definition of PRIregister

[Julien Grall]

On 23/07/2019 22:35, Julien Grall wrote:
> The definition of PRIregister varies between Arm32 and Arm64 (32-bit vs
> 64-bit). However, some of the users uses the wrong padding.
> 
> For more consistency, the padding is now moved into the PRIregister and
> varies depending on the architecture.

Below a suggested new commit message:

"The definition of PRIregister varies between Arm32 and Arm64 (32-bit vs
64-bit). However, some of the users uses the wrong padding and others are
not using padding at all.

For more consistency, the padding is now moved into the PRIregister and
varies depending on the architecture."

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 7/7] xen/arm: types: Specify the zero padding in the definition of PRIregister

[Volodymyr Babchuk]

Julien Grall writes:

> On 23/07/2019 22:35, Julien Grall wrote:
>> The definition of PRIregister varies between Arm32 and Arm64 (32-bit vs
>> 64-bit). However, some of the users uses the wrong padding.
>>
>> For more consistency, the padding is now moved into the PRIregister and
>> varies depending on the architecture.
>
> Below a suggested new commit message:
>
> "The definition of PRIregister varies between Arm32 and Arm64 (32-bit vs
> 64-bit). However, some of the users uses the wrong padding and others are
> not using padding at all.
>
> For more consistency, the padding is now moved into the PRIregister and
> varies depending on the architecture."
With this commit message:

Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

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

Re: [Xen-devel] [PATCH 1/7] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Stefano Stabellini]

On Fri, 26 Jul 2019, Volodymyr Babchuk wrote:
> > On 26/07/2019 13:14, Volodymyr Babchuk wrote:
> >>
> >> Hi Julien,
> >
> > Hi Volodymyr,
> >
> >> Julien Grall writes:
> >>
> >>> Currently, the structure vcpu_guest_core_regs is part of the public API.
> >>> This implies that any change in the structure should be backward
> >>> compatible.
> >>>
> >>> However, the structure is only needed by the tools and Xen. It is also
> >>> not expected to be ever used outside of that context. So we could save us
> >>> some headache by only declaring the structure for Xen and tools.
> >>>
> >>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> >>> ---
> >>>      This is a follow-up of the discussion [1].
> >>>
> >>>      [1] <3c245c5b-51c6-1d0e-ad6c-42414573166f@arm.com>
> >>>
> >>>      Changes in v3:
> >>>          - Avoid introduce a new #ifdef in the header by moving the
> >>>          definitions later on.
> >>> ---
> >>>   xen/include/public/arch-arm.h | 24 ++++++++++++------------
> >>>   1 file changed, 12 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> >>> index 3e8cdc151d..7ce139a0f5 100644
> >>> --- a/xen/include/public/arch-arm.h
> >>> +++ b/xen/include/public/arch-arm.h
> >>> @@ -197,6 +197,18 @@
> >>>       } while ( 0 )
> >>>   #define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
> >>>   +typedef uint64_t xen_pfn_t;
> >>> +#define PRI_xen_pfn PRIx64
> >>> +#define PRIu_xen_pfn PRIu64
> >>> +
> >>> +/* Maximum number of virtual CPUs in legacy multi-processor guests. */
> >>> +/* Only one. All other VCPUS must use VCPUOP_register_vcpu_info */
> >> Just a suggestion: you already touching this part. Maybe you'll fix this
> >> comment as well?
> >
> > I am not sure what's wrong with the current comment. Can you expand
> > your thoughts please?
> Sure. It does not conform to CODING_STYLE:
> 
>    Comments containing a single sentence may end with a full
>    stop; comments containing several sentences must have a full stop
>    after each sentence.
> 
> The second comment misses full stop at the end. Also, maybe we should
> consider this as s multi-line comment:
> 
>    Multi-line comment blocks should start and end with comment markers on
>    separate lines and each line should begin with a leading '*'.

I'll improve on commit

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

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

Re: [Xen-devel] [PATCH 2/7] xen/arm: SCTLR_EL1 is a 64-bit register on Arm64

[Stefano Stabellini]

On Fri, 26 Jul 2019, Volodymyr Babchuk wrote:
> Hi,
> 
> Julien Grall writes:
> 
> > On Arm64, system registers are always 64-bit including SCTLR_EL1.
> > However, Xen is assuming this is 32-bit because earlier revision of
> > Armv8 had the top 32-bit RES0 (see ARM DDI0595.b).
> >
> > From Armv8.5, some bits in [63:32] will be defined and allowed to be
> > modified by the guest. So we would effectively reset those bits to 0
> > after each context switch. This means the guest may not function
> > correctly afterwards.
> >
> > Rather than resetting to 0 the bits [63:32], preserve them acxcross
> typo: across
> > context switch.
> >
> > Note that the corresponding register on Arm32 (i.e SCTLR) is always
> > 32-bit. So we need to use register_t anywhere we deal the SCTLR{,_EL1}.
> >
> > Outside interface is switched to use 64-bit to allow ABI compatibility
> > between 32-bit and 64-bit.
> >
> > Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> Providing that typo will be fixed:
> Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Fixed the typo on commit


> >
> > ---
> >     All the other system registers should be switched to 64-bit. This is
> >     done separatly as this is the only system register that currently
> >     not save/restore correctly.
> >
> >     I would consider to backport it as we would end up to disable
> >     features behind the back of the guest.
> > ---
> >  tools/xentrace/xenctx.c       |  4 +++-
> >  xen/arch/arm/guest_walk.c     |  2 +-
> >  xen/arch/arm/traps.c          | 10 +++++-----
> >  xen/include/asm-arm/domain.h  |  3 ++-
> >  xen/include/asm-arm/p2m.h     |  4 ++--
> >  xen/include/public/arch-arm.h |  4 ++--
> >  6 files changed, 15 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
> > index e647179e19..2fa864f867 100644
> > --- a/tools/xentrace/xenctx.c
> > +++ b/tools/xentrace/xenctx.c
> > @@ -598,6 +598,8 @@ static void print_ctx_32(vcpu_guest_context_t *ctx)
> >  
> >      printf("r12_fiq: %08"PRIx32"\n", regs->r12_fiq);
> >      printf("\n");
> > +    /* SCTLR is always 32-bit */
> > +    printf("SCTLR: %08"PRIx32"\n", (uint32_t)ctx->sctlr);
> >  }
> >  
> >  #ifdef __aarch64__
> > @@ -659,6 +661,7 @@ static void print_ctx_64(vcpu_guest_context_t *ctx)
> >      printf("x28: %016"PRIx64"\t", regs->x28);
> >      printf("x29: %016"PRIx64"\n", regs->x29);
> >      printf("\n");
> > +    printf("SCTLR_EL1: %016"PRIx64"\n", ctx->sctlr);
> >  }
> >  #endif /* __aarch64__ */
> >  
> > @@ -675,7 +678,6 @@ static void print_ctx(vcpu_guest_context_any_t *ctx_any)
> >      print_ctx_32(ctx);
> >  #endif
> >  
> > -    printf("SCTLR: %08"PRIx32"\n", ctx->sctlr);
> >      printf("TTBCR: %016"PRIx64"\n", ctx->ttbcr);
> >      printf("TTBR0: %016"PRIx64"\n", ctx->ttbr0);
> >      printf("TTBR1: %016"PRIx64"\n", ctx->ttbr1);
> > diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> > index c6d6e23bf5..a1cdd7f4af 100644
> > --- a/xen/arch/arm/guest_walk.c
> > +++ b/xen/arch/arm/guest_walk.c
> > @@ -589,7 +589,7 @@ static bool guest_walk_ld(const struct vcpu *v,
> >  bool guest_walk_tables(const struct vcpu *v, vaddr_t gva,
> >                         paddr_t *ipa, unsigned int *perms)
> >  {
> > -    uint32_t sctlr = READ_SYSREG(SCTLR_EL1);
> > +    register_t sctlr = READ_SYSREG(SCTLR_EL1);
> >      register_t tcr = READ_SYSREG(TCR_EL1);
> >      unsigned int _perms;
> >  
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 3103620323..111a2029e6 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -384,7 +384,7 @@ void panic_PAR(uint64_t par)
> >  
> >  static void cpsr_switch_mode(struct cpu_user_regs *regs, int mode)
> >  {
> > -    uint32_t sctlr = READ_SYSREG32(SCTLR_EL1);
> > +    register_t sctlr = READ_SYSREG(SCTLR_EL1);
> >  
> >      regs->cpsr &= ~(PSR_MODE_MASK|PSR_IT_MASK|PSR_JAZELLE|PSR_BIG_ENDIAN|PSR_THUMB);
> >  
> > @@ -400,7 +400,7 @@ static void cpsr_switch_mode(struct cpu_user_regs *regs, int mode)
> >  
> >  static vaddr_t exception_handler32(vaddr_t offset)
> >  {
> > -    uint32_t sctlr = READ_SYSREG32(SCTLR_EL1);
> > +    register_t sctlr = READ_SYSREG(SCTLR_EL1);
> >  
> >      if ( sctlr & SCTLR_A32_EL1_V )
> >          return 0xffff0000 + offset;
> > @@ -719,7 +719,7 @@ crash_system:
> >  
> >  struct reg_ctxt {
> >      /* Guest-side state */
> > -    uint32_t sctlr_el1;
> > +    register_t sctlr_el1;
> >      register_t tcr_el1;
> >      uint64_t ttbr0_el1, ttbr1_el1;
> >  #ifdef CONFIG_ARM_32
> > @@ -822,7 +822,7 @@ static void show_registers_32(const struct cpu_user_regs *regs,
> >  
> >      if ( guest_mode )
> >      {
> > -        printk("     SCTLR: %08"PRIx32"\n", ctxt->sctlr_el1);
> > +        printk("     SCTLR: %"PRIregister"\n", ctxt->sctlr_el1);
> >          printk("       TCR: %08"PRIregister"\n", ctxt->tcr_el1);
> >          printk("     TTBR0: %016"PRIx64"\n", ctxt->ttbr0_el1);
> >          printk("     TTBR1: %016"PRIx64"\n", ctxt->ttbr1_el1);
> > @@ -894,7 +894,7 @@ static void show_registers_64(const struct cpu_user_regs *regs,
> >          printk("   ESR_EL1: %08"PRIx32"\n", ctxt->esr_el1);
> >          printk("   FAR_EL1: %016"PRIx64"\n", ctxt->far);
> >          printk("\n");
> > -        printk(" SCTLR_EL1: %08"PRIx32"\n", ctxt->sctlr_el1);
> > +        printk(" SCTLR_EL1: %"PRIregister"\n", ctxt->sctlr_el1);
> >          printk("   TCR_EL1: %08"PRIregister"\n", ctxt->tcr_el1);
> >          printk(" TTBR0_EL1: %016"PRIx64"\n", ctxt->ttbr0_el1);
> >          printk(" TTBR1_EL1: %016"PRIx64"\n", ctxt->ttbr1_el1);
> > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> > index 2960a53e69..86ebdd2bcf 100644
> > --- a/xen/include/asm-arm/domain.h
> > +++ b/xen/include/asm-arm/domain.h
> > @@ -167,7 +167,8 @@ struct arch_vcpu
> >  #endif
> >  
> >      /* Control Registers */
> > -    uint32_t actlr, sctlr;
> > +    register_t sctlr;
> > +    uint32_t actlr;
> >      uint32_t cpacr;
> >  
> >      uint32_t contextidr;
> > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> > index 2f89bb00c3..03f2ee75c1 100644
> > --- a/xen/include/asm-arm/p2m.h
> > +++ b/xen/include/asm-arm/p2m.h
> > @@ -391,12 +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;
> > +    const register_t mask = SCTLR_Axx_ELx_C | SCTLR_Axx_ELx_M;
> >  
> >      /* Only works with the current vCPU */
> >      ASSERT(current == v);
> >  
> > -    return (READ_SYSREG32(SCTLR_EL1) & mask) == mask;
> > +    return (READ_SYSREG(SCTLR_EL1) & mask) == mask;
> >  }
> >  
> >  #endif /* _XEN_P2M_H */
> > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> > index 7ce139a0f5..d9a06efbd8 100644
> > --- a/xen/include/public/arch-arm.h
> > +++ b/xen/include/public/arch-arm.h
> > @@ -291,7 +291,7 @@ struct vcpu_guest_context {
> >  
> >      struct vcpu_guest_core_regs user_regs;  /* Core CPU registers */
> >  
> > -    uint32_t sctlr;
> > +    uint64_t sctlr;
> >      uint64_t ttbcr, ttbr0, ttbr1;
> >  };
> >  typedef struct vcpu_guest_context vcpu_guest_context_t;
> > @@ -380,7 +380,7 @@ typedef uint64_t xen_callback_t;
> >  #define PSR_GUEST32_INIT  (PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_SVC)
> >  #define PSR_GUEST64_INIT (PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_EL1h)
> >  
> > -#define SCTLR_GUEST_INIT    0x00c50078
> > +#define SCTLR_GUEST_INIT    xen_mk_ullong(0x00c50078)
> >  
> >  /*
> >   * Virtual machine platform (memory layout, interrupts)
> 
> 
> -- 
> Volodymyr Babchuk at EPAM

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

Re: [Xen-devel] [PATCH 3/7] xen/arm: Rework psr_mode_is_32bit()

[Stefano Stabellini]

On Fri, 26 Jul 2019, Volodymyr Babchuk wrote:
> Julien Grall writes:
> 
> > Hi,
> >
> > On 26/07/2019 13:31, Volodymyr Babchuk wrote:
> >>
> >> Julien Grall writes:
> >>
> >>> psr_mode_is_32bit() prototype does not match the rest of the helpers for
> >>> the process state. Looking at the callers, most of them will access
> >>> struct cpu_user_regs just for calling psr_mode_is_32bit().
> >>>
> >>> The macro is now reworked to take a struct cpu_user_regs in parameter.
> >>> At the same time take the opportunity to switch to a static inline
> >>> helper.
> >> I'm a bit concerned about naming now. As psr_mode_is_32bit() is now have
> >> no psr parameter, and ARM ARM uses term "state" instead of "mode", maybe
> >> it is worth to rename this helper to something like "is_32bit_state"?
> >
> > It really depends how you see it. The bit is part of the "mode" field,
> > so technically we are checking whether the mode corresponds to a
> > 32-bit one or not. This is also inline with the rest of the helpers
> > within this header.
> >
> > I would be willing to consider renaming the helper to regs_mode_is_32bit().
> I'm fine with this name.

The patch is fine by me, as is, or with the name changed to
regs_mode_is_32bit. (I didn't commit it.)

Either way:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


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

Re: [Xen-devel] [PATCH 4/7] xen/arm: traps: Avoid using BUG_ON() in _show_registers()

[Stefano Stabellini]

On Fri, 26 Jul 2019, Volodymyr Babchuk wrote:
> Julien Grall writes:
> 
> > At the moment, _show_registers() is using a BUG_ON() to assert only
> > userspace will run 32-bit code in a 64-bit domain.
> >
> > Such extra precaution is not necessary and could be avoided by only
> > checking the CPU mode to decide whether show_registers_64() or
> > show_reigsters_32() should be called.
> >
> > This has also the nice advantage to avoid nested if in the code.
> >
> > Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> > ---
> >  xen/arch/arm/traps.c | 16 +++-------------
> >  1 file changed, 3 insertions(+), 13 deletions(-)
> >
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 54e66a86d0..132686ee0f 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -914,21 +914,11 @@ static void _show_registers(const struct cpu_user_regs *regs,
> >  
> >      if ( guest_mode )
> >      {
> > -        if ( is_32bit_domain(v->domain) )
> > +        if ( psr_mode_is_32bit(regs) )
> >              show_registers_32(regs, ctxt, guest_mode, v);
> >  #ifdef CONFIG_ARM_64
> > -        else if ( is_64bit_domain(v->domain) )
> > -        {
> > -            if ( psr_mode_is_32bit(regs) )
> > -            {
> > -                BUG_ON(!usr_mode(regs));
> > -                show_registers_32(regs, ctxt, guest_mode, v);
> > -            }
> > -            else
> > -            {
> > -                show_registers_64(regs, ctxt, guest_mode, v);
> > -            }
> > -        }
> > +        else
> > +            show_registers_64(regs, ctxt, guest_mode, v);
> >  #endif
> >      }
> >      else
> 
> 
> -- 
> Volodymyr Babchuk at EPAM

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

Re: [Xen-devel] [PATCH 5/7] xen/arm: traps: Avoid BUG_ON() in do_trap_brk()

[Stefano Stabellini]

On Tue, 23 Jul 2019, Julien Grall wrote:
> At the moment, do_trap_brk() is using a BUG_ON() to check the hardware
> has been correctly configured during boot.
> 
> Any error when configuring the hardware could result to a guest 'brk'
> trapping in the hypervisor and crash it.
> 
> This is pretty harsh to kill Xen when actually killing the guest would
> be enough as misconfiguring this trap would not lead to exposing
> sensitive data. Replace the BUG_ON() with crashing the guest.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/traps.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 132686ee0f..ef37ca6bde 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1304,10 +1304,15 @@ int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
>  #ifdef CONFIG_ARM_64
>  static void do_trap_brk(struct cpu_user_regs *regs, const union hsr hsr)
>  {
> -    /* HCR_EL2.TGE and MDCR_EL2.TDE are not set so we never receive
> -     * software breakpoint exception for EL1 and EL0 here.
> +    /*
> +     * HCR_EL2.TGE and MDCR_EL2.TDR are currently not set. So we should
> +     * never receive software breakpoing exception for EL1 and EL0 here.
>       */
> -    BUG_ON(!hyp_mode(regs));
> +    if ( !hyp_mode(regs) )
> +    {
> +        domain_crash(current->domain);
> +        return;
> +    }

This is a good change to have. I am wondering if it would make sense to
also printk some debug messages, maybe even show_execution_state. If so,
we need to be careful that it's only done in debug builds, or it is rate
limited. What do you think? In any case, it is not necessary for this
patch to be accepted so:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

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

Re: [Xen-devel] [PATCH 6/7] xen/arm: vsmc: The function identifier is always 32-bit

[Stefano Stabellini]

On Fri, 26 Jul 2019, Volodymyr Babchuk wrote:
> Julien Grall writes:
> 
> > On Arm64, the SMCCC function identifier is always stored in the first 32-bit
> > of x0 register. The rest of the bits are not defined and should be
> > ignored.
> >
> > This means the variable funcid should be an uint32_t rather than
> > register_t.
> >
> > Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

> > ---
> >  xen/arch/arm/vsmc.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> > index f8e350311d..a36db15fff 100644
> > --- a/xen/arch/arm/vsmc.c
> > +++ b/xen/arch/arm/vsmc.c
> > @@ -220,7 +220,7 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
> >  {
> >      bool handled = false;
> >      const union hsr hsr = { .bits = regs->hsr };
> > -    register_t funcid = get_user_reg(regs, 0);
> > +    uint32_t funcid = get_user_reg(regs, 0);
> >  
> >      /*
> >       * Check immediate value for HVC32, HVC64 and SMC64.
> > @@ -286,7 +286,7 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
> >  
> >      if ( !handled )
> >      {
> > -        gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n", funcid);
> > +        gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %#x\n", funcid);
> >  
> >          /* Inform caller that function is not supported. */
> >          set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
> 
> 
> -- 
> Volodymyr Babchuk at EPAM

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

Re: [Xen-devel] [PATCH 7/7] xen/arm: types: Specify the zero padding in the definition of PRIregister

[Stefano Stabellini]

On Fri, 26 Jul 2019, Volodymyr Babchuk wrote:
> > On 23/07/2019 22:35, Julien Grall wrote:
> >> The definition of PRIregister varies between Arm32 and Arm64 (32-bit vs
> >> 64-bit). However, some of the users uses the wrong padding.
> >>
> >> For more consistency, the padding is now moved into the PRIregister and
> >> varies depending on the architecture.
> >
> > Below a suggested new commit message:
> >
> > "The definition of PRIregister varies between Arm32 and Arm64 (32-bit vs
> > 64-bit). However, some of the users uses the wrong padding and others are
> > not using padding at all.
> >
> > For more consistency, the padding is now moved into the PRIregister and
> > varies depending on the architecture."
> With this commit message:
> 
> Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

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

Re: [Xen-devel] [PATCH 5/7] xen/arm: traps: Avoid BUG_ON() in do_trap_brk()

[Julien Grall]

Hi Stefano,

On 7/29/19 11:02 PM, Stefano Stabellini wrote:
> On Tue, 23 Jul 2019, Julien Grall wrote:
>> At the moment, do_trap_brk() is using a BUG_ON() to check the hardware
>> has been correctly configured during boot.
>>
>> Any error when configuring the hardware could result to a guest 'brk'
>> trapping in the hypervisor and crash it.
>>
>> This is pretty harsh to kill Xen when actually killing the guest would
>> be enough as misconfiguring this trap would not lead to exposing
>> sensitive data. Replace the BUG_ON() with crashing the guest.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/traps.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 132686ee0f..ef37ca6bde 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -1304,10 +1304,15 @@ int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
>>   #ifdef CONFIG_ARM_64
>>   static void do_trap_brk(struct cpu_user_regs *regs, const union hsr hsr)
>>   {
>> -    /* HCR_EL2.TGE and MDCR_EL2.TDE are not set so we never receive
>> -     * software breakpoint exception for EL1 and EL0 here.
>> +    /*
>> +     * HCR_EL2.TGE and MDCR_EL2.TDR are currently not set. So we should
>> +     * never receive software breakpoing exception for EL1 and EL0 here.
>>        */
>> -    BUG_ON(!hyp_mode(regs));
>> +    if ( !hyp_mode(regs) )
>> +    {
>> +        domain_crash(current->domain);
>> +        return;
>> +    }
> 
> This is a good change to have. I am wondering if it would make sense to
> also printk some debug messages, maybe even show_execution_state. If so,
> we need to be careful that it's only done in debug builds, or it is rate
> limited. What do you think?

Messages are already printed by domain_crash(...). But I don't see the 
concern about non-debug build or even not rate limiting... If the domain 
crash, then it will not cause anymore print and therefore you can't spam 
the console here.

> In any case, it is not necessary for this
> patch to be accepted so:
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 5/7] xen/arm: traps: Avoid BUG_ON() in do_trap_brk()

[Stefano Stabellini]

On Tue, 30 Jul 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 7/29/19 11:02 PM, Stefano Stabellini wrote:
> > On Tue, 23 Jul 2019, Julien Grall wrote:
> > > At the moment, do_trap_brk() is using a BUG_ON() to check the hardware
> > > has been correctly configured during boot.
> > > 
> > > Any error when configuring the hardware could result to a guest 'brk'
> > > trapping in the hypervisor and crash it.
> > > 
> > > This is pretty harsh to kill Xen when actually killing the guest would
> > > be enough as misconfiguring this trap would not lead to exposing
> > > sensitive data. Replace the BUG_ON() with crashing the guest.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > ---
> > >   xen/arch/arm/traps.c | 11 ++++++++---
> > >   1 file changed, 8 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > > index 132686ee0f..ef37ca6bde 100644
> > > --- a/xen/arch/arm/traps.c
> > > +++ b/xen/arch/arm/traps.c
> > > @@ -1304,10 +1304,15 @@ int do_bug_frame(const struct cpu_user_regs *regs,
> > > vaddr_t pc)
> > >   #ifdef CONFIG_ARM_64
> > >   static void do_trap_brk(struct cpu_user_regs *regs, const union hsr hsr)
> > >   {
> > > -    /* HCR_EL2.TGE and MDCR_EL2.TDE are not set so we never receive
> > > -     * software breakpoint exception for EL1 and EL0 here.
> > > +    /*
> > > +     * HCR_EL2.TGE and MDCR_EL2.TDR are currently not set. So we should
> > > +     * never receive software breakpoing exception for EL1 and EL0 here.
> > >        */
> > > -    BUG_ON(!hyp_mode(regs));
> > > +    if ( !hyp_mode(regs) )
> > > +    {
> > > +        domain_crash(current->domain);
> > > +        return;
> > > +    }
> > 
> > This is a good change to have. I am wondering if it would make sense to
> > also printk some debug messages, maybe even show_execution_state. If so,
> > we need to be careful that it's only done in debug builds, or it is rate
> > limited. What do you think?
> 
> Messages are already printed by domain_crash(...). But I don't see the concern
> about non-debug build or even not rate limiting... If the domain crash, then
> it will not cause anymore print and therefore you can't spam the console here.

Ah yes, you are quite right

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

Re: [Xen-devel] [PATCH 3/7] xen/arm: Rework psr_mode_is_32bit()

[Julien Grall]

Hi Stefano,

On 29/07/2019 22:52, Stefano Stabellini wrote:
> On Fri, 26 Jul 2019, Volodymyr Babchuk wrote:
>> Julien Grall writes:
>>
>>> Hi,
>>>
>>> On 26/07/2019 13:31, Volodymyr Babchuk wrote:
>>>>
>>>> Julien Grall writes:
>>>>
>>>>> psr_mode_is_32bit() prototype does not match the rest of the helpers for
>>>>> the process state. Looking at the callers, most of them will access
>>>>> struct cpu_user_regs just for calling psr_mode_is_32bit().
>>>>>
>>>>> The macro is now reworked to take a struct cpu_user_regs in parameter.
>>>>> At the same time take the opportunity to switch to a static inline
>>>>> helper.
>>>> I'm a bit concerned about naming now. As psr_mode_is_32bit() is now have
>>>> no psr parameter, and ARM ARM uses term "state" instead of "mode", maybe
>>>> it is worth to rename this helper to something like "is_32bit_state"?
>>>
>>> It really depends how you see it. The bit is part of the "mode" field,
>>> so technically we are checking whether the mode corresponds to a
>>> 32-bit one or not. This is also inline with the rest of the helpers
>>> within this header.
>>>
>>> I would be willing to consider renaming the helper to regs_mode_is_32bit().
>> I'm fine with this name.
> 
> The patch is fine by me, as is, or with the name changed to
> regs_mode_is_32bit. (I didn't commit it.)

I am thinking to get the renaming separately. So I can also rework the other 
helpers.

> 
> Either way:
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

So I will commit as is and add in my todo list renaming.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 5/7] xen/arm: traps: Avoid BUG_ON() in do_trap_brk()

[Andrew Cooper]

On 30/07/2019 18:00, Stefano Stabellini wrote:
> On Tue, 30 Jul 2019, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 7/29/19 11:02 PM, Stefano Stabellini wrote:
>>> On Tue, 23 Jul 2019, Julien Grall wrote:
>>>> At the moment, do_trap_brk() is using a BUG_ON() to check the hardware
>>>> has been correctly configured during boot.
>>>>
>>>> Any error when configuring the hardware could result to a guest 'brk'
>>>> trapping in the hypervisor and crash it.
>>>>
>>>> This is pretty harsh to kill Xen when actually killing the guest would
>>>> be enough as misconfiguring this trap would not lead to exposing
>>>> sensitive data. Replace the BUG_ON() with crashing the guest.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>> ---
>>>>   xen/arch/arm/traps.c | 11 ++++++++---
>>>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>>> index 132686ee0f..ef37ca6bde 100644
>>>> --- a/xen/arch/arm/traps.c
>>>> +++ b/xen/arch/arm/traps.c
>>>> @@ -1304,10 +1304,15 @@ int do_bug_frame(const struct cpu_user_regs *regs,
>>>> vaddr_t pc)
>>>>   #ifdef CONFIG_ARM_64
>>>>   static void do_trap_brk(struct cpu_user_regs *regs, const union hsr hsr)
>>>>   {
>>>> -    /* HCR_EL2.TGE and MDCR_EL2.TDE are not set so we never receive
>>>> -     * software breakpoint exception for EL1 and EL0 here.
>>>> +    /*
>>>> +     * HCR_EL2.TGE and MDCR_EL2.TDR are currently not set. So we should
>>>> +     * never receive software breakpoing exception for EL1 and EL0 here.
>>>>        */
>>>> -    BUG_ON(!hyp_mode(regs));
>>>> +    if ( !hyp_mode(regs) )
>>>> +    {
>>>> +        domain_crash(current->domain);
>>>> +        return;
>>>> +    }
>>> This is a good change to have. I am wondering if it would make sense to
>>> also printk some debug messages, maybe even show_execution_state. If so,
>>> we need to be careful that it's only done in debug builds, or it is rate
>>> limited. What do you think?
>> Messages are already printed by domain_crash(...). But I don't see the concern
>> about non-debug build or even not rate limiting... If the domain crash, then
>> it will not cause anymore print and therefore you can't spam the console here.
> Ah yes, you are quite right

I still have a series pending to force people to put a useful printk()
in, because there is nothing more infuriating than to find a
domain_crash() with no clarifying context.

It should be a gprintk(XENLOG_ERR, and will eventually be folded into
domain_crash()'s prototype.

~Andrew

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