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

[Pranavkumar Sawargaonkar <pranavkumar@xxxxxxxxxx>]

On 3 December 2013 21:39, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> 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

Thanks for spotting other changes.
Patch looks good for me.

Thanks,
Pranav

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