| 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 | expand |
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
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
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?
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
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
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(-)