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
drew
--
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
