On Fri, Oct 28, 2022 at 4:26 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Fri, Oct 28, 2022, Aaron Lewis wrote:
> > A stall in this situation could have an impact on live migration. So,
> > to avoid that disable the print if the write is initiated by the host.
>
> ...
>
> > Fixes: 5753785fa977 ("KVM: do not #GP on perf MSR writes when vPMU is disabled")
> > Signed-off-by: Aaron Lewis <aaronlewis@xxxxxxxxxx>
> > ---
> > arch/x86/kvm/x86.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 9cf1ba865562..a3b842467bd2 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -3778,7 +3778,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >
> > case MSR_K7_PERFCTR0 ... MSR_K7_PERFCTR3:
> > case MSR_P6_PERFCTR0 ... MSR_P6_PERFCTR1:
> > - pr = true;
> > + pr = !msr_info->host_initiated;
>
> Wasting guest cycles isn't any better, and there are plenty more MSRs that don't
> honor report_ignored_msrs, i.e. you'll be playing whack-a-mole.
>
> My first choice would be to delete all MSR printks, but since that's probably not
> an option...
>
> ---
> From: Sean Christopherson <seanjc@xxxxxxxxxx>
> Date: Fri, 28 Oct 2022 09:20:03 -0700
> Subject: [PATCH] KVM: x86: Gate all "unimplemented MSR" prints on
> report_ignored_msrs
>
> Add helpers to print unimplemented MSR accesses and condition all such
> prints on report_ignored_msrs, i.e. honor userspace's request to not
> print unimplemented MSRs. Even though vcpu_unimpl() is ratelimited,
> printing can still be problematic, e.g. if a print gets stalled when host
> userspace is writing MSRs during live migration, an effective stall can
> result in very noticeable disruption in the guest.
>
Yeah, I like this approach better.
> @@ -3778,16 +3775,13 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>
> case MSR_K7_PERFCTR0 ... MSR_K7_PERFCTR3:
> case MSR_P6_PERFCTR0 ... MSR_P6_PERFCTR1:
> - pr = true;
> - fallthrough;
> case MSR_K7_EVNTSEL0 ... MSR_K7_EVNTSEL3:
> case MSR_P6_EVNTSEL0 ... MSR_P6_EVNTSEL1:
> if (kvm_pmu_is_valid_msr(vcpu, msr))
> return kvm_pmu_set_msr(vcpu, msr_info);
>
> - if (pr || data != 0)
> - vcpu_unimpl(vcpu, "disabled perfctr wrmsr: "
> - "0x%x data 0x%llx\n", msr, data);
> + if (data)
> + kvm_pr_unimpl_wrmsr(vcpu, msr, data);
Any reason to keep the check for 'data' around? Now that it's
checking for 'report_ignored_msrs' maybe we don't need that check as
well. I'm not sure what the harm is in removing it, and with this
change we are additionally restricting pmu counter == 0 from printing.
If there's nothing special about it, let them both print.
> break;
> case MSR_K7_CLK_CTL:
> /*
> @@ -3814,9 +3808,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> /* Drop writes to this legacy MSR -- see rdmsr
> * counterpart for further detail.
> */
> - if (report_ignored_msrs)
> - vcpu_unimpl(vcpu, "ignored wrmsr: 0x%x data 0x%llx\n",
> - msr, data);
> + kvm_pr_unimpl_wrmsr(vcpu, msr, data);
> break;
> case MSR_AMD64_OSVW_ID_LENGTH:
> if (!guest_cpuid_has(vcpu, X86_FEATURE_OSVW))
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index 829d3134c1eb..4921a0774bf7 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -330,6 +330,18 @@ extern bool report_ignored_msrs;
>
