| Message ID | 1386085234.13256.49.camel@kazak.uk.xensource.com |
|---|---|
| State | New |
| Headers | show |
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@linaro.org> >>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> >> [...] >>> 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@citrix.com> > 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@linaro.org> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > Cc: Anup Patel <anup.patel@linaro.org> > Cc: patches@linaro.org > Cc: patches@apm.com Acked-by: Julien Grall <julien.grall@linaro.org> > --- > 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 { > }; >
On 3 December 2013 21:39, Julien Grall <julien.grall@linaro.org> 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@linaro.org> >>>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> >>> [...] >>>> 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@citrix.com> >> 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@linaro.org> >> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> >> Cc: Anup Patel <anup.patel@linaro.org> >> Cc: patches@linaro.org >> Cc: patches@apm.com > > Acked-by: Julien Grall <julien.grall@linaro.org> > >> --- >> 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
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 { };