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

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

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