Re: [PATCH] KVM: PPC: Book3S HV: Check for updated HDSISR on P9 HDSI exception

[Paolo Bonzini]

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




signature.asc
Description: OpenPGP digital signature

Re: [PATCH] KVM: PPC: Book3S HV: Check for updated HDSISR on P9 HDSI exception

[David Gibson]

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

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH] KVM: PPC: Book3S HV: Check for updated HDSISR on P9 HDSI exception

[Paolo Bonzini]

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

Re: [PATCH] KVM: PPC: Book3S HV: Check for updated HDSISR on P9 HDSI exception

[David Gibson]

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 

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

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

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

[Michael Neuling]

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