KVM: PPC: Book3S HV: Check for updated HDSISR on P9 HDSI exception

Message ID 20170915052614.8231-1-mikey@neuling.org
State Accepted
Headers show
Series
  • KVM: PPC: Book3S HV: Check for updated HDSISR on P9 HDSI exception
Related show

Commit Message

Michael Neuling Sept. 15, 2017, 5:26 a.m.
On POWER9 DD2.1 and below, sometimes on a Hypervisor Data Storage
Interrupt (HDSI) the HDSISR is not be updated at all.

To work around this we put a canary value into the HDSISR before
returning to a guest and then check for this canary when we take a
HDSI. If we find the canary on a HDSI, we know the hardware didn't
update the HDSISR. In this case we return to the guest to retake the
HDSI which should correctly update the HDSISR the second time HDSI
entry.

After talking to Paulus we've applied this workaround to all POWER9
CPUs. The workaround of returning to the guest shouldn't ever be
triggered on well behaving CPU. The extra instructions should have
negligible performance impact.

Signed-off-by: Michael Neuling <mikey@neuling.org>
---
 arch/powerpc/kvm/book3s_hv_rmhandlers.S | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Comments

David Gibson Sept. 20, 2017, 6:46 a.m. | #1
On Fri, Sep 15, 2017 at 03:26:14PM +1000, Michael Neuling wrote:
> On POWER9 DD2.1 and below, sometimes on a Hypervisor Data Storage
> Interrupt (HDSI) the HDSISR is not be updated at all.
> 
> To work around this we put a canary value into the HDSISR before
> returning to a guest and then check for this canary when we take a
> HDSI. If we find the canary on a HDSI, we know the hardware didn't
> update the HDSISR. In this case we return to the guest to retake the
> HDSI which should correctly update the HDSISR the second time HDSI
> entry.
> 
> After talking to Paulus we've applied this workaround to all POWER9
> CPUs. The workaround of returning to the guest shouldn't ever be
> triggered on well behaving CPU. The extra instructions should have
> negligible performance impact.
> 
> Signed-off-by: Michael Neuling <mikey@neuling.org>

Paolo,

Any chance you can take this directly into your staging tree.  This is
a workaround for a potentially serious hardware bug, and we want to
get it into the downstream release ASAP.  However, Paulus is away for
a couple of weeks, so we can't count on him staging it in the kvm-ppc
tree soon.

> ---
>  arch/powerpc/kvm/book3s_hv_rmhandlers.S | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> index 663a4a861e..70dca60569 100644
> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> @@ -1118,6 +1118,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>  BEGIN_FTR_SECTION
>  	mtspr	SPRN_PPR, r0
>  END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> +
> +/* Move canary into DSISR to check for later */
> +BEGIN_FTR_SECTION
> +	li	r0, 0x7fff
> +	mtspr	SPRN_HDSISR, r0
> +END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
> +
>  	ld	r0, VCPU_GPR(R0)(r4)
>  	ld	r4, VCPU_GPR(R4)(r4)
>  
> @@ -1947,9 +1954,14 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_RADIX)
>  kvmppc_hdsi:
>  	ld	r3, VCPU_KVM(r9)
>  	lbz	r0, KVM_RADIX(r3)
> -	cmpwi	r0, 0
>  	mfspr	r4, SPRN_HDAR
>  	mfspr	r6, SPRN_HDSISR
> +BEGIN_FTR_SECTION
> +	/* Look for DSISR canary. If we find it, retry instruction */
> +	cmpdi	r6, 0x7fff
> +	beq	6f
> +END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
> +	cmpwi	r0, 0
>  	bne	.Lradix_hdsi		/* on radix, just save DAR/DSISR/ASDR */
>  	/* HPTE not found fault or protection fault? */
>  	andis.	r0, r6, (DSISR_NOHPTE | DSISR_PROTFAULT)@h
Paolo Bonzini Sept. 22, 2017, 8:46 a.m. | #2
On 15/09/2017 07:26, Michael Neuling wrote:
> On POWER9 DD2.1 and below, sometimes on a Hypervisor Data Storage
> Interrupt (HDSI) the HDSISR is not be updated at all.
> 
> To work around this we put a canary value into the HDSISR before
> returning to a guest and then check for this canary when we take a
> HDSI. If we find the canary on a HDSI, we know the hardware didn't
> update the HDSISR. In this case we return to the guest to retake the
> HDSI which should correctly update the HDSISR the second time HDSI
> entry.
> 
> After talking to Paulus we've applied this workaround to all POWER9
> CPUs. The workaround of returning to the guest shouldn't ever be
> triggered on well behaving CPU. The extra instructions should have
> negligible performance impact.
> 
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> ---
>  arch/powerpc/kvm/book3s_hv_rmhandlers.S | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> index 663a4a861e..70dca60569 100644
> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> @@ -1118,6 +1118,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>  BEGIN_FTR_SECTION
>  	mtspr	SPRN_PPR, r0
>  END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> +
> +/* Move canary into DSISR to check for later */
> +BEGIN_FTR_SECTION
> +	li	r0, 0x7fff
> +	mtspr	SPRN_HDSISR, r0
> +END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
> +
>  	ld	r0, VCPU_GPR(R0)(r4)
>  	ld	r4, VCPU_GPR(R4)(r4)
>  
> @@ -1947,9 +1954,14 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_RADIX)
>  kvmppc_hdsi:
>  	ld	r3, VCPU_KVM(r9)
>  	lbz	r0, KVM_RADIX(r3)
> -	cmpwi	r0, 0
>  	mfspr	r4, SPRN_HDAR
>  	mfspr	r6, SPRN_HDSISR
> +BEGIN_FTR_SECTION
> +	/* Look for DSISR canary. If we find it, retry instruction */
> +	cmpdi	r6, 0x7fff
> +	beq	6f
> +END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
> +	cmpwi	r0, 0
>  	bne	.Lradix_hdsi		/* on radix, just save DAR/DSISR/ASDR */
>  	/* HPTE not found fault or protection fault? */
>  	andis.	r0, r6, (DSISR_NOHPTE | DSISR_PROTFAULT)@h
> 

Applied to kvm/master, thanks.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Gibson Sept. 22, 2017, 10:30 a.m. | #3
On Fri, Sep 22, 2017 at 10:46:36AM +0200, Paolo Bonzini wrote:
> On 15/09/2017 07:26, Michael Neuling wrote:
> > On POWER9 DD2.1 and below, sometimes on a Hypervisor Data Storage
> > Interrupt (HDSI) the HDSISR is not be updated at all.
> > 
> > To work around this we put a canary value into the HDSISR before
> > returning to a guest and then check for this canary when we take a
> > HDSI. If we find the canary on a HDSI, we know the hardware didn't
> > update the HDSISR. In this case we return to the guest to retake the
> > HDSI which should correctly update the HDSISR the second time HDSI
> > entry.
> > 
> > After talking to Paulus we've applied this workaround to all POWER9
> > CPUs. The workaround of returning to the guest shouldn't ever be
> > triggered on well behaving CPU. The extra instructions should have
> > negligible performance impact.
> > 
> > Signed-off-by: Michael Neuling <mikey@neuling.org>
> > ---
> >  arch/powerpc/kvm/book3s_hv_rmhandlers.S | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > index 663a4a861e..70dca60569 100644
> > --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > @@ -1118,6 +1118,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> >  BEGIN_FTR_SECTION
> >  	mtspr	SPRN_PPR, r0
> >  END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> > +
> > +/* Move canary into DSISR to check for later */
> > +BEGIN_FTR_SECTION
> > +	li	r0, 0x7fff
> > +	mtspr	SPRN_HDSISR, r0
> > +END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
> > +
> >  	ld	r0, VCPU_GPR(R0)(r4)
> >  	ld	r4, VCPU_GPR(R4)(r4)
> >  
> > @@ -1947,9 +1954,14 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_RADIX)
> >  kvmppc_hdsi:
> >  	ld	r3, VCPU_KVM(r9)
> >  	lbz	r0, KVM_RADIX(r3)
> > -	cmpwi	r0, 0
> >  	mfspr	r4, SPRN_HDAR
> >  	mfspr	r6, SPRN_HDSISR
> > +BEGIN_FTR_SECTION
> > +	/* Look for DSISR canary. If we find it, retry instruction */
> > +	cmpdi	r6, 0x7fff
> > +	beq	6f
> > +END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
> > +	cmpwi	r0, 0
> >  	bne	.Lradix_hdsi		/* on radix, just save DAR/DSISR/ASDR */
> >  	/* HPTE not found fault or protection fault? */
> >  	andis.	r0, r6, (DSISR_NOHPTE | DSISR_PROTFAULT)@h
> > 
> 
> Applied to kvm/master, thanks.

I'm not seeing it; have you pushed out the tree?
Paolo Bonzini Sept. 22, 2017, 10:32 a.m. | #4
On 22/09/2017 12:30, David Gibson wrote:
> On Fri, Sep 22, 2017 at 10:46:36AM +0200, Paolo Bonzini wrote:
>> On 15/09/2017 07:26, Michael Neuling wrote:
>>> On POWER9 DD2.1 and below, sometimes on a Hypervisor Data Storage
>>> Interrupt (HDSI) the HDSISR is not be updated at all.
>>>
>>> To work around this we put a canary value into the HDSISR before
>>> returning to a guest and then check for this canary when we take a
>>> HDSI. If we find the canary on a HDSI, we know the hardware didn't
>>> update the HDSISR. In this case we return to the guest to retake the
>>> HDSI which should correctly update the HDSISR the second time HDSI
>>> entry.
>>>
>>> After talking to Paulus we've applied this workaround to all POWER9
>>> CPUs. The workaround of returning to the guest shouldn't ever be
>>> triggered on well behaving CPU. The extra instructions should have
>>> negligible performance impact.
>>>
>>> Signed-off-by: Michael Neuling <mikey@neuling.org>
>>> ---
>>>  arch/powerpc/kvm/book3s_hv_rmhandlers.S | 14 +++++++++++++-
>>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>>> index 663a4a861e..70dca60569 100644
>>> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>>> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>>> @@ -1118,6 +1118,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>>>  BEGIN_FTR_SECTION
>>>  	mtspr	SPRN_PPR, r0
>>>  END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>>> +
>>> +/* Move canary into DSISR to check for later */
>>> +BEGIN_FTR_SECTION
>>> +	li	r0, 0x7fff
>>> +	mtspr	SPRN_HDSISR, r0
>>> +END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
>>> +
>>>  	ld	r0, VCPU_GPR(R0)(r4)
>>>  	ld	r4, VCPU_GPR(R4)(r4)
>>>  
>>> @@ -1947,9 +1954,14 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_RADIX)
>>>  kvmppc_hdsi:
>>>  	ld	r3, VCPU_KVM(r9)
>>>  	lbz	r0, KVM_RADIX(r3)
>>> -	cmpwi	r0, 0
>>>  	mfspr	r4, SPRN_HDAR
>>>  	mfspr	r6, SPRN_HDSISR
>>> +BEGIN_FTR_SECTION
>>> +	/* Look for DSISR canary. If we find it, retry instruction */
>>> +	cmpdi	r6, 0x7fff
>>> +	beq	6f
>>> +END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
>>> +	cmpwi	r0, 0
>>>  	bne	.Lradix_hdsi		/* on radix, just save DAR/DSISR/ASDR */
>>>  	/* HPTE not found fault or protection fault? */
>>>  	andis.	r0, r6, (DSISR_NOHPTE | DSISR_PROTFAULT)@h
>>>
>>
>> Applied to kvm/master, thanks.
> 
> I'm not seeing it; have you pushed out the tree?

No, not yet (doing some tests).

Paolo

Patch

diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
index 663a4a861e..70dca60569 100644
--- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
+++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
@@ -1118,6 +1118,13 @@  END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
 BEGIN_FTR_SECTION
 	mtspr	SPRN_PPR, r0
 END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
+
+/* Move canary into DSISR to check for later */
+BEGIN_FTR_SECTION
+	li	r0, 0x7fff
+	mtspr	SPRN_HDSISR, r0
+END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
+
 	ld	r0, VCPU_GPR(R0)(r4)
 	ld	r4, VCPU_GPR(R4)(r4)
 
@@ -1947,9 +1954,14 @@  END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_RADIX)
 kvmppc_hdsi:
 	ld	r3, VCPU_KVM(r9)
 	lbz	r0, KVM_RADIX(r3)
-	cmpwi	r0, 0
 	mfspr	r4, SPRN_HDAR
 	mfspr	r6, SPRN_HDSISR
+BEGIN_FTR_SECTION
+	/* Look for DSISR canary. If we find it, retry instruction */
+	cmpdi	r6, 0x7fff
+	beq	6f
+END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
+	cmpwi	r0, 0
 	bne	.Lradix_hdsi		/* on radix, just save DAR/DSISR/ASDR */
 	/* HPTE not found fault or protection fault? */
 	andis.	r0, r6, (DSISR_NOHPTE | DSISR_PROTFAULT)@h