v4.9-rc1 fails booting as a guest on ARM64 Cavium ThunderX

v4.9-rc1 fails booting as a guest on ARM64 Cavium ThunderX

[Auger Eric]

Hi,

I am not able to boot 4.9-rc1 as a guest on Cavium ThunderX (dt and acpi
mode). Bisecting the guest shows that the problem shows up at

91ef84428a86b75a52e15c6fe4f56b446ba75f93
irqchip/gic-v3: Reset BPR during initialization

If I remove the write to the ICC_BPR1_EL1 register on guest, the VM boots.

Investigating KVM code ...

Thanks

Eric

Re: v4.9-rc1 fails booting as a guest on ARM64 Cavium ThunderX

[Marc Zyngier]

+Robert

On 21/10/16 08:01, Auger Eric wrote:
> Hi,
> 
> I am not able to boot 4.9-rc1 as a guest on Cavium ThunderX (dt and acpi
> mode). Bisecting the guest shows that the problem shows up at
> 
> 91ef84428a86b75a52e15c6fe4f56b446ba75f93
> irqchip/gic-v3: Reset BPR during initialization
> 
> If I remove the write to the ICC_BPR1_EL1 register on guest, the VM boots.

That's very odd. A ICC_BPR1_EL1 access when HCR_EL2.IMO is set only
affects ICH_VMCR_EL2.VBPR1. It is not trapped, since we don't set
ICH_HCR_EL2.TALL1. It is a very boring sysreg!

So from a pure architectural point of view, I don't see how this can
fail. I've just run the same configuration on my Freescale board (GICv3
as well), and can't see any issue at all.

> Investigating KVM code ...

What is the failure syndrome? Do you see it crashing? Locking up? What
is the PC at that stage?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: v4.9-rc1 fails booting as a guest on ARM64 Cavium ThunderX

[Auger Eric]

Hi Marc,

On 21/10/2016 10:45, Marc Zyngier wrote:
> +Robert
> 
> On 21/10/16 08:01, Auger Eric wrote:
>> Hi,
>>
>> I am not able to boot 4.9-rc1 as a guest on Cavium ThunderX (dt and acpi
>> mode). Bisecting the guest shows that the problem shows up at
>>
>> 91ef84428a86b75a52e15c6fe4f56b446ba75f93
>> irqchip/gic-v3: Reset BPR during initialization
>>
>> If I remove the write to the ICC_BPR1_EL1 register on guest, the VM boots.
> 
> That's very odd. A ICC_BPR1_EL1 access when HCR_EL2.IMO is set only
> affects ICH_VMCR_EL2.VBPR1. It is not trapped, since we don't set
> ICH_HCR_EL2.TALL1. It is a very boring sysreg!
> 
> So from a pure architectural point of view, I don't see how this can
> fail. I've just run the same configuration on my Freescale board (GICv3
> as well), and can't see any issue at all.
> 
>> Investigating KVM code ...
> 
> What is the failure syndrome? Do you see it crashing? Locking up? What
> is the PC at that stage?
No guest crash. the guest just locks up. No traces output.

Thanks

Eric
> 
> Thanks,
> 
> 	M.
> 

Re: v4.9-rc1 fails booting as a guest on ARM64 Cavium ThunderX

[Marc Zyngier]

On 21/10/16 10:05, Auger Eric wrote:
> Hi Marc,
> 
> On 21/10/2016 10:45, Marc Zyngier wrote:
>> +Robert
>>
>> On 21/10/16 08:01, Auger Eric wrote:
>>> Hi,
>>>
>>> I am not able to boot 4.9-rc1 as a guest on Cavium ThunderX (dt and acpi
>>> mode). Bisecting the guest shows that the problem shows up at
>>>
>>> 91ef84428a86b75a52e15c6fe4f56b446ba75f93
>>> irqchip/gic-v3: Reset BPR during initialization
>>>
>>> If I remove the write to the ICC_BPR1_EL1 register on guest, the VM boots.
>>
>> That's very odd. A ICC_BPR1_EL1 access when HCR_EL2.IMO is set only
>> affects ICH_VMCR_EL2.VBPR1. It is not trapped, since we don't set
>> ICH_HCR_EL2.TALL1. It is a very boring sysreg!
>>
>> So from a pure architectural point of view, I don't see how this can
>> fail. I've just run the same configuration on my Freescale board (GICv3
>> as well), and can't see any issue at all.
>>
>>> Investigating KVM code ...
>>
>> What is the failure syndrome? Do you see it crashing? Locking up? What
>> is the PC at that stage?
> No guest crash. the guest just locks up. No traces output.

But you're able to kill the guest, right, and the CPU is not going to
lalaland. We should be able to put a breakpoint on this instruction
using qemu + GDB, and step it to find out what's happening. Or even
execute the instruction in isolation with a bunch of printks in the guest.

	M.
-- 
Jazz is not dead. It just smells funny...

Re: v4.9-rc1 fails booting as a guest on ARM64 Cavium ThunderX

[Auger Eric]

Hi Marc,
On 21/10/2016 11:40, Marc Zyngier wrote:
> On 21/10/16 10:05, Auger Eric wrote:
>> Hi Marc,
>>
>> On 21/10/2016 10:45, Marc Zyngier wrote:
>>> +Robert
>>>
>>> On 21/10/16 08:01, Auger Eric wrote:
>>>> Hi,
>>>>
>>>> I am not able to boot 4.9-rc1 as a guest on Cavium ThunderX (dt and acpi
>>>> mode). Bisecting the guest shows that the problem shows up at
>>>>
>>>> 91ef84428a86b75a52e15c6fe4f56b446ba75f93
>>>> irqchip/gic-v3: Reset BPR during initialization
>>>>
>>>> If I remove the write to the ICC_BPR1_EL1 register on guest, the VM boots.
>>>
>>> That's very odd. A ICC_BPR1_EL1 access when HCR_EL2.IMO is set only
>>> affects ICH_VMCR_EL2.VBPR1. It is not trapped, since we don't set
>>> ICH_HCR_EL2.TALL1. It is a very boring sysreg!
>>>
>>> So from a pure architectural point of view, I don't see how this can
>>> fail. I've just run the same configuration on my Freescale board (GICv3
>>> as well), and can't see any issue at all.
>>>
>>>> Investigating KVM code ...
>>>
>>> What is the failure syndrome? Do you see it crashing? Locking up? What
>>> is the PC at that stage?
>> No guest crash. the guest just locks up. No traces output.
> 
> But you're able to kill the guest, right,
Yes I am
 and the CPU is not going to
> lalaland. We should be able to put a breakpoint on this instruction
> using qemu + GDB, and step it to find out what's happening. Or even
> execute the instruction in isolation with a bunch of printks in the guest.
Yep I will investigate this afternoon.

Thanks

Eric
> 
> 	M.
> 

Re: v4.9-rc1 fails booting as a guest on ARM64 Cavium ThunderX

[Daniel Thompson]

On 21/10/16 09:45, Marc Zyngier wrote:
> +Robert
>
> On 21/10/16 08:01, Auger Eric wrote:
>> Hi,
>>
>> I am not able to boot 4.9-rc1 as a guest on Cavium ThunderX (dt and acpi
>> mode). Bisecting the guest shows that the problem shows up at
>>
>> 91ef84428a86b75a52e15c6fe4f56b446ba75f93
>> irqchip/gic-v3: Reset BPR during initialization
>>
>> If I remove the write to the ICC_BPR1_EL1 register on guest, the VM boots.
>
> That's very odd. A ICC_BPR1_EL1 access when HCR_EL2.IMO is set only
> affects ICH_VMCR_EL2.VBPR1. It is not trapped, since we don't set
> ICH_HCR_EL2.TALL1. It is a very boring sysreg!
>
> So from a pure architectural point of view, I don't see how this can
> fail. I've just run the same configuration on my Freescale board (GICv3
> as well), and can't see any issue at all.

Wow. Of all the patches I written for arm64 this was not the one I 
regarded as especially risky [at least when compared to the others ;-)].

Of course it does rely on writing a zero being "an attempt to program 
the binary point field to a value less than the reset value" and "sets 
the field to the reset value." and from a silicon validation point of 
view that might be regarded as a interesting corner case...


Daniel.

Re: v4.9-rc1 fails booting as a guest on ARM64 Cavium ThunderX

[Andrew Jones]

[-- Attachment #1: Type: text/plain, Size: 2570 bytes --]

On Fri, Oct 21, 2016 at 11:46:04AM +0200, Auger Eric wrote:
> Hi Marc,
> On 21/10/2016 11:40, Marc Zyngier wrote:
> > On 21/10/16 10:05, Auger Eric wrote:
> >> Hi Marc,
> >>
> >> On 21/10/2016 10:45, Marc Zyngier wrote:
> >>> +Robert
> >>>
> >>> On 21/10/16 08:01, Auger Eric wrote:
> >>>> Hi,
> >>>>
> >>>> I am not able to boot 4.9-rc1 as a guest on Cavium ThunderX (dt and acpi
> >>>> mode). Bisecting the guest shows that the problem shows up at
> >>>>
> >>>> 91ef84428a86b75a52e15c6fe4f56b446ba75f93
> >>>> irqchip/gic-v3: Reset BPR during initialization
> >>>>
> >>>> If I remove the write to the ICC_BPR1_EL1 register on guest, the VM boots.
> >>>
> >>> That's very odd. A ICC_BPR1_EL1 access when HCR_EL2.IMO is set only
> >>> affects ICH_VMCR_EL2.VBPR1. It is not trapped, since we don't set
> >>> ICH_HCR_EL2.TALL1. It is a very boring sysreg!
> >>>
> >>> So from a pure architectural point of view, I don't see how this can
> >>> fail. I've just run the same configuration on my Freescale board (GICv3
> >>> as well), and can't see any issue at all.
> >>>
> >>>> Investigating KVM code ...
> >>>
> >>> What is the failure syndrome? Do you see it crashing? Locking up? What
> >>> is the PC at that stage?
> >> No guest crash. the guest just locks up. No traces output.
> > 
> > But you're able to kill the guest, right,
> Yes I am
>  and the CPU is not going to
> > lalaland. We should be able to put a breakpoint on this instruction
> > using qemu + GDB, and step it to find out what's happening. Or even
> > execute the instruction in isolation with a bunch of printks in the guest.
> Yep I will investigate this afternoon.
>

You might be able to debug faster with kvm-unit-tests. The attached patch
applies to the arm/gic branch of my repo,

 https://github.com/rhdrjones/kvm-unit-tests/commits/arm/gic

and reproduces the issue. Before applying the attached patch, running

 $ arm/run arm/gic.flat

passes.

 PASS: gicv3: ipi: self: Completed in 100 ms

After applying the patch it times-out

 FAIL: gicv3: ipi: self: Timed-out (5s). ACKS: missing=1 extra=0 unexpected=0

Using the monitor and stopping/starting the vcpu to see what it's doing I
confirmed that we're just spinning in udelay waiting for the interrupt.

So it appears setting this register to zero disables the vcpu's ability
to receive interrupts?

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.

Thanks,
drew

[-- Attachment #2: 0001-arm64-gic-write-bpr1.patch --]
[-- Type: text/plain, Size: 1401 bytes --]

>From cd8989fa11255d0bf21d46051a65c3b073a182e6 Mon Sep 17 00:00:00 2001
From: Andrew Jones <drjones@redhat.com>
Date: Fri, 21 Oct 2016 13:22:38 +0200
Subject: [kvm-unit-tests PATCH] arm64: gic: write bpr1

---
 lib/arm/gic.c              | 1 +
 lib/arm64/asm/arch_gicv3.h | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/lib/arm/gic.c b/lib/arm/gic.c
index bb62407f7286..c44f7614be4d 100644
--- a/lib/arm/gic.c
+++ b/lib/arm/gic.c
@@ -137,6 +137,7 @@ void gicv3_enable_defaults(void)
 	gicv3_redist_wait_for_rwp();
 
 	gicv3_write_pmr(0xf0);
+	gicv3_write_bpr1(0);
 	gicv3_write_ctlr(ICC_CTLR_EL1_EOImode_drop_dir);
 	gicv3_write_grpen1(1);
 }
diff --git a/lib/arm64/asm/arch_gicv3.h b/lib/arm64/asm/arch_gicv3.h
index eff2efdfe2d4..cd9d8c95ef0d 100644
--- a/lib/arm64/asm/arch_gicv3.h
+++ b/lib/arm64/asm/arch_gicv3.h
@@ -18,6 +18,7 @@
 #define ICC_CTLR_EL1			sys_reg(3, 0, 12, 12, 4)
 #define ICC_SRE_EL1			sys_reg(3, 0, 12, 12, 5)
 #define ICC_GRPEN1_EL1			sys_reg(3, 0, 12, 12, 7)
+#define ICC_BPR1_EL1			sys_reg(3, 0, 12, 12, 3)
 
 #define ICC_SRE_EL2			sys_reg(3, 4, 12, 9, 5)
 
@@ -162,6 +163,11 @@ static inline void gicv3_write_sre(u32 val)
 	isb();
 }
 
+static inline void gicv3_write_bpr1(u32 val)
+{
+	asm volatile("msr_s " __stringify(ICC_BPR1_EL1) ", %0" : : "r" (val));
+}
+
 #define gicv3_read_typer(c)		readq(c)
 #define gicv3_write_irouter(v, c)	writeq(v, c)
 
-- 
2.7.4

Re: v4.9-rc1 fails booting as a guest on ARM64 Cavium ThunderX

[Peter Maydell]

On 21 October 2016 at 12:49, Andrew Jones <drjones@redhat.com> 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...

thanks
-- PMM

Re: v4.9-rc1 fails booting as a guest on ARM64 Cavium ThunderX

[Andrew Jones]

On Fri, Oct 21, 2016 at 12:57:37PM +0100, Peter Maydell wrote:
> On 21 October 2016 at 12:49, Andrew Jones <drjones@redhat.com> 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

Thanks,
drew

Re: v4.9-rc1 fails booting as a guest on ARM64 Cavium ThunderX

[Marc Zyngier]

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@redhat.com> 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) */

-- 
Jazz is not dead. It just smells funny...

Re: v4.9-rc1 fails booting as a guest on ARM64 Cavium ThunderX

[Peter Maydell]

On 21 October 2016 at 13:52, Marc Zyngier <marc.zyngier@arm.com> wrote:
> 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);


...a write function that calls read_sysreg() and returns a value?

thanks
-- PMM

Re: v4.9-rc1 fails booting as a guest on ARM64 Cavium ThunderX

[Andrew Jones]

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@redhat.com> 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 

Re: v4.9-rc1 fails booting as a guest on ARM64 Cavium ThunderX

[Marc Zyngier]

On Fri, 21 Oct 2016 13:58:47 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 21 October 2016 at 13:52, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > 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);  
> 
> 
> ...a write function that calls read_sysreg() and returns a value?

As you can tell, I haven't even compiled the 32bit version... :-/

	M.
-- 
Jazz is not dead. It just smells funny.

Re: v4.9-rc1 fails booting as a guest on ARM64 Cavium ThunderX

[Marc Zyngier]

On Fri, 21 Oct 2016 15:06:39 +0200
Andrew Jones <drjones@redhat.com> 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@redhat.com> 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.

Re: v4.9-rc1 fails booting as a guest on ARM64 Cavium ThunderX

[Robert Richter]

On 21.10.16 13:52:43, 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@redhat.com> 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...

Marc, we are looking into this.

-Robert

Re: v4.9-rc1 fails booting as a guest on ARM64 Cavium ThunderX

[Robert Richter]

On 21.10.16 13:52:43, 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@redhat.com> 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...

"The minimum binary value programmable is BP_MIN. Attempt to
set it to a lower value automatically sets it to the minimum value
BP_MIN."

The min value is 3 in this level, thus it should return 3 for [2:0] of
BPR1.

I am going to reproduce this.

-Robert

Re: v4.9-rc1 fails booting as a guest on ARM64 Cavium ThunderX

[Marc Zyngier]

On 26/10/16 08:18, Robert Richter wrote:
> On 21.10.16 13:52:43, 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@redhat.com> 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...
> 
> "The minimum binary value programmable is BP_MIN. Attempt to
> set it to a lower value automatically sets it to the minimum value
> BP_MIN."
> 
> The min value is 3 in this level, thus it should return 3 for [2:0] of
> BPR1.

Exactly (though there is a number of other rules such as taking
ICC_CTLR_EL1.CBPR into account). And all this should reflect in
VMCR_EL2.VBPR1.

> I am going to reproduce this.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: v4.9-rc1 fails booting as a guest on ARM64 Cavium ThunderX

[Robert Richter]

Eric,

On 21.10.16 11:05:01, Auger Eric wrote:
> On 21/10/2016 10:45, Marc Zyngier wrote:

> > What is the failure syndrome? Do you see it crashing? Locking up? What
> > is the PC at that stage?
> No guest crash. the guest just locks up. No traces output.

I am trying to reproduce this but could boot a 4.9-rc3 kernel as
guest. Are there any special settings? Here my command line:

 qemu-system-aarch64 -name qemu.test -enable-kvm -smp 8 -m 16G -cpu host -nographic -kernel /boot/vmlinuz-4.9.0-rc3.0.vanilla10-00002-gcfd8c2cee6ac -initrd /boot/initrd.img-4.9.0-rc3.0.vanilla10-00002-gcfd8c2cee6ac -M virt,gic_version=3 -hdb /dev/sdb -append "root=/dev/vda3"

Thanks,

-Robert

Re: v4.9-rc1 fails booting as a guest on ARM64 Cavium ThunderX

[Auger Eric]

Hi Robert,

On 03/11/2016 19:01, Robert Richter wrote:
> Eric,
> 
> On 21.10.16 11:05:01, Auger Eric wrote:
>> On 21/10/2016 10:45, Marc Zyngier wrote:
> 
>>> What is the failure syndrome? Do you see it crashing? Locking up? What
>>> is the PC at that stage?
>> No guest crash. the guest just locks up. No traces output.
> 
> I am trying to reproduce this but could boot a 4.9-rc3 kernel as
> guest. Are there any special settings? Here my command line:
> 
>  qemu-system-aarch64 -name qemu.test -enable-kvm -smp 8 -m 16G -cpu host -nographic -kernel /boot/vmlinuz-4.9.0-rc3.0.vanilla10-00002-gcfd8c2cee6ac -initrd /boot/initrd.img-4.9.0-rc3.0.vanilla10-00002-gcfd8c2cee6ac -M virt,gic_version=3 -hdb /dev/sdb -append "root=/dev/vda3"
> 
> Thanks,

no, nothing special. I checked again with a 4.9-rc3 host and guest and
the guest fails booting except if I comment the gic_write_bpr1(0) call.

Thanks

Eric

> 
> -Robert
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>