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