On Fri, 21 Oct 2016 15:06:39 +0200
Andrew Jones <drjones@xxxxxxxxxx> 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 <drjones@xxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
