From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marc Zyngier Subject: Re: v4.9-rc1 fails booting as a guest on ARM64 Cavium ThunderX Date: Fri, 21 Oct 2016 14:47:43 +0100 Message-ID: <20161021144743.32b44b3f@arm.com> References: <924fd627-329a-54e0-694c-a9bdf979f696@redhat.com> <99c7d3b5-0495-083f-b4b7-b58b20cf859e@redhat.com> <5a5e4ed5-a575-f720-36f0-acb545a170f4@arm.com> <20161021114951.gm3u6msvl4jz2kv7@kamzik.brq.redhat.com> <20161021120732.bl5p5vji25zibap3@kamzik.brq.redhat.com> <20161021130639.6kxvi73kmagf2gwu@kamzik.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: Peter Maydell , Auger Eric , Daniel Thompson , "kvm@vger.kernel.org" , Robert Richter , "kvmarm@lists.cs.columbia.edu" To: Andrew Jones Return-path: Received: from foss.arm.com ([217.140.101.70]:44296 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755529AbcJUNrt (ORCPT ); Fri, 21 Oct 2016 09:47:49 -0400 In-Reply-To: <20161021130639.6kxvi73kmagf2gwu@kamzik.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Fri, 21 Oct 2016 15:06:39 +0200 Andrew Jones wrote: > On Fri, Oct 21, 2016 at 01:52:43PM +0100, Marc Zyngier wrote: > > On 21/10/16 13:07, Andrew Jones wrote: > > > On Fri, Oct 21, 2016 at 12:57:37PM +0100, Peter Maydell wrote: > > >> On 21 October 2016 at 12:49, Andrew Jones wrote: > > >>> I also read the register before writing it and saw it was 3. I tried > > >>> writing 3 instead of 0 to see what would happen, but the failure > > >>> persisted. I did read back the register after writing it to confirm the > > >>> change took affect. > > >> > > >> So what does it read back as after you write 0? The GICv3 spec > > >> says it can't read back as zero... > > >> > > > > > > I read back zero > > > > > > pre-read bpr1=3 > > > post-read bpr1=0 > > > FAIL: gicv3: ipi: self: Timed-out (5s). ACKS: missing=1 extra=0 unexpected=0 > > > > Gah... I guess we'll have to either roll a dice, or get someone from > > Cavium to tell us what's happening here. In the meantime, can you give > > the following patch a go? It doesn't fire on my FSL box, but everything > > hunky dory on it so far... > > > > Thanks, > > > > M. > > > > diff --git a/arch/arm/include/asm/arch_gicv3.h b/arch/arm/include/asm/arch_gicv3.h > > index a808829..5c03171 100644 > > --- a/arch/arm/include/asm/arch_gicv3.h > > +++ b/arch/arm/include/asm/arch_gicv3.h > > @@ -222,6 +222,11 @@ static inline void gic_write_bpr1(u32 val) > > write_sysreg(val, ICC_BPR1); > > } > > > > +static inline u32 gic_write_bpr1(void) > > +{ > > + return read_sysreg(ICC_BPR1); > > +} > > + > > /* > > * Even in 32bit systems that use LPAE, there is no guarantee that the I/O > > * interface provides true 64bit atomic accesses, so using strd/ldrd doesn't > > diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h > > index f8ae6d6..74fe2c9 100644 > > --- a/arch/arm64/include/asm/arch_gicv3.h > > +++ b/arch/arm64/include/asm/arch_gicv3.h > > @@ -184,6 +184,13 @@ static inline void gic_write_bpr1(u32 val) > > asm volatile("msr_s " __stringify(ICC_BPR1_EL1) ", %0" : : "r" (val)); > > } > > > > +static inline u32 gic_read_bpr1(void) > > +{ > > + u64 val; > > + asm volatile("mrs_s %0, " __stringify(ICC_BPR1_EL1) : "=r" (val)); > > + return val; > > +} > > + > > #define gic_read_typer(c) readq_relaxed(c) > > #define gic_write_irouter(v, c) writeq_relaxed(v, c) > > > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > > index 9b81bd8..db90286 100644 > > --- a/drivers/irqchip/irq-gic-v3.c > > +++ b/drivers/irqchip/irq-gic-v3.c > > @@ -482,6 +482,8 @@ static int gic_populate_rdist(void) > > > > static void gic_cpu_sys_reg_init(void) > > { > > + u32 bpr1_old, bpr1_new; > > + > > /* > > * Need to check that the SRE bit has actually been set. If > > * not, it means that SRE is disabled at EL2. We're going to > > @@ -499,9 +501,19 @@ static void gic_cpu_sys_reg_init(void) > > * Some firmwares hand over to the kernel with the BPR changed from > > * its reset value (and with a value large enough to prevent > > * any pre-emptive interrupts from working at all). Writing a zero > > - * to BPR restores is reset value. > > + * to BPR restores is reset value (though there seems to be some > > + * less than compliant implementations around, hence the warning...). > > */ > > + bpr1_old = gic_read_bpr1(); > > gic_write_bpr1(0); > > + isb(); > > + bpr1_new = gic_read_bpr1(); > > + > > + if (bpr1_new == 0) { > > + pr_warn("Failed to reset BPR1 (%d), restoring previous (%d)\n", > > + bpr1_new, bpr1_old); > > + gic_write_bpr1(bpr1_old); > > + } > > > > if (static_key_true(&supports_deactivate)) { > > /* EOI drops priority only (mode 1) */ > > > > FWIW, the equivalent implementation didn't help kvm-unit-tests. Still > timing-out. Even after attempting to set it back to old, which was 3, > when I read it again I still get zero, > > pre-read bpr1=3 > post-read bpr1=0 > new = 0, old = 3 > resetting to old and doing post-post-read... > post-post-read bpr1=0 > FAIL: gicv3: ipi: self: Timed-out (5s). ACKS: missing=1 extra=0 unexpected=0 fsck... Let's try something else. Obviously, Hardcoded values are bad. Untested. M. diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c index 3947095cc0a1..0d5596f5876c 100644 --- a/virt/kvm/arm/hyp/vgic-v3-sr.c +++ b/virt/kvm/arm/hyp/vgic-v3-sr.c @@ -160,6 +160,8 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu) dsb(st); cpu_if->vgic_vmcr = read_gicreg(ICH_VMCR_EL2); + if (unlikely(!((cpu_if->vgic_vmcr >> 18) & 7))) + cpu_if->vgic_vmcr |= 3 << 18; if (vcpu->arch.vgic_cpu.live_lrs) { int i; -- Jazz is not dead. It just smells funny.