[Acked] [SRU][Xenial][PATCH 1/1] KVM: PPC: Book3S: Treat VTB as a per-subcore register, not per-thread
Andy Whitcroft
apw at canonical.com
Tue Nov 14 15:16:16 UTC 2017
On Fri, Nov 03, 2017 at 01:05:44PM -0400, Joseph Salisbury wrote:
> From: Paul Mackerras <paulus at ozlabs.org>
>
> BugLink: http://bugs.launchpad.net/bugs/1727331
>
> POWER8 has one virtual timebase (VTB) register per subcore, not one
> per CPU thread. The HV KVM code currently treats VTB as a per-thread
> register, which can lead to spurious soft lockup messages from guests
> which use the VTB as the time source for the soft lockup detector.
> (CPUs before POWER8 did not have the VTB register.)
>
> For HV KVM, this fixes the problem by making only the primary thread
> in each virtual core save and restore the VTB value. With this,
> the VTB state becomes part of the kvmppc_vcore structure. This
> also means that "piggybacking" of multiple virtual cores onto one
> subcore is not possible on POWER8, because then the virtual cores
> would share a single VTB register.
>
> PR KVM emulates a VTB register, which is per-vcpu because PR KVM
> has no notion of CPU threads or SMT. For PR KVM we move the VTB
> state into the kvmppc_vcpu_book3s struct.
>
> Cc: stable at vger.kernel.org # v3.14+
> Reported-by: Thomas Huth <thuth at redhat.com>
> Tested-by: Thomas Huth <thuth at redhat.com>
> (backported from commit 88b02cf97bb7e742db3e31671d54177e3e19fd89)
> Signed-off-by: Paul Mackerras <paulus at ozlabs.org>
> Signed-off-by: Joseph Salisbury <joseph.salisbury at canonical.com>
> ---
> arch/powerpc/include/asm/kvm_book3s.h | 1 +
> arch/powerpc/include/asm/kvm_host.h | 2 +-
> arch/powerpc/kernel/asm-offsets.c | 2 +-
> arch/powerpc/kvm/book3s.c | 6 ------
> arch/powerpc/kvm/book3s_emulate.c | 2 +-
> arch/powerpc/kvm/book3s_hv.c | 14 +++++++++++---
> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 14 +++++++-------
> arch/powerpc/kvm/book3s_pr.c | 8 +++++++-
> 8 files changed, 29 insertions(+), 20 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
> index 9fac01c..299737b 100644
> --- a/arch/powerpc/include/asm/kvm_book3s.h
> +++ b/arch/powerpc/include/asm/kvm_book3s.h
> @@ -83,6 +83,7 @@ struct kvmppc_vcpu_book3s {
> u64 sdr1;
> u64 hior;
> u64 msr_mask;
> + u64 vtb;
> #ifdef CONFIG_PPC_BOOK3S_32
> u32 vsid_pool[VSID_POOL_SIZE];
> u32 vsid_next;
> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
> index a92d95a..0d95aae 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -297,6 +297,7 @@ struct kvmppc_vcore {
> u32 arch_compat;
> ulong pcr;
> ulong dpdes; /* doorbell state (POWER8) */
> + ulong vtb; /* virtual timebase */
> ulong conferring_threads;
> };
>
> @@ -473,7 +474,6 @@ struct kvm_vcpu_arch {
> ulong purr;
> ulong spurr;
> ulong ic;
> - ulong vtb;
> ulong dscr;
> ulong amr;
> ulong uamor;
> diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
> index 40da691..2692fa6 100644
> --- a/arch/powerpc/kernel/asm-offsets.c
> +++ b/arch/powerpc/kernel/asm-offsets.c
> @@ -519,7 +519,6 @@ int main(void)
> DEFINE(VCPU_PURR, offsetof(struct kvm_vcpu, arch.purr));
> DEFINE(VCPU_SPURR, offsetof(struct kvm_vcpu, arch.spurr));
> DEFINE(VCPU_IC, offsetof(struct kvm_vcpu, arch.ic));
> - DEFINE(VCPU_VTB, offsetof(struct kvm_vcpu, arch.vtb));
> DEFINE(VCPU_DSCR, offsetof(struct kvm_vcpu, arch.dscr));
> DEFINE(VCPU_AMR, offsetof(struct kvm_vcpu, arch.amr));
> DEFINE(VCPU_UAMOR, offsetof(struct kvm_vcpu, arch.uamor));
> @@ -572,6 +571,7 @@ int main(void)
> DEFINE(VCORE_LPCR, offsetof(struct kvmppc_vcore, lpcr));
> DEFINE(VCORE_PCR, offsetof(struct kvmppc_vcore, pcr));
> DEFINE(VCORE_DPDES, offsetof(struct kvmppc_vcore, dpdes));
> + DEFINE(VCORE_VTB, offsetof(struct kvmppc_vcore, vtb));
> DEFINE(VCPU_SLB_E, offsetof(struct kvmppc_slb, orige));
> DEFINE(VCPU_SLB_V, offsetof(struct kvmppc_slb, origv));
> DEFINE(VCPU_SLB_SIZE, sizeof(struct kvmppc_slb));
> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> index 099c79d..a86fad3 100644
> --- a/arch/powerpc/kvm/book3s.c
> +++ b/arch/powerpc/kvm/book3s.c
> @@ -591,9 +591,6 @@ int kvmppc_get_one_reg(struct kvm_vcpu *vcpu, u64 id,
> case KVM_REG_PPC_BESCR:
> *val = get_reg_val(id, vcpu->arch.bescr);
> break;
> - case KVM_REG_PPC_VTB:
> - *val = get_reg_val(id, vcpu->arch.vtb);
> - break;
> case KVM_REG_PPC_IC:
> *val = get_reg_val(id, vcpu->arch.ic);
> break;
> @@ -665,9 +662,6 @@ int kvmppc_set_one_reg(struct kvm_vcpu *vcpu, u64 id,
> case KVM_REG_PPC_BESCR:
> vcpu->arch.bescr = set_reg_val(id, *val);
> break;
> - case KVM_REG_PPC_VTB:
> - vcpu->arch.vtb = set_reg_val(id, *val);
> - break;
> case KVM_REG_PPC_IC:
> vcpu->arch.ic = set_reg_val(id, *val);
> break;
> diff --git a/arch/powerpc/kvm/book3s_emulate.c b/arch/powerpc/kvm/book3s_emulate.c
> index 729f8fa..8359752 100644
> --- a/arch/powerpc/kvm/book3s_emulate.c
> +++ b/arch/powerpc/kvm/book3s_emulate.c
> @@ -580,7 +580,7 @@ int kvmppc_core_emulate_mfspr_pr(struct kvm_vcpu *vcpu, int sprn, ulong *spr_val
> *spr_val = vcpu->arch.spurr;
> break;
> case SPRN_VTB:
> - *spr_val = vcpu->arch.vtb;
> + *spr_val = to_book3s(vcpu)->vtb;
> break;
> case SPRN_IC:
> *spr_val = vcpu->arch.ic;
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index d4b50a9..bb8243e 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -1101,6 +1101,9 @@ static int kvmppc_get_one_reg_hv(struct kvm_vcpu *vcpu, u64 id,
> case KVM_REG_PPC_DPDES:
> *val = get_reg_val(id, vcpu->arch.vcore->dpdes);
> break;
> + case KVM_REG_PPC_VTB:
> + *val = get_reg_val(id, vcpu->arch.vcore->vtb);
> + break;
> case KVM_REG_PPC_DAWR:
> *val = get_reg_val(id, vcpu->arch.dawr);
> break;
> @@ -1296,6 +1299,9 @@ static int kvmppc_set_one_reg_hv(struct kvm_vcpu *vcpu, u64 id,
> case KVM_REG_PPC_DPDES:
> vcpu->arch.vcore->dpdes = set_reg_val(id, *val);
> break;
> + case KVM_REG_PPC_VTB:
> + vcpu->arch.vcore->vtb = set_reg_val(id, *val);
> + break;
> case KVM_REG_PPC_DAWR:
> vcpu->arch.dawr = set_reg_val(id, *val);
> break;
> @@ -2122,9 +2128,11 @@ static bool can_piggyback_subcore(struct kvmppc_vcore *pvc,
> pvc->lpcr != vc->lpcr)
> return false;
>
> - /* P8 guest with > 1 thread per core would see wrong TIR value */
> - if (cpu_has_feature(CPU_FTR_ARCH_207S) &&
> - (vc->num_threads > 1 || pvc->num_threads > 1))
> + /*
> + * P8 guests can't do piggybacking, because then the
> + * VTB would be shared between the vcpus.
> + */
> + if (cpu_has_feature(CPU_FTR_ARCH_207S))
> return false;
>
> n_thr = cip->subcore_threads[sub] + pvc->num_threads;
> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> index 5663af2..bb8c706 100644
> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> @@ -622,9 +622,11 @@ ALT_FTR_SECTION_END_IFSET(CPU_FTR_ARCH_207S)
> 38:
>
> BEGIN_FTR_SECTION
> - /* DPDES is shared between threads */
> + /* DPDES and VTB are shared between threads */
> ld r8, VCORE_DPDES(r5)
> + ld r7, VCORE_VTB(r5)
> mtspr SPRN_DPDES, r8
> + mtspr SPRN_VTB, r7
> END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
>
> /* Mark the subcore state as inside guest */
> @@ -794,10 +796,8 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_207S)
> mtspr SPRN_CIABR, r7
> mtspr SPRN_TAR, r8
> ld r5, VCPU_IC(r4)
> - ld r6, VCPU_VTB(r4)
> - mtspr SPRN_IC, r5
> - mtspr SPRN_VTB, r6
> ld r8, VCPU_EBBHR(r4)
> + mtspr SPRN_IC, r5
> mtspr SPRN_EBBHR, r8
> ld r5, VCPU_EBBRR(r4)
> ld r6, VCPU_BESCR(r4)
> @@ -1278,10 +1278,8 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_207S)
> stw r6, VCPU_PSPB(r9)
> std r7, VCPU_FSCR(r9)
> mfspr r5, SPRN_IC
> - mfspr r6, SPRN_VTB
> mfspr r7, SPRN_TAR
> std r5, VCPU_IC(r9)
> - std r6, VCPU_VTB(r9)
> std r7, VCPU_TAR(r9)
> mfspr r8, SPRN_EBBHR
> std r8, VCPU_EBBHR(r9)
> @@ -1518,9 +1516,11 @@ kvmhv_switch_to_host:
> isync
>
> BEGIN_FTR_SECTION
> - /* DPDES is shared between threads */
> + /* DPDES and VTB are shared between threads */
> mfspr r7, SPRN_DPDES
> + mfspr r8, SPRN_VTB
> std r7, VCORE_DPDES(r5)
> + std r8, VCORE_VTB(r5)
> /* clear DPDES so we don't get guest doorbells in the host */
> li r8, 0
> mtspr SPRN_DPDES, r8
> diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
> index 49f5dad..28fdc7f 100644
> --- a/arch/powerpc/kvm/book3s_pr.c
> +++ b/arch/powerpc/kvm/book3s_pr.c
> @@ -226,7 +226,7 @@ void kvmppc_copy_from_svcpu(struct kvm_vcpu *vcpu,
> */
> vcpu->arch.purr += get_tb() - vcpu->arch.entry_tb;
> vcpu->arch.spurr += get_tb() - vcpu->arch.entry_tb;
> - vcpu->arch.vtb += get_vtb() - vcpu->arch.entry_vtb;
> + to_book3s(vcpu)->vtb += get_vtb() - vcpu->arch.entry_vtb;
> if (cpu_has_feature(CPU_FTR_ARCH_207S))
> vcpu->arch.ic += mfspr(SPRN_IC) - vcpu->arch.entry_ic;
> svcpu->in_use = false;
> @@ -1325,6 +1325,9 @@ static int kvmppc_get_one_reg_pr(struct kvm_vcpu *vcpu, u64 id,
> case KVM_REG_PPC_HIOR:
> *val = get_reg_val(id, to_book3s(vcpu)->hior);
> break;
> + case KVM_REG_PPC_VTB:
> + *val = get_reg_val(id, to_book3s(vcpu)->vtb);
> + break;
> case KVM_REG_PPC_LPCR:
> case KVM_REG_PPC_LPCR_64:
> /*
> @@ -1361,6 +1364,9 @@ static int kvmppc_set_one_reg_pr(struct kvm_vcpu *vcpu, u64 id,
> to_book3s(vcpu)->hior = set_reg_val(id, *val);
> to_book3s(vcpu)->hior_explicit = true;
> break;
> + case KVM_REG_PPC_VTB:
> + to_book3s(vcpu)->vtb = set_reg_val(id, *val);
> + break;
> case KVM_REG_PPC_LPCR:
> case KVM_REG_PPC_LPCR_64:
> kvmppc_set_lpcr_pr(vcpu, set_reg_val(id, *val));
Looks reasonable to me.
Acked-by: Andy Whitcroft <apw at canonical.com>
-apw
More information about the kernel-team
mailing list