[PATCH] KVM: x86: Fix a stall when KVM_SET_MSRS is called on the pmu counters

[PATCH] KVM: x86: Fix a stall when KVM_SET_MSRS is called on the pmu counters

[Aaron Lewis]

When the host initiates a wrmsr through KVM_SET_MSRS don't print an
error if the PMU is disabled, doing so can cause a noticeable stall
while printing the error message.

The profile below was taken while calling KVM_SET_MSRS on the pmu
counters while the PMU was disabled in KVM.  Even though the print is
rate limited it still manages to consume by far the majority of the
time.

-   99.75%     0.00%  [.] __ioctl
   - __ioctl
      - 99.74% entry_SYSCALL_64_after_hwframe
           do_syscall_64
           sys_ioctl
         - do_vfs_ioctl
            - 92.48% kvm_vcpu_ioctl
               - kvm_arch_vcpu_ioctl
                  - 85.12% kvm_set_msr_ignored_check
                       svm_set_msr
                       kvm_set_msr_common
                       printk
                       vprintk_func
                       vprintk_default
                       vprintk_emit
                       console_unlock
                       call_console_drivers
                       univ8250_console_write
                       serial8250_console_write
                       uart_console_write

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@google.com>
---
 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;
 		fallthrough;
 	case MSR_K7_EVNTSEL0 ... MSR_K7_EVNTSEL3:
 	case MSR_P6_EVNTSEL0 ... MSR_P6_EVNTSEL1:
-- 
2.38.1.273.g43a17bfeac-goog

Re: [PATCH] KVM: x86: Fix a stall when KVM_SET_MSRS is called on the pmu counters

[Sean Christopherson]

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@google.com>
> ---
>  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@google.com>
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.

E.g. the profile below was taken while calling KVM_SET_MSRS on the PMU
counters while the PMU was disabled in KVM.

  -   99.75%     0.00%  [.] __ioctl
   - __ioctl
      - 99.74% entry_SYSCALL_64_after_hwframe
           do_syscall_64
           sys_ioctl
         - do_vfs_ioctl
            - 92.48% kvm_vcpu_ioctl
               - kvm_arch_vcpu_ioctl
                  - 85.12% kvm_set_msr_ignored_check
                       svm_set_msr
                       kvm_set_msr_common
                       printk
                       vprintk_func
                       vprintk_default
                       vprintk_emit
                       console_unlock
                       call_console_drivers
                       univ8250_console_write
                       serial8250_console_write
                       uart_console_write

Reported-by: Aaron Lewis <aaronlewis@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/hyperv.c  | 10 ++++------
 arch/x86/kvm/svm/svm.c |  5 ++---
 arch/x86/kvm/vmx/vmx.c |  4 +---
 arch/x86/kvm/x86.c     | 18 +++++-------------
 arch/x86/kvm/x86.h     | 12 ++++++++++++
 5 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 0adf4a437e85..4aacd9bd1e3d 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -1404,8 +1404,7 @@ static int kvm_hv_set_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data,
 	case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER:
 		return syndbg_set_msr(vcpu, msr, data, host);
 	default:
-		vcpu_unimpl(vcpu, "Hyper-V unhandled wrmsr: 0x%x data 0x%llx\n",
-			    msr, data);
+		kvm_pr_unimpl_wrmsr(vcpu, msr, data);
 		return 1;
 	}
 	return 0;
@@ -1526,8 +1525,7 @@ static int kvm_hv_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host)
 			return 1;
 		break;
 	default:
-		vcpu_unimpl(vcpu, "Hyper-V unhandled wrmsr: 0x%x data 0x%llx\n",
-			    msr, data);
+		kvm_pr_unimpl_wrmsr(vcpu, msr, data);
 		return 1;
 	}
 
@@ -1579,7 +1577,7 @@ static int kvm_hv_get_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata,
 	case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER:
 		return syndbg_get_msr(vcpu, msr, pdata, host);
 	default:
-		vcpu_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr);
+		kvm_pr_unimpl_rdmsr(vcpu, msr);
 		return 1;
 	}
 
@@ -1644,7 +1642,7 @@ static int kvm_hv_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata,
 		data = APIC_BUS_FREQUENCY;
 		break;
 	default:
-		vcpu_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr);
+		kvm_pr_unimpl_rdmsr(vcpu, msr);
 		return 1;
 	}
 	*pdata = data;
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 58f0077d9357..432da7752c2f 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3010,8 +3010,7 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 		break;
 	case MSR_IA32_DEBUGCTLMSR:
 		if (!lbrv) {
-			vcpu_unimpl(vcpu, "%s: MSR_IA32_DEBUGCTL 0x%llx, nop\n",
-				    __func__, data);
+			kvm_pr_unimpl_wrmsr(vcpu, ecx, data);
 			break;
 		}
 		if (data & DEBUGCTL_RESERVED_BITS)
@@ -3040,7 +3039,7 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 	case MSR_VM_CR:
 		return svm_set_vm_cr(vcpu, data);
 	case MSR_VM_IGNNE:
-		vcpu_unimpl(vcpu, "unimplemented wrmsr: 0x%x data 0x%llx\n", ecx, data);
+		kvm_pr_unimpl_wrmsr(vcpu, ecx, data);
 		break;
 	case MSR_F10H_DECFG: {
 		struct kvm_msr_entry msr_entry;
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 9dba04b6b019..def6c1961355 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2105,9 +2105,7 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_IA32_DEBUGCTLMSR: {
 		u64 invalid = data & ~vcpu_supported_debugctl(vcpu);
 		if (invalid & (DEBUGCTLMSR_BTF|DEBUGCTLMSR_LBR)) {
-			if (report_ignored_msrs)
-				vcpu_unimpl(vcpu, "%s: BTF|LBR in IA32_DEBUGCTLMSR 0x%llx, nop\n",
-					    __func__, data);
+			kvm_pr_unimpl_wrmsr(vcpu, msr_index, data);
 			data &= ~(DEBUGCTLMSR_BTF|DEBUGCTLMSR_LBR);
 			invalid &= ~(DEBUGCTLMSR_BTF|DEBUGCTLMSR_LBR);
 		}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4bd5f8a751de..a2f0f7f2eb56 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3539,7 +3539,6 @@ static void record_steal_time(struct kvm_vcpu *vcpu)
 
 int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 {
-	bool pr = false;
 	u32 msr = msr_info->index;
 	u64 data = msr_info->data;
 
@@ -3590,15 +3589,13 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		if (data == BIT_ULL(18)) {
 			vcpu->arch.msr_hwcr = data;
 		} else if (data != 0) {
-			vcpu_unimpl(vcpu, "unimplemented HWCR wrmsr: 0x%llx\n",
-				    data);
+			kvm_pr_unimpl_wrmsr(vcpu, msr, data);
 			return 1;
 		}
 		break;
 	case MSR_FAM10H_MMIO_CONF_BASE:
 		if (data != 0) {
-			vcpu_unimpl(vcpu, "unimplemented MMIO_CONF_BASE wrmsr: "
-				    "0x%llx\n", data);
+			kvm_pr_unimpl_wrmsr(vcpu, msr, data);
 			return 1;
 		}
 		break;
@@ -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);
 		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;
 
 extern bool eager_page_split;
 
+static inline void kvm_pr_unimpl_wrmsr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
+{
+	if (report_ignored_msrs)
+		vcpu_unimpl(vcpu, "Unhandled WRMSR(0x%x) = 0x%llx\n", msr, data);
+}
+
+static inline void kvm_pr_unimpl_rdmsr(struct kvm_vcpu *vcpu, u32 msr)
+{
+	if (report_ignored_msrs)
+		vcpu_unimpl(vcpu, "Unhandled RDMSR(0x%x)\n", msr);
+}
+
 static inline u64 nsec_to_cycles(struct kvm_vcpu *vcpu, u64 nsec)
 {
 	return pvclock_scale_delta(nsec, vcpu->arch.virtual_tsc_mult,

base-commit: e18d6152ff0f41b7f01f9817372022df04e0d354
-- 

Re: [PATCH] KVM: x86: Fix a stall when KVM_SET_MSRS is called on the pmu counters

[Aaron Lewis]

On Fri, Oct 28, 2022 at 4:26 PM Sean Christopherson <seanjc@google.com> 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@google.com>
> > ---
> > 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@google.com>
> 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;
>

Re: [PATCH] KVM: x86: Fix a stall when KVM_SET_MSRS is called on the pmu counters

[Sean Christopherson]

On Fri, Oct 28, 2022, Aaron Lewis wrote:
> On Fri, Oct 28, 2022 at 4:26 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Fri, Oct 28, 2022, Aaron Lewis wrote:
> > @@ -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.

Checking 'dat' doesn't restrict counter 0, it skips printing if the guest (or host)
is writing '0', e.g. it would also skip the case you encountered where the host is
blindly "restoring" unused MSRs.

Or did I misunderstand your comment?

Re: [PATCH] KVM: x86: Fix a stall when KVM_SET_MSRS is called on the pmu counters

[Jim Mattson]

On Fri, Oct 28, 2022 at 2:51 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Fri, Oct 28, 2022, Aaron Lewis wrote:
> > On Fri, Oct 28, 2022 at 4:26 PM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Fri, Oct 28, 2022, Aaron Lewis wrote:
> > > @@ -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.
>
> Checking 'dat' doesn't restrict counter 0, it skips printing if the guest (or host)
> is writing '0', e.g. it would also skip the case you encountered where the host is
> blindly "restoring" unused MSRs.

The VMM is only blind because KVM_GET_MSR_INDEX_LIST poked it in the
eye. It would be nice to have an API that the VMM could query for the
list of supported MSRs.

> Or did I misunderstand your comment?

Re: [PATCH] KVM: x86: Fix a stall when KVM_SET_MSRS is called on the pmu counters

[Sean Christopherson]

On Fri, Oct 28, 2022, Jim Mattson wrote:
> On Fri, Oct 28, 2022 at 2:51 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Fri, Oct 28, 2022, Aaron Lewis wrote:
> > > On Fri, Oct 28, 2022 at 4:26 PM Sean Christopherson <seanjc@google.com> wrote:
> > > >
> > > > On Fri, Oct 28, 2022, Aaron Lewis wrote:
> > > > @@ -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.
> >
> > Checking 'dat' doesn't restrict counter 0, it skips printing if the guest (or host)
> > is writing '0', e.g. it would also skip the case you encountered where the host is
> > blindly "restoring" unused MSRs.
> 
> The VMM is only blind because KVM_GET_MSR_INDEX_LIST poked it in the
> eye. It would be nice to have an API that the VMM could query for the
> list of supported MSRs.

That should be a fairly easy bug fix, kvm_init_msr_list() can and should omit PMU
MSRs if enable_pmu==false.

Re: [PATCH] KVM: x86: Fix a stall when KVM_SET_MSRS is called on the pmu counters

[Paolo Bonzini]

On 10/29/22 00:43, Sean Christopherson wrote:
>>> Checking 'dat' doesn't restrict counter 0, it skips printing if the guest (or host)
>>> is writing '0', e.g. it would also skip the case you encountered where the host is
>>> blindly "restoring" unused MSRs.
>> The VMM is only blind because KVM_GET_MSR_INDEX_LIST poked it in the
>> eye. It would be nice to have an API that the VMM could query for the
>> list of supported MSRs.
> That should be a fairly easy bug fix, kvm_init_msr_list() can and should omit PMU
> MSRs if enable_pmu==false.

Aaron, are you going to send a patch for this?

Thanks,

Paolo

Re: [PATCH] KVM: x86: Fix a stall when KVM_SET_MSRS is called on the pmu counters

[Aaron Lewis]

On Wed, Nov 2, 2022 at 5:55 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 10/29/22 00:43, Sean Christopherson wrote:
> >>> Checking 'dat' doesn't restrict counter 0, it skips printing if the guest (or host)
> >>> is writing '0', e.g. it would also skip the case you encountered where the host is
> >>> blindly "restoring" unused MSRs.
> >> The VMM is only blind because KVM_GET_MSR_INDEX_LIST poked it in the
> >> eye. It would be nice to have an API that the VMM could query for the
> >> list of supported MSRs.
> > That should be a fairly easy bug fix, kvm_init_msr_list() can and should omit PMU
> > MSRs if enable_pmu==false.
>
> Aaron, are you going to send a patch for this?
>
> Thanks,
>
> Paolo
>

Yes, I can do that.

Re: [PATCH] KVM: x86: Fix a stall when KVM_SET_MSRS is called on the pmu counters

[Aaron Lewis]

On Fri, Oct 28, 2022 at 2:51 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Fri, Oct 28, 2022, Aaron Lewis wrote:
> > On Fri, Oct 28, 2022 at 4:26 PM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Fri, Oct 28, 2022, Aaron Lewis wrote:
> > > @@ -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.
>
> Checking 'dat' doesn't restrict counter 0, it skips printing if the guest (or host)
> is writing '0', e.g. it would also skip the case you encountered where the host is
> blindly "restoring" unused MSRs.
>
> Or did I misunderstand your comment?

That makes sense.

Reviewed-by: Aaron Lewis <aaronlewis@google.com>