Re: [Xen-devel] [PATCH] xen: arm: Fixing ttbcr (TCR_EL1 for AArch64) size.

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 12/03/2013 03:40 PM, Ian Campbell wrote:
> On Tue, 2013-12-03 at 13:47 +0000, Ian Campbell wrote:
>> On Tue, 2013-12-03 at 17:33 +0530, Pranavkumar Sawargaonkar wrote:
>>> This patch fixes size of ttbcr register (TCR_EL1 in case of AArch64)
>>> and it's programming considering size in case of context switch.
>>>
>>> Currently ttbcr is defined as 32b register but for AArch64 TCR_EL1
>>> size is 64b.
>>
>> Thanks for tracking this down.
>>
>>> Signed-off-by: Anup Patel <anup.patel@xxxxxxxxxx>
>>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@xxxxxxxxxx>
>> [...]
>>> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
>>> index d5cae2e..2aa4443 100644
>>> --- a/xen/include/asm-arm/domain.h
>>> +++ b/xen/include/asm-arm/domain.h
>>> @@ -165,7 +165,11 @@ struct arch_vcpu
>>>  
>>>      /* MMU */
>>>      register_t vbar;
>>> +#ifdef CONFIG_ARM_32
>>>      uint32_t ttbcr;
>>> +#else
>>> +    uint64_t ttbcr;
>>> +#endif
>>
>> I think that actually only this hunk is required. 
> 
> Actually, it turns out there a few other places where ttbcr is
> incorrectly stored in a 32-bit variable. Including one which needed some
> careful consideration before it was safe to change. Here is what I came
> up with.
> 
> -------------8<---------------
> 
> From 832544695c39a7d3f98e73381010eaab491982cc Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Date: Tue, 3 Dec 2013 15:13:36 +0000
> Subject: [PATCH] xen: arm: TCR_EL1 is 64-bit on arm64
> 
> Storing it in a 32-bit variable in struct arch_vcpu caused breakage over
> context switch.
> 
> There were also several other places which stored this as the 32-bit value.
> Update them all.
> 
> The "struct vcpu_guest_context" case needs special consideration. This struct
> is in theory is exposed to guests, via the VCPUOP_initialise hypercall.
> However as discussed in
> http://lists.xen.org/archives/html/xen-devel/2013-10/msg00912.html this isn't
> really a guest visible interface since ARM uses PSCI for VCPU bringup
> (VCPUOP_initialise simply isn't available) The other users of this interface
> are the domctls, which are not a stable API. Therefore while fixing the ttbcr
> size also surround the struct in ifdefs to restrict the struct to the
> hypervisor and the tools only (omitting the extra complexity of renaming as I
> suggested in the referenced thread)
> 
> NB TCR_EL1 on arm64 is known as TTBCR on arm32, hence the apparent naming
> inconsistencies.
> 
> Spotted-by: Pranavkumar Sawargaonkar <pranavkumar@xxxxxxxxxx>
> Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Cc: Anup Patel <anup.patel@xxxxxxxxxx>
> Cc: patches@xxxxxxxxxx
> Cc: patches@xxxxxxx

Acked-by: Julien Grall <julien.grall@xxxxxxxxxx>

> ---
>  xen/arch/arm/traps.c          |   11 ++++++-----
>  xen/include/asm-arm/domain.h  |    2 +-
>  xen/include/public/arch-arm.h |    6 ++++--
>  3 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 53eb0cf..b2725df 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -330,7 +330,8 @@ static void inject_undef64_exception(struct cpu_user_regs 
> *regs, int instr_len)
>  
>  struct reg_ctxt {
>      /* Guest-side state */
> -    uint32_t sctlr_el1, tcr_el1;
> +    uint32_t sctlr_el1;
> +    register_t tcr_el1;
>      uint64_t ttbr0_el1, ttbr1_el1;
>  #ifdef CONFIG_ARM_32
>      uint32_t dfsr, ifsr;
> @@ -433,7 +434,7 @@ static void show_registers_32(struct cpu_user_regs *regs,
>      if ( guest_mode )
>      {
>          printk("     SCTLR: %08"PRIx32"\n", ctxt->sctlr_el1);
> -        printk("       TCR: %08"PRIx32"\n", ctxt->tcr_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);
>          printk("      IFAR: %08"PRIx32", IFSR: %08"PRIx32"\n"
> @@ -505,7 +506,7 @@ static void show_registers_64(struct cpu_user_regs *regs,
>          printk("   FAR_EL1: %016"PRIx64"\n", ctxt->far);
>          printk("\n");
>          printk(" SCTLR_EL1: %08"PRIx32"\n", ctxt->sctlr_el1);
> -        printk("   TCR_EL1: %08"PRIx32"\n", ctxt->tcr_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);
>          printk("\n");
> @@ -1261,14 +1262,14 @@ static void do_sysreg(struct cpu_user_regs *regs,
>  
>  void dump_guest_s1_walk(struct domain *d, vaddr_t addr)
>  {
> -    uint32_t ttbcr = READ_SYSREG32(TCR_EL1);
> +    register_t ttbcr = READ_SYSREG(TCR_EL1);
>      uint64_t ttbr0 = READ_SYSREG64(TTBR0_EL1);
>      paddr_t paddr;
>      uint32_t offset;
>      uint32_t *first = NULL, *second = NULL;
>  
>      printk("dom%d VA 0x%08"PRIvaddr"\n", d->domain_id, addr);
> -    printk("    TTBCR: 0x%08"PRIx32"\n", ttbcr);
> +    printk("    TTBCR: 0x%08"PRIregister"\n", ttbcr);
>      printk("    TTBR0: 0x%016"PRIx64" = 0x%"PRIpaddr"\n",
>             ttbr0, p2m_lookup(d, ttbr0 & PAGE_MASK));
>  
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index d5cae2e..8ebee3e 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -165,7 +165,7 @@ struct arch_vcpu
>  
>      /* MMU */
>      register_t vbar;
> -    uint32_t ttbcr;
> +    register_t ttbcr;
>      uint64_t ttbr0, ttbr1;
>  
>      uint32_t dacr; /* 32-bit guests only */
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index cb41ddc..475cb4a 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -278,6 +278,7 @@ typedef uint64_t xen_pfn_t;
>  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)
> @@ -285,11 +286,12 @@ struct vcpu_guest_context {
>  
>      struct vcpu_guest_core_regs user_regs;  /* Core CPU registers */
>  
> -    uint32_t sctlr, ttbcr;
> -    uint64_t ttbr0, ttbr1;
> +    uint32_t sctlr;
> +    uint64_t ttbcr, ttbr0, ttbr1;
>  };
>  typedef struct vcpu_guest_context vcpu_guest_context_t;
>  DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
> +#endif
>  
>  struct arch_vcpu_info {
>  };
> 


-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel