From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH] xen: arm: Fixing ttbcr (TCR_EL1 for AArch64) size. Date: Tue, 03 Dec 2013 16:09:08 +0000 Message-ID: <529E0224.3000407@linaro.org> References: <1386072224-4478-1-git-send-email-pranavkumar@linaro.org> <1386078453.13256.35.camel@kazak.uk.xensource.com> <1386085234.13256.49.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1386085234.13256.49.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: Anup Patel , patches@linaro.org, patches@apm.com, xen-devel@lists.xen.org, julien.grall@citrix.com, stefano.stabellini@citrix.com, Pranavkumar Sawargaonkar List-Id: xen-devel@lists.xenproject.org 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 >>> Signed-off-by: Pranavkumar Sawargaonkar >> [...] >>> 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 > 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 > Signed-off-by: Ian Campbell > Cc: Anup Patel > Cc: patches@linaro.org > Cc: patches@apm.com Acked-by: Julien Grall > --- > 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