Messages in this thread | | | Date | Tue, 17 May 2022 16:56:32 +0800 | Subject | Re: [PATCH v11 14/16] KVM: x86/vmx: Flip Arch LBREn bit on guest state change | From | "Yang, Weijiang" <> |
| |
On 5/13/2022 12:02 PM, Yang, Weijiang wrote: > > On 5/12/2022 9:18 PM, Paolo Bonzini wrote: >> On 5/11/22 09:43, Yang, Weijiang wrote: >>>> Instead of using flip_arch_lbr_ctl, SMM should save the value of >>>> the MSR >>>> in kvm_x86_ops->enter_smm, and restore it in kvm_x86_ops->leave_smm >>>> (feel free to do it only if guest_cpuid_has(vcpu, X86_FEATURE_LM), >>>> i.e. >>>> the 32-bit case can be ignored). >>> In the case of migration in SMM, I assume kvm_x86_ops->enter_smm() >>> called in source side >>> >>> and kvm_x86_ops->leave_smm() is called at destination, then should the >>> saved LBREn be transferred >>> >>> to destination too? The destination can rely on the bit to defer >>> setting >>> LBREn bit in >> Hi, it's transferred automatically if the MSR is saved in the SMM save >> state area. Both enter_smm and leave_smm can access the save state >> area. >> >> The enter_smm callback is called after saving "normal" state, and it has >> to save the state + clear the bit; likewise, the leave_smm callback is >> called before saving "normal" state and will restore the old value of >> the MSR. > > Hi, I modified this patch to consolidate your suggestion above, see > below patch. > > I added more things to ease migration handling in SMM because: 1) qemu > checks > > LBREn before transfer Arch LBR MSRs. 2)Perf event is created when > LBREn is being > > set. Two things are not certain: 1) IA32_LBR_CTL doesn't have > corresponding slot in > > SMRAM,not sure if we need to rely on it to transfer the MSR. 2) I > chose 0x7f10 as > > the offset(CET takes 0x7f08) for storage, need you double check if > it's free or used. > > Thanks a lot!
Hi, Paolo,
I found there're some rebase conflicts between this series and your kvm queue branch
due to PEBS patches, I can re-post a new version based on your queue branch if necessary.
Waiting for your comments on this patch...
> > ==================================================================== > > From ceba1527fd87cdc789b38fce454058fca6582b0a Mon Sep 17 00:00:00 2001 > From: Yang Weijiang <weijiang.yang@intel.com> > Date: Thu, 5 Aug 2021 20:48:39 +0800 > Subject: [PATCH] KVM: x86/vmx: Flip Arch LBREn bit on guest state change > > Per spec:"IA32_LBR_CTL.LBREn is saved and cleared on #SMI, and restored > on RSM. On a warm reset, all LBR MSRs, including IA32_LBR_DEPTH, have > their > values preserved. However, IA32_LBR_CTL.LBREn is cleared to 0, disabling > LBRs." Given migration in SMM, use a reserved bit(63) of the MSR to > mirror > LBREn bit, it facilitates Arch LBR specific handling during live > migration > because user space will check LBREn to decide whether it's necessary to > migrate the Arch LBR related MSRs. When the mirrored bit and LBREn bit > are > both set, it means Arch LBR was active in SMM, so only create perf event > and defer the LBREn bit restoring to leave_smm callback. > Also clear LBREn at warm reset. > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > --- > arch/x86/kvm/vmx/pmu_intel.c | 16 +++++++++++++--- > arch/x86/kvm/vmx/vmx.c | 29 +++++++++++++++++++++++++++++ > arch/x86/kvm/vmx/vmx.h | 1 + > 3 files changed, 43 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c > index 038fdb788ccd..652601ad99ea 100644 > --- a/arch/x86/kvm/vmx/pmu_intel.c > +++ b/arch/x86/kvm/vmx/pmu_intel.c > @@ -373,6 +373,8 @@ static bool arch_lbr_depth_is_valid(struct > kvm_vcpu *vcpu, u64 depth) > return (depth == pmu->kvm_arch_lbr_depth); > } > > +#define ARCH_LBR_IN_SMM BIT(63) > + > static bool arch_lbr_ctl_is_valid(struct kvm_vcpu *vcpu, u64 ctl) > { > struct kvm_cpuid_entry2 *entry; > @@ -380,7 +382,7 @@ static bool arch_lbr_ctl_is_valid(struct kvm_vcpu > *vcpu, u64 ctl) > if (!kvm_cpu_cap_has(X86_FEATURE_ARCH_LBR)) > return false; > > - if (ctl & ~KVM_ARCH_LBR_CTL_MASK) > + if (ctl & ~(KVM_ARCH_LBR_CTL_MASK | ARCH_LBR_IN_SMM)) > goto warn; > > entry = kvm_find_cpuid_entry(vcpu, 0x1c, 0); > @@ -425,6 +427,10 @@ static int intel_pmu_get_msr(struct kvm_vcpu > *vcpu, struct msr_data *msr_info) > return 0; > case MSR_ARCH_LBR_CTL: > msr_info->data = vmcs_read64(GUEST_IA32_LBR_CTL); > + if (to_vmx(vcpu)->lbr_in_smm) { > + msr_info->data |= ARCH_LBR_CTL_LBREN; > + msr_info->data |= ARCH_LBR_IN_SMM; > + } > return 0; > default: > if ((pmc = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0)) || > @@ -501,11 +507,15 @@ static int intel_pmu_set_msr(struct kvm_vcpu > *vcpu, struct msr_data *msr_info) > if (!arch_lbr_ctl_is_valid(vcpu, data)) > break; > > - vmcs_write64(GUEST_IA32_LBR_CTL, data); > - > if (intel_pmu_lbr_is_enabled(vcpu) && !lbr_desc->event && > (data & ARCH_LBR_CTL_LBREN)) > intel_pmu_create_guest_lbr_event(vcpu); > + > + if (data & ARCH_LBR_IN_SMM) { > + data &= ~ARCH_LBR_CTL_LBREN; > + data &= ~ARCH_LBR_IN_SMM; > + } > + vmcs_write64(GUEST_IA32_LBR_CTL, data); > return 0; > default: > if ((pmc = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0)) || > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 6d6ee9cf82f5..f754b9400151 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -4543,6 +4543,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu > *vcpu, bool init_event) > > vmx->rmode.vm86_active = 0; > vmx->spec_ctrl = 0; > + vmx->lbr_in_smm = false; > > vmx->msr_ia32_umwait_control = 0; > > @@ -4593,6 +4594,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu > *vcpu, bool init_event) > if (!init_event) { > if (static_cpu_has(X86_FEATURE_ARCH_LBR)) > vmcs_write64(GUEST_IA32_LBR_CTL, 0); > + } else { > + flip_arch_lbr_ctl(vcpu, false); > } > } > > @@ -7695,6 +7698,8 @@ static int vmx_smi_allowed(struct kvm_vcpu > *vcpu, bool for_injection) > > static int vmx_enter_smm(struct kvm_vcpu *vcpu, char *smstate) > { > + struct lbr_desc *lbr_desc = vcpu_to_lbr_desc(vcpu); > + struct kvm_pmu *pmu = vcpu_to_pmu(vcpu); > struct vcpu_vmx *vmx = to_vmx(vcpu); > > vmx->nested.smm.guest_mode = is_guest_mode(vcpu); > @@ -7704,12 +7709,26 @@ static int vmx_enter_smm(struct kvm_vcpu > *vcpu, char *smstate) > vmx->nested.smm.vmxon = vmx->nested.vmxon; > vmx->nested.vmxon = false; > vmx_clear_hlt(vcpu); > + > + if (kvm_cpu_cap_has(X86_FEATURE_ARCH_LBR) && > + test_bit(INTEL_PMC_IDX_FIXED_VLBR, pmu->pmc_in_use) && > + lbr_desc->event && guest_cpuid_has(vcpu, X86_FEATURE_LM)) { > + u64 ctl = vmcs_read64(GUEST_IA32_LBR_CTL); > + > + put_smstate(u64, smstate, 0x7f10, ctl); > + vmcs_write64(GUEST_IA32_LBR_CTL, ctl & ~ARCH_LBR_CTL_LBREN); > + vmx->lbr_in_smm = true; > + } > + > return 0; > } > > static int vmx_leave_smm(struct kvm_vcpu *vcpu, const char *smstate) > { > + struct lbr_desc *lbr_desc = vcpu_to_lbr_desc(vcpu); > + struct kvm_pmu *pmu = vcpu_to_pmu(vcpu); > struct vcpu_vmx *vmx = to_vmx(vcpu); > + > int ret; > > if (vmx->nested.smm.vmxon) { > @@ -7725,6 +7744,16 @@ static int vmx_leave_smm(struct kvm_vcpu *vcpu, > const char *smstate) > vmx->nested.nested_run_pending = 1; > vmx->nested.smm.guest_mode = false; > } > + > + if (kvm_cpu_cap_has(X86_FEATURE_ARCH_LBR) && > + test_bit(INTEL_PMC_IDX_FIXED_VLBR, pmu->pmc_in_use) && > + lbr_desc->event && guest_cpuid_has(vcpu, X86_FEATURE_LM)) { > + u64 ctl = GET_SMSTATE(u64, smstate, 0x7f10); > + > + vmcs_write64(GUEST_IA32_LBR_CTL, ctl | ARCH_LBR_CTL_LBREN); > + vmx->lbr_in_smm = false; > + } > + > return 0; > } > > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > index b98c7e96697a..a227fe8bf288 100644 > --- a/arch/x86/kvm/vmx/vmx.h > +++ b/arch/x86/kvm/vmx/vmx.h > @@ -351,6 +351,7 @@ struct vcpu_vmx { > > struct pt_desc pt_desc; > struct lbr_desc lbr_desc; > + bool lbr_in_smm; > > /* Save desired MSR intercept (read: pass-through) state */ > #define MAX_POSSIBLE_PASSTHROUGH_MSRS 15
| |