[PATCH 0/2] KVM: vmx, pmu: respect KVM_GET_MSR_INDEX_LIST/KVM_SET_MSR contracts

[PATCH 0/2] KVM: vmx, pmu: respect KVM_GET_MSR_INDEX_LIST/KVM_SET_MSR contracts

[Paolo Bonzini]

Whenever an MSR is part of KVM_GET_MSR_INDEX_LIST, it has to be always
settable with KVM_SET_MSR.  Right now MSR_IA32_DS_AREA, MSR_ARCH_LBR_DEPTH
and MSR_ARCH_LBR_CTL are not fulfilling this, resulting in selftests
failures on <=Skylake processors.

Fix this, and in general allow host-initiated writes of a default
value for all PMU MSRs

Paolo Bonzini (2):
  KVM: vmx, pmu: accept 0 for absent MSRs when host-initiated
  KVM: x86: always allow host-initiated writes to PMU MSRs

 arch/x86/kvm/pmu.c           |  4 ++--
 arch/x86/kvm/pmu.h           |  4 ++--
 arch/x86/kvm/svm/pmu.c       |  2 +-
 arch/x86/kvm/vmx/pmu_intel.c | 42 +++++++++++++++++++++++++-----------
 arch/x86/kvm/x86.c           | 10 ++++-----
 5 files changed, 39 insertions(+), 23 deletions(-)

-- 
2.31.1

[PATCH 1/2] KVM: vmx, pmu: accept 0 for absent MSRs when host-initiated

[Paolo Bonzini]

Whenever an MSR is part of KVM_GET_MSR_INDEX_LIST, as is the case for
MSR_IA32_DS_AREA, MSR_ARCH_LBR_DEPTH or MSR_ARCH_LBR_CTL, it has to be
always settable with KVM_SET_MSR.  Accept a zero value for these MSRs
to obey the contract.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/vmx/pmu_intel.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index 3e04d0407605..66496cb41494 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -367,8 +367,9 @@ static bool arch_lbr_depth_is_valid(struct kvm_vcpu *vcpu, u64 depth)
 {
 	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
 
-	if (!kvm_cpu_cap_has(X86_FEATURE_ARCH_LBR))
-		return false;
+	if (!kvm_cpu_cap_has(X86_FEATURE_ARCH_LBR) ||
+	    !guest_cpuid_has(vcpu, X86_FEATURE_ARCH_LBR))
+		return depth == 0;
 
 	return (depth == pmu->kvm_arch_lbr_depth);
 }
@@ -378,7 +379,7 @@ static bool arch_lbr_ctl_is_valid(struct kvm_vcpu *vcpu, u64 ctl)
 	struct kvm_cpuid_entry2 *entry;
 
 	if (!kvm_cpu_cap_has(X86_FEATURE_ARCH_LBR))
-		return false;
+		return ctl == 0;
 
 	if (ctl & ~KVM_ARCH_LBR_CTL_MASK)
 		goto warn;
@@ -510,6 +511,8 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		}
 		break;
 	case MSR_IA32_DS_AREA:
+		if (msr_info->host_initiated && data && !guest_cpuid_has(vcpu, X86_FEATURE_DS))
+			return 1;
 		if (is_noncanonical_address(data, vcpu))
 			return 1;
 		pmu->ds_area = data;
@@ -525,7 +528,11 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_ARCH_LBR_DEPTH:
 		if (!arch_lbr_depth_is_valid(vcpu, data))
 			return 1;
+
 		lbr_desc->records.nr = data;
+		if (!guest_cpuid_has(vcpu, X86_FEATURE_ARCH_LBR))
+			return 0;
+
 		/*
 		 * Writing depth MSR from guest could either setting the
 		 * MSR or resetting the LBR records with the side-effect.
@@ -535,6 +542,8 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_ARCH_LBR_CTL:
 		if (!arch_lbr_ctl_is_valid(vcpu, data))
 			break;
+		if (!guest_cpuid_has(vcpu, X86_FEATURE_ARCH_LBR))
+			return 0;
 
 		vmcs_write64(GUEST_IA32_LBR_CTL, data);
 
-- 
2.31.1

[PATCH 2/2] KVM: x86: always allow host-initiated writes to PMU MSRs

[Paolo Bonzini]

Whenever an MSR is part of KVM_GET_MSR_INDEX_LIST, it has to be always
retrievable and settable with KVM_GET_MSR and KVM_SET_MSR.  Accept
the PMU MSRs unconditionally in intel_is_valid_msr, if the access was
host-initiated.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/pmu.c           |  4 ++--
 arch/x86/kvm/pmu.h           |  4 ++--
 arch/x86/kvm/svm/pmu.c       |  2 +-
 arch/x86/kvm/vmx/pmu_intel.c | 27 +++++++++++++++++----------
 arch/x86/kvm/x86.c           | 10 +++++-----
 5 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index a2eaae85d97b..c6e57367f009 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -455,10 +455,10 @@ void kvm_pmu_deliver_pmi(struct kvm_vcpu *vcpu)
 	}
 }
 
-bool kvm_pmu_is_valid_msr(struct kvm_vcpu *vcpu, u32 msr)
+bool kvm_pmu_is_valid_msr(struct kvm_vcpu *vcpu, u32 msr, bool host_initiated)
 {
 	return static_call(kvm_x86_pmu_msr_idx_to_pmc)(vcpu, msr) ||
-		static_call(kvm_x86_pmu_is_valid_msr)(vcpu, msr);
+		static_call(kvm_x86_pmu_is_valid_msr)(vcpu, msr, host_initiated);
 }
 
 static void kvm_pmu_mark_pmc_in_use(struct kvm_vcpu *vcpu, u32 msr)
diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
index 7824bdd8626e..cea52d1bcc76 100644
--- a/arch/x86/kvm/pmu.h
+++ b/arch/x86/kvm/pmu.h
@@ -29,7 +29,7 @@ struct kvm_pmu_ops {
 		unsigned int idx, u64 *mask);
 	struct kvm_pmc *(*msr_idx_to_pmc)(struct kvm_vcpu *vcpu, u32 msr);
 	bool (*is_valid_rdpmc_ecx)(struct kvm_vcpu *vcpu, unsigned int idx);
-	bool (*is_valid_msr)(struct kvm_vcpu *vcpu, u32 msr);
+	bool (*is_valid_msr)(struct kvm_vcpu *vcpu, u32 msr, bool host_initiated);
 	int (*get_msr)(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
 	int (*set_msr)(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
 	void (*refresh)(struct kvm_vcpu *vcpu);
@@ -186,7 +186,7 @@ void kvm_pmu_deliver_pmi(struct kvm_vcpu *vcpu);
 void kvm_pmu_handle_event(struct kvm_vcpu *vcpu);
 int kvm_pmu_rdpmc(struct kvm_vcpu *vcpu, unsigned pmc, u64 *data);
 bool kvm_pmu_is_valid_rdpmc_ecx(struct kvm_vcpu *vcpu, unsigned int idx);
-bool kvm_pmu_is_valid_msr(struct kvm_vcpu *vcpu, u32 msr);
+bool kvm_pmu_is_valid_msr(struct kvm_vcpu *vcpu, u32 msr, bool host_initiated);
 int kvm_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
 int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
 void kvm_pmu_refresh(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
index 136039fc6d01..0e5784371ac0 100644
--- a/arch/x86/kvm/svm/pmu.c
+++ b/arch/x86/kvm/svm/pmu.c
@@ -229,7 +229,7 @@ static struct kvm_pmc *amd_rdpmc_ecx_to_pmc(struct kvm_vcpu *vcpu,
 	return &counters[idx];
 }
 
-static bool amd_is_valid_msr(struct kvm_vcpu *vcpu, u32 msr)
+static bool amd_is_valid_msr(struct kvm_vcpu *vcpu, u32 msr, bool host_initiated)
 {
 	/* All MSRs refer to exactly one PMC, so msr_idx_to_pmc is enough.  */
 	return false;
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index 66496cb41494..c8c3f55630ea 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -207,38 +207,45 @@ static bool intel_pmu_is_valid_lbr_msr(struct kvm_vcpu *vcpu, u32 index)
 	return false;
 }
 
-static bool intel_is_valid_msr(struct kvm_vcpu *vcpu, u32 msr)
+static bool intel_is_valid_msr(struct kvm_vcpu *vcpu, u32 msr, bool host_initiated)
 {
 	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
 	u64 perf_capabilities = vcpu->arch.perf_capabilities;
-	int ret;
 
 	switch (msr) {
 	case MSR_CORE_PERF_FIXED_CTR_CTRL:
 	case MSR_CORE_PERF_GLOBAL_STATUS:
 	case MSR_CORE_PERF_GLOBAL_CTRL:
 	case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
-		ret = pmu->version > 1;
+		if (host_initiated)
+			return true;
+		return pmu->version > 1;
 		break;
 	case MSR_IA32_PEBS_ENABLE:
-		ret = perf_capabilities & PERF_CAP_PEBS_FORMAT;
+		if (host_initiated)
+			return true;
+		return perf_capabilities & PERF_CAP_PEBS_FORMAT;
 		break;
 	case MSR_IA32_DS_AREA:
-		ret = guest_cpuid_has(vcpu, X86_FEATURE_DS);
+		if (host_initiated)
+			return true;
+		return guest_cpuid_has(vcpu, X86_FEATURE_DS);
 		break;
 	case MSR_PEBS_DATA_CFG:
-		ret = (perf_capabilities & PERF_CAP_PEBS_BASELINE) &&
+		if (host_initiated)
+			return true;
+		return (perf_capabilities & PERF_CAP_PEBS_BASELINE) &&
 			((perf_capabilities & PERF_CAP_PEBS_FORMAT) > 3);
 		break;
 	default:
-		ret = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0) ||
+		if (host_initiated)
+			return true;
+		return get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0) ||
 			get_gp_pmc(pmu, msr, MSR_P6_EVNTSEL0) ||
 			get_fixed_pmc(pmu, msr) || get_fw_gp_pmc(pmu, msr) ||
 			intel_pmu_is_valid_lbr_msr(vcpu, msr);
 		break;
 	}
-
-	return ret;
 }
 
 static struct kvm_pmc *intel_msr_idx_to_pmc(struct kvm_vcpu *vcpu, u32 msr)
@@ -688,7 +695,7 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
 		INTEL_PMC_MAX_GENERIC, pmu->nr_arch_fixed_counters);
 
 	nested_vmx_pmu_refresh(vcpu,
-			       intel_is_valid_msr(vcpu, MSR_CORE_PERF_GLOBAL_CTRL));
+			       intel_is_valid_msr(vcpu, MSR_CORE_PERF_GLOBAL_CTRL, false));
 
 	if (cpuid_model_is_consistent(vcpu)) {
 		x86_perf_get_lbr(&lbr_desc->records);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a291236b4695..7460b9a77d9a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3725,7 +3725,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		fallthrough;
 	case MSR_K7_EVNTSEL0 ... MSR_K7_EVNTSEL3:
 	case MSR_P6_EVNTSEL0 ... MSR_P6_EVNTSEL1:
-		if (kvm_pmu_is_valid_msr(vcpu, msr))
+		if (kvm_pmu_is_valid_msr(vcpu, msr, msr_info->host_initiated))
 			return kvm_pmu_set_msr(vcpu, msr_info);
 
 		if (pr || data != 0)
@@ -3808,7 +3808,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		break;
 #endif
 	default:
-		if (kvm_pmu_is_valid_msr(vcpu, msr))
+		if (kvm_pmu_is_valid_msr(vcpu, msr, msr_info->host_initiated))
 			return kvm_pmu_set_msr(vcpu, msr_info);
 		return KVM_MSR_RET_INVALID;
 	}
@@ -3888,7 +3888,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		msr_info->data = 0;
 		break;
 	case MSR_F15H_PERF_CTL0 ... MSR_F15H_PERF_CTR5:
-		if (kvm_pmu_is_valid_msr(vcpu, msr_info->index))
+		if (kvm_pmu_is_valid_msr(vcpu, msr_info->index, msr_info->host_initiated))
 			return kvm_pmu_get_msr(vcpu, msr_info);
 		if (!msr_info->host_initiated)
 			return 1;
@@ -3898,7 +3898,7 @@ int kvm_get_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:
 	case MSR_P6_EVNTSEL0 ... MSR_P6_EVNTSEL1:
-		if (kvm_pmu_is_valid_msr(vcpu, msr_info->index))
+		if (kvm_pmu_is_valid_msr(vcpu, msr_info->index, msr_info->host_initiated))
 			return kvm_pmu_get_msr(vcpu, msr_info);
 		msr_info->data = 0;
 		break;
@@ -4144,7 +4144,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		break;
 #endif
 	default:
-		if (kvm_pmu_is_valid_msr(vcpu, msr_info->index))
+		if (kvm_pmu_is_valid_msr(vcpu, msr_info->index, msr_info->host_initiated))
 			return kvm_pmu_get_msr(vcpu, msr_info);
 		return KVM_MSR_RET_INVALID;
 	}
-- 
2.31.1

Re: [PATCH 1/2] KVM: vmx, pmu: accept 0 for absent MSRs when host-initiated

[Sean Christopherson]

On Tue, May 31, 2022, Paolo Bonzini wrote:
> Whenever an MSR is part of KVM_GET_MSR_INDEX_LIST, as is the case for
> MSR_IA32_DS_AREA, MSR_ARCH_LBR_DEPTH or MSR_ARCH_LBR_CTL, it has to be
> always settable with KVM_SET_MSR.  Accept a zero value for these MSRs
> to obey the contract.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/vmx/pmu_intel.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
> index 3e04d0407605..66496cb41494 100644
> --- a/arch/x86/kvm/vmx/pmu_intel.c
> +++ b/arch/x86/kvm/vmx/pmu_intel.c
> @@ -367,8 +367,9 @@ static bool arch_lbr_depth_is_valid(struct kvm_vcpu *vcpu, u64 depth)
>  {
>  	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
>  
> -	if (!kvm_cpu_cap_has(X86_FEATURE_ARCH_LBR))
> -		return false;
> +	if (!kvm_cpu_cap_has(X86_FEATURE_ARCH_LBR) ||
> +	    !guest_cpuid_has(vcpu, X86_FEATURE_ARCH_LBR))
> +		return depth == 0;
>  
>  	return (depth == pmu->kvm_arch_lbr_depth);
>  }
> @@ -378,7 +379,7 @@ static bool arch_lbr_ctl_is_valid(struct kvm_vcpu *vcpu, u64 ctl)
>  	struct kvm_cpuid_entry2 *entry;
>  
>  	if (!kvm_cpu_cap_has(X86_FEATURE_ARCH_LBR))
> -		return false;
> +		return ctl == 0;
>  
>  	if (ctl & ~KVM_ARCH_LBR_CTL_MASK)
>  		goto warn;
> @@ -510,6 +511,8 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		}
>  		break;
>  	case MSR_IA32_DS_AREA:
> +		if (msr_info->host_initiated && data && !guest_cpuid_has(vcpu, X86_FEATURE_DS))
> +			return 1;
>  		if (is_noncanonical_address(data, vcpu))
>  			return 1;
>  		pmu->ds_area = data;
> @@ -525,7 +528,11 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	case MSR_ARCH_LBR_DEPTH:
>  		if (!arch_lbr_depth_is_valid(vcpu, data))
>  			return 1;
> +
>  		lbr_desc->records.nr = data;
> +		if (!guest_cpuid_has(vcpu, X86_FEATURE_ARCH_LBR))
> +			return 0;

This is wrong, it will allow an unchecked wrmsrl() to MSR_ARCH_LBR_DEPTH if
X86_FEATURE_ARCH_LBR is not supported by hardware but userspace forces it in
guest CPUID. 

This the only user of arch_lbr_depth_is_valid(), just open code the logic.

> +
>  		/*
>  		 * Writing depth MSR from guest could either setting the
>  		 * MSR or resetting the LBR records with the side-effect.
> @@ -535,6 +542,8 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	case MSR_ARCH_LBR_CTL:
>  		if (!arch_lbr_ctl_is_valid(vcpu, data))
>  			break;
> +		if (!guest_cpuid_has(vcpu, X86_FEATURE_ARCH_LBR))
> +			return 0;

Similar bug here.

Can we just punt this out of kvm/queue until its been properly reviewed?  At the
barest of glances, there are multiple flaws that should block this from being
merged.  Based on the number of checks against X86_FEATURE_ARCH_LBR in KVM, and
my vague recollection of the passthrough behavior, this is a _massive_ feature.

The pr_warn_ratelimited() shouldn't exist; it's better than a non-ratelimited warn,
but it's ultimately useless.

This should check kvm_cpu_has() to ensure the field exists, e.g. if the feature
is supported in hardware but cpu_has_vmx_arch_lbr() returns false for whatever
reason.

	if (!init_event) {
		if (static_cpu_has(X86_FEATURE_ARCH_LBR))
			vmcs_write64(GUEST_IA32_LBR_CTL, 0);

intel_pmu_lbr_is_enabled() is going to be a performance problem, e.g. _should_ be
gated by static_cpu_has() to avoid overhead on CPUs without arch LBRs, and is
going to incur a _guest_ CPUID lookup on X86_FEATURE_PDCM for every VM-Entry if
arch LBRs are exposed to the guest (at least, I think that's what it does).

>  
>  		vmcs_write64(GUEST_IA32_LBR_CTL, data);
>  
> -- 
> 2.31.1
> 
> 

Re: [PATCH 2/2] KVM: x86: always allow host-initiated writes to PMU MSRs

[Like Xu]

On 1/6/2022 1:54 am, Paolo Bonzini wrote:
>   	switch (msr) {
>   	case MSR_CORE_PERF_FIXED_CTR_CTRL:
>   	case MSR_CORE_PERF_GLOBAL_STATUS:
>   	case MSR_CORE_PERF_GLOBAL_CTRL:
>   	case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
> -		ret = pmu->version > 1;
> +		if (host_initiated)
> +			return true;
> +		return pmu->version > 1;

I was shocked not to see this style of code:

	return host_initiated || other-else;

>   		break;
>   	case MSR_IA32_PEBS_ENABLE:
> -		ret = perf_capabilities & PERF_CAP_PEBS_FORMAT;
> +		if (host_initiated)
> +			return true;
> +		return perf_capabilities & PERF_CAP_PEBS_FORMAT;
>   		break;
>   	case MSR_IA32_DS_AREA:
> -		ret = guest_cpuid_has(vcpu, X86_FEATURE_DS);
> +		if (host_initiated)
> +			return true;
> +		return guest_cpuid_has(vcpu, X86_FEATURE_DS);
>   		break;
>   	case MSR_PEBS_DATA_CFG:
> -		ret = (perf_capabilities & PERF_CAP_PEBS_BASELINE) &&
> +		if (host_initiated)
> +			return true;
> +		return (perf_capabilities & PERF_CAP_PEBS_BASELINE) &&
>   			((perf_capabilities & PERF_CAP_PEBS_FORMAT) > 3);
>   		break;
>   	default:
> -		ret = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0) ||
> +		if (host_initiated)
> +			return true;

All default checks will fall in here.

Considering the MSR addresses of different groups are contiguous,
how about separating this part of the mixed statement may look even clearer:

+       case MSR_IA32_PERFCTR0 ... (MSR_IA32_PERFCTR0 + INTEL_PMC_MAX_GENERIC - 1):
+               return host_initiated || get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0);
+       case MSR_IA32_PMC0 ... (MSR_IA32_PMC0 + INTEL_PMC_MAX_GENERIC -1 ):
+               return host_initiated || get_fw_gp_pmc(pmu, msr);
+       case MSR_P6_EVNTSEL0 ... (MSR_P6_EVNTSEL0 + INTEL_PMC_MAX_GENERIC - 1):
+                       return host_initiated || get_gp_pmc(pmu, msr, 
MSR_P6_EVNTSEL0);
+       case MSR_CORE_PERF_FIXED_CTR0 ... (MSR_CORE_PERF_FIXED_CTR0 + 
KVM_PMC_MAX_FIXED - 1):
+               return host_initiated || get_fixed_pmc(pmu, msr);

> +		return get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0) ||
>   			get_gp_pmc(pmu, msr, MSR_P6_EVNTSEL0) ||
>   			get_fixed_pmc(pmu, msr) || get_fw_gp_pmc(pmu, msr) ||
>   			intel_pmu_is_valid_lbr_msr(vcpu, msr);
>   		break;
>   	}

Re: [PATCH 1/2] KVM: vmx, pmu: accept 0 for absent MSRs when host-initiated

[Like Xu]

On 1/6/2022 2:37 am, Sean Christopherson wrote:
> On Tue, May 31, 2022, Paolo Bonzini wrote:
>> Whenever an MSR is part of KVM_GET_MSR_INDEX_LIST, as is the case for
>> MSR_IA32_DS_AREA, MSR_ARCH_LBR_DEPTH or MSR_ARCH_LBR_CTL, it has to be
>> always settable with KVM_SET_MSR.  Accept a zero value for these MSRs
>> to obey the contract.

Do we have a rule to decide whether to put MSRs into KVM_GET_MSR_INDEX_LIST,
for example a large number of LBR MSRs do not appear in it ?

I assume that this rule also applies in the case of !enable_pmu:

diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index 7a74691de223..3575a3746bf9 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -439,11 +439,19 @@ static void kvm_pmu_mark_pmc_in_use(struct kvm_vcpu *vcpu, 
u32 msr)

  int kvm_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
  {
+	if (msr_info->host_initiated && !vcpu->kvm->arch.enable_pmu) {
+		msr_info->data = 0;
+		return 0;
+	}
+
  	return static_call(kvm_x86_pmu_get_msr)(vcpu, msr_info);
  }

  int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
  {
+	if (msr_info->host_initiated && !vcpu->kvm->arch.enable_pmu)
+		return !!msr_info->data;
+
  	kvm_pmu_mark_pmc_in_use(vcpu, msr_info->index);
  	return static_call(kvm_x86_pmu_set_msr)(vcpu, msr_info);
  }
diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
index 256244b8f89c..fe520b2649b5 100644
--- a/arch/x86/kvm/svm/pmu.c
+++ b/arch/x86/kvm/svm/pmu.c
@@ -182,7 +182,16 @@ static struct kvm_pmc *amd_rdpmc_ecx_to_pmc(struct kvm_vcpu 
*vcpu,
  static bool amd_is_valid_msr(struct kvm_vcpu *vcpu, u32 msr, bool host_initiated)
  {
  	/* All MSRs refer to exactly one PMC, so msr_idx_to_pmc is enough.  */
-	return false;
+	if (!host_initiated)
+		return false;
+
+	switch (msr) {
+	case MSR_K7_EVNTSEL0 ... MSR_K7_PERFCTR3:
+	case MSR_F15H_PERF_CTL0 ... MSR_F15H_PERF_CTR5:
+		return true;
+	default:
+		return false;
+	}
  }

  static struct kvm_pmc *amd_msr_idx_to_pmc(struct kvm_vcpu *vcpu, u32 msr)

>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   arch/x86/kvm/vmx/pmu_intel.c | 15 ++++++++++++---
>>   1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
>> index 3e04d0407605..66496cb41494 100644
>> --- a/arch/x86/kvm/vmx/pmu_intel.c
>> +++ b/arch/x86/kvm/vmx/pmu_intel.c
>> @@ -367,8 +367,9 @@ static bool arch_lbr_depth_is_valid(struct kvm_vcpu *vcpu, u64 depth)
>>   {
>>   	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
>>   
>> -	if (!kvm_cpu_cap_has(X86_FEATURE_ARCH_LBR))
>> -		return false;
>> +	if (!kvm_cpu_cap_has(X86_FEATURE_ARCH_LBR) ||
>> +	    !guest_cpuid_has(vcpu, X86_FEATURE_ARCH_LBR))
>> +		return depth == 0;
>>   
>>   	return (depth == pmu->kvm_arch_lbr_depth);
>>   }
>> @@ -378,7 +379,7 @@ static bool arch_lbr_ctl_is_valid(struct kvm_vcpu *vcpu, u64 ctl)
>>   	struct kvm_cpuid_entry2 *entry;
>>   
>>   	if (!kvm_cpu_cap_has(X86_FEATURE_ARCH_LBR))
>> -		return false;
>> +		return ctl == 0;
>>   
>>   	if (ctl & ~KVM_ARCH_LBR_CTL_MASK)
>>   		goto warn;
>> @@ -510,6 +511,8 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>   		}
>>   		break;
>>   	case MSR_IA32_DS_AREA:
>> +		if (msr_info->host_initiated && data && !guest_cpuid_has(vcpu, X86_FEATURE_DS))
>> +			return 1;
>>   		if (is_noncanonical_address(data, vcpu))
>>   			return 1;
>>   		pmu->ds_area = data;
>> @@ -525,7 +528,11 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>   	case MSR_ARCH_LBR_DEPTH:
>>   		if (!arch_lbr_depth_is_valid(vcpu, data))
>>   			return 1;
>> +
>>   		lbr_desc->records.nr = data;
>> +		if (!guest_cpuid_has(vcpu, X86_FEATURE_ARCH_LBR))
>> +			return 0;
> 
> This is wrong, it will allow an unchecked wrmsrl() to MSR_ARCH_LBR_DEPTH if
> X86_FEATURE_ARCH_LBR is not supported by hardware but userspace forces it in
> guest CPUID.

What should we expect if the userspace forces guest to use features not 
supported by KVM,
especially the emulation of this feature depends on the functionality of host 
and guest vcpu model ?

> 
> This the only user of arch_lbr_depth_is_valid(), just open code the logic.
> 
>> +
>>   		/*
>>   		 * Writing depth MSR from guest could either setting the
>>   		 * MSR or resetting the LBR records with the side-effect.
>> @@ -535,6 +542,8 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>   	case MSR_ARCH_LBR_CTL:
>>   		if (!arch_lbr_ctl_is_valid(vcpu, data))
>>   			break;
>> +		if (!guest_cpuid_has(vcpu, X86_FEATURE_ARCH_LBR))
>> +			return 0;
> 
> Similar bug here.
> 
> Can we just punt this out of kvm/queue until its been properly reviewed?  At the
> barest of glances, there are multiple flaws that should block this from being

TBH, our reviewers' attention could not be focused on these patches until the
day it was ready to be ravaged. "Try to accept" is a good thing, and things need
to move forward, not simply be abandoned to the side.

Although later versions of ARCH_LBR is not on my review list, any developer would
sincerely appreciate finding more flaws through the queue tree, please take a 
look at PEBS.

> merged.  Based on the number of checks against X86_FEATURE_ARCH_LBR in KVM, and
> my vague recollection of the passthrough behavior, this is a _massive_ feature.
> 
> The pr_warn_ratelimited() shouldn't exist; it's better than a non-ratelimited warn,
> but it's ultimately useless.

We have two pr_warn_ratelimited(). The one in arch_lbr_ctl_is_valid() should be 
dropped.

> 
> This should check kvm_cpu_has() to ensure the field exists, e.g. if the feature
> is supported in hardware but cpu_has_vmx_arch_lbr() returns false for whatever
> reason.
> 
> 	if (!init_event) {
> 		if (static_cpu_has(X86_FEATURE_ARCH_LBR))
> 			vmcs_write64(GUEST_IA32_LBR_CTL, 0);

If we tweak vmcs_config.vm*_ctrl via adjust_vmx_controls(), VMCS fields
(if any, like this one on a capable platform) are still accessible on the KVM 
side, aren't they?

- VM_ENTRY_LOAD_IA32_LBR_CTL
- VM_EXIT_CLEAR_IA32_LBR_CTL

> 
> intel_pmu_lbr_is_enabled() is going to be a performance problem, e.g. _should_ be
> gated by static_cpu_has() to avoid overhead on CPUs without arch LBRs, and is
> going to incur a _guest_ CPUID lookup on X86_FEATURE_PDCM for every VM-Entry if
> arch LBRs are exposed to the guest (at least, I think that's what it does).

Indeed, this also applies to the legacy LBR, and we can certainly improve it.

> 
>>   
>>   		vmcs_write64(GUEST_IA32_LBR_CTL, data);
>>   
>> -- 
>> 2.31.1
>>
>>
> 

Re: [PATCH 1/2] KVM: vmx, pmu: accept 0 for absent MSRs when host-initiated

[Paolo Bonzini]

On 6/1/22 04:46, Like Xu wrote:
> On 1/6/2022 2:37 am, Sean Christopherson wrote:
>> On Tue, May 31, 2022, Paolo Bonzini wrote:
>>> Whenever an MSR is part of KVM_GET_MSR_INDEX_LIST, as is the case for
>>> MSR_IA32_DS_AREA, MSR_ARCH_LBR_DEPTH or MSR_ARCH_LBR_CTL, it has to be
>>> always settable with KVM_SET_MSR.  Accept a zero value for these MSRs
>>> to obey the contract.
> 
> Do we have a rule to decide whether to put MSRs into 
> KVM_GET_MSR_INDEX_LIST,
> for example a large number of LBR MSRs do not appear in it ?

In general I think it's much better to include them.  The only reason 
not to include them should be if the number of MSRs is variable and the 
actual number is accessible via KVM_GET_SUPPORTED_CPUID, a feature MSR, 
or KVM_CHECK_EXTENSION.

>> This is wrong, it will allow an unchecked wrmsrl() to 
>> MSR_ARCH_LBR_DEPTH if
>> X86_FEATURE_ARCH_LBR is not supported by hardware but userspace forces 
>> it in
>> guest CPUID.
> 
> What should we expect if the userspace forces guest to use features not 
> supported by KVM,
> especially the emulation of this feature depends on the functionality of 
> host and guest vcpu model ?

Certainly not a WARN or invalid vmwrite.

Paolo

Re: [PATCH 1/2] KVM: vmx, pmu: accept 0 for absent MSRs when host-initiated

[Paolo Bonzini]

On 5/31/22 20:37, Sean Christopherson wrote:
>> +
>>   		/*
>>   		 * Writing depth MSR from guest could either setting the
>>   		 * MSR or resetting the LBR records with the side-effect.
>> @@ -535,6 +542,8 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>   	case MSR_ARCH_LBR_CTL:
>>   		if (!arch_lbr_ctl_is_valid(vcpu, data))
>>   			break;
>> +		if (!guest_cpuid_has(vcpu, X86_FEATURE_ARCH_LBR))
>> +			return 0;
> 
> Similar bug here.
> 
> Can we just punt this out of kvm/queue until its been properly reviewed?

Yes, I agree.  I have started making some changes and pushed the result 
to kvm/arch-lbr-for-weijiang.

Most of the MSR handling is rewritten (and untested).

The nested VMX handling was also completely broken so I just removed it. 
  Instead, KVM should be adjusted so that it does not whine.

Paolo

Re: [PATCH 1/2] KVM: vmx, pmu: accept 0 for absent MSRs when host-initiated

[Yang, Weijiang]

On 6/1/2022 4:54 PM, Paolo Bonzini wrote:
> On 5/31/22 20:37, Sean Christopherson wrote:
>>> +
>>>           /*
>>>            * Writing depth MSR from guest could either setting the
>>>            * MSR or resetting the LBR records with the side-effect.
>>> @@ -535,6 +542,8 @@ static int intel_pmu_set_msr(struct kvm_vcpu 
>>> *vcpu, struct msr_data *msr_info)
>>>       case MSR_ARCH_LBR_CTL:
>>>           if (!arch_lbr_ctl_is_valid(vcpu, data))
>>>               break;
>>> +        if (!guest_cpuid_has(vcpu, X86_FEATURE_ARCH_LBR))
>>> +            return 0;
>>
>> Similar bug here.
>>
>> Can we just punt this out of kvm/queue until its been properly reviewed?
>
> Yes, I agree.  I have started making some changes and pushed the 
> result to kvm/arch-lbr-for-weijiang.
>
> Most of the MSR handling is rewritten (and untested).
>
> The nested VMX handling was also completely broken so I just removed 
> it.  Instead, KVM should be adjusted so that it does not whine.

Noted, I'll run tests based on it, thanks a lot!

Has the branch been pushed? I cannot see it.

>
> Paolo
>

Re: [PATCH 1/2] KVM: vmx, pmu: accept 0 for absent MSRs when host-initiated

[Paolo Bonzini]

On 6/1/22 11:12, Yang, Weijiang wrote:
>>>
>>
>> Yes, I agree.  I have started making some changes and pushed the 
>> result to kvm/arch-lbr-for-weijiang.
>>
>> Most of the MSR handling is rewritten (and untested).
>>
>> The nested VMX handling was also completely broken so I just removed 
>> it.  Instead, KVM should be adjusted so that it does not whine.
> 
> Noted, I'll run tests based on it, thanks a lot!
> 
> Has the branch been pushed? I cannot see it.

It's just lbr-for-weijiang, sorry for mistyping.

Paolo

Re: [PATCH 1/2] KVM: vmx, pmu: accept 0 for absent MSRs when host-initiated

[Yang, Weijiang]

On 6/1/2022 6:15 PM, Paolo Bonzini wrote:
> On 6/1/22 11:12, Yang, Weijiang wrote:
>>> Yes, I agree.  I have started making some changes and pushed the
>>> result to kvm/arch-lbr-for-weijiang.
>>>
>>> Most of the MSR handling is rewritten (and untested).
>>>
>>> The nested VMX handling was also completely broken so I just removed
>>> it.  Instead, KVM should be adjusted so that it does not whine.
>> Noted, I'll run tests based on it, thanks a lot!
>>
>> Has the branch been pushed? I cannot see it.
> It's just lbr-for-weijiang, sorry for mistyping.
Found it, thank you!
>
> Paolo
>

Re: [PATCH 1/2] KVM: vmx, pmu: accept 0 for absent MSRs when host-initiated

[Sean Christopherson]

On Wed, Jun 01, 2022, Like Xu wrote:
> On 1/6/2022 2:37 am, Sean Christopherson wrote:
> > Can we just punt this out of kvm/queue until its been properly reviewed?  At the
> > barest of glances, there are multiple flaws that should block this from being
> 
> TBH, our reviewers' attention could not be focused on these patches until the
> day it was ready to be ravaged. "Try to accept" is a good thing, and things need
> to move forward, not simply be abandoned to the side.

I strongly disagree, to put it mildly.  Accepting flawed, buggy code because
reviewers and maintainers are overloaded does not solve anything, it only makes
the problem worse.  More than likely, the submitter(s) has moved on to writing
the next pile of patches, while the same set of people that are trying to review
submissions are left to pick up the pieces.  There are numerous examples of
accepting code without (IMO) proper review and tests costing us dearly in the
long run.

If people want their code to be merged more quickly, then they can do so by
helping address the underlying problems, e.g. write tests that actually try to
break their feature instead of doing the bare minimum, review each others code,
clean up the existing code (and tests!), etc...  There's a reason vPMU features
tend to not get a lot of reviews; KVM doesn't even get the basics right, so there's
not a lot of interest in trying to enable fancy, complex features.

Merging patches/series because they _haven't_ gotten reviews is all kinds of
backwards.  In addition to creating _more_ work for reviewers and maintainers,
it effectively penalizes teams/companies for reviewing each other's code, which
is seriously fubar and again exacerbates the problem of reviewers being overloaded.

Re: [PATCH 1/2] KVM: vmx, pmu: accept 0 for absent MSRs when host-initiated

[Like Xu]

Thanks for your sincerity, always.

On 2/6/2022 12:39 am, Sean Christopherson wrote:
> On Wed, Jun 01, 2022, Like Xu wrote:
>> On 1/6/2022 2:37 am, Sean Christopherson wrote:
>>> Can we just punt this out of kvm/queue until its been properly reviewed?  At the
>>> barest of glances, there are multiple flaws that should block this from being
>>
>> TBH, our reviewers' attention could not be focused on these patches until the
>> day it was ready to be ravaged. "Try to accept" is a good thing, and things need
>> to move forward, not simply be abandoned to the side.
> 
> I strongly disagree, to put it mildly.  Accepting flawed, buggy code because
> reviewers and maintainers are overloaded does not solve anything, it only makes
> the problem worse.  More than likely, the submitter(s) has moved on to writing
> the next pile of patches, while the same set of people that are trying to review
> submissions are left to pick up the pieces.  There are numerous examples of
> accepting code without (IMO) proper review and tests costing us dearly in the
> long run.

I actually agree and understand the situation of maintainers/reviewers.
No one wants to maintain flawed code, especially in this community
where the majority of previous contributors disappeared after the code
was merged in. The existing heavy maintenance burden is already visible.

Thus we may have a maintainer/reviewers scalability issue. Due to missing
trust, competence or mastery of rules, most of the patches sent to the list
have no one to point out their flaws. I have privately received many complaints
about the indifference of our community, which is distressing.

To improve that, I propose to add "let's try to accept" before "queued, thanks".

Obviously, "try to accept" is not a 100% commitment and it will fail with high
probability, but such a stance (along with standard clarifications and requirements)
from reviewers and maintainers will make the contributors more concerned,
attract potential volunteers, and focus the efforts of our nominated reviewers.

Such moves include explicit acceptance or rejection, a "try to accept" response
from some key persons (even if it ends up being a failure), or a separate git 
branch,
but please, don't leave a lasting silence, especially for those big series.

Similar moves will increase transparency in decision making to reward and
attract a steady stream of high quality trusted contributors to do more and more
for our community and their employers (if any).

> 
> If people want their code to be merged more quickly, then they can do so by
> helping address the underlying problems, e.g. write tests that actually try to
> break their feature instead of doing the bare minimum, review each others code,
> clean up the existing code (and tests!), etc...  There's a reason vPMU features
> tend to not get a lot of reviews; KVM doesn't even get the basics right, so there's
> not a lot of interest in trying to enable fancy, complex features.

I'd like know more about "KVM doesn't even get the basics right" on vPMU. :D

> 
> Merging patches/series because they _haven't_ gotten reviews is all kinds of
> backwards.  In addition to creating _more_ work for reviewers and maintainers,
> it effectively penalizes teams/companies for reviewing each other's code, which
> is seriously fubar and again exacerbates the problem of reviewers being overloaded.

Re: [PATCH 2/2] KVM: x86: always allow host-initiated writes to PMU MSRs

[Sean Christopherson]

On Tue, May 31, 2022, Paolo Bonzini wrote:
> Whenever an MSR is part of KVM_GET_MSR_INDEX_LIST, it has to be always
> retrievable and settable with KVM_GET_MSR and KVM_SET_MSR.  Accept
> the PMU MSRs unconditionally in intel_is_valid_msr, if the access was
> host-initiated.

...so that userspace can explode in intel_get_msr() or intel_set_msr().  Selftests
that regurgitate MSRs are still failing.  The below "fixes" the issue, but I don't
know that it's actually a good idea.  I also haven't tried AMD.

diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index 515ab6594333..fcb5224028a6 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -401,7 +401,7 @@ static int intel_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
                        return 0;
        }

-       return 1;
+       return !msr_info->host_initiated;
 }

 static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
@@ -497,7 +497,7 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
                        return 0;
        }

-       return 1;
+       return !msr_info->host_initiated;
 }

 static void setup_fixed_pmc_eventsel(struct kvm_pmu *pmu)

Re: [PATCH 1/2] KVM: vmx, pmu: accept 0 for absent MSRs when host-initiated

[Sean Christopherson]

On Thu, Jun 02, 2022, Like Xu wrote:
> Thanks for your sincerity, always.
> 
> On 2/6/2022 12:39 am, Sean Christopherson wrote:
> > On Wed, Jun 01, 2022, Like Xu wrote:
> > > On 1/6/2022 2:37 am, Sean Christopherson wrote:
> > > > Can we just punt this out of kvm/queue until its been properly reviewed?  At the
> > > > barest of glances, there are multiple flaws that should block this from being
> > > 
> > > TBH, our reviewers' attention could not be focused on these patches until the
> > > day it was ready to be ravaged. "Try to accept" is a good thing, and things need
> > > to move forward, not simply be abandoned to the side.
> > 
> > I strongly disagree, to put it mildly.  Accepting flawed, buggy code because
> > reviewers and maintainers are overloaded does not solve anything, it only makes
> > the problem worse.  More than likely, the submitter(s) has moved on to writing
> > the next pile of patches, while the same set of people that are trying to review
> > submissions are left to pick up the pieces.  There are numerous examples of
> > accepting code without (IMO) proper review and tests costing us dearly in the
> > long run.
> 
> I actually agree and understand the situation of maintainers/reviewers.
> No one wants to maintain flawed code, especially in this community
> where the majority of previous contributors disappeared after the code
> was merged in. The existing heavy maintenance burden is already visible.
> 
> Thus we may have a maintainer/reviewers scalability issue. Due to missing
> trust, competence or mastery of rules, most of the patches sent to the list
> have no one to point out their flaws.

Then write tests and run the ones that already exist.  Relying purely on reviewers
to detect flaws does not and cannot scale.  I agree that we currently have a
scalability issue, but I have different views on how to improve things.

> I have privately received many complaints about the indifference of our
> community, which is distressing.
> 
> To improve that, I propose to add "let's try to accept" before "queued, thanks".
> 
> Obviously, "try to accept" is not a 100% commitment and it will fail with high
> probability, but such a stance (along with standard clarifications and requirements)
> from reviewers and maintainers will make the contributors more concerned,
> attract potential volunteers, and focus the efforts of our nominated reviewers.
> 
> Such moves include explicit acceptance or rejection, a "try to accept" response
> from some key persons (even if it ends up being a failure), or a separate
> git branch,
> but please, don't leave a lasting silence, especially for those big series.

I completely agree on needing better transparency for the lifecycle of patches  
going through the KVM tree.  First and foremost, there need to be formal, documented 
rules for the "official" kvm/* branches, e.g. everything in kvm/queue passes ABC
tests, everything in kvm/next also passes XYZ tests.  That would also be a good 
place to document expectations, how things works, etc...

At that point, I think we could add e.g. kvm/pending to hold patches that have  
gotten the "queued, thanks" but haven't yet passed testing to get all the way to
kvm/queue.  In theory, that would eliminate, or at least significantly reduce, the
non-trivial time gap between applying them locally and actually pushing to kvm/queue.

I'll work on writing rules+documentation and getting buy-in from Paolo.

That said, I'm still opposed to pivoting to a "let's try to accept" approach.
IMO, a major part of the problem is lack of testcases and lack of _running_ what
tests we do have.  To be blunt, it's beyond frustrating infuriating that a series
that's at v12 breaks _existing_ KVM-Unit-Tests on any AMD host.  And that same
series wasn't accompanied by any new testcases.

Yes, a kvm/pending or whatever would help mitigate such issues, but that doesn't
fundamentally reduce the burden put on maintainers/reviewers.  Basic issues such
as breaking KUT should be caught before a series is posted.  I realize there are
exceptions, e.g. folks from Intel and AMD may not have access to the right
hardware, but given that you're also posting AMD specific patches, I don't think
that exception applies here.  And yes, mistakes will happen, I've certainly been
guilty of my share, but I fully expect any such mistakes to be caught well before
getting to v12.

The other way to reduce maintainer/reviewer burden is by writing thorough testcases.
A thorough, well-documented test not only proves that the code works, it also shows
to reviewiers that the developer actually considered edge cases and helps expedite
the process of thinking through such edge cases.  E.g. taking arch LBRs as an
example, a test that actually visited every edge in KVM's state machine for enabling
and disabling MSR intercepts would improve confidence that the code is correct and
would greatly help reviewers understand the various combinations and transitions.

I fully realize that writing tests is not glamorous, and that some of KVM's tooling
and infrastructure is lacking, but IMO having contributors truly buy-in to writing
tests for testing's sake instead of viewing writing tests as a necessary evil to get
accepted upstream would go a long, long way to helping improve the overall velocity
of KVM development.

> Similar moves will increase transparency in decision making to reward and
> attract a steady stream of high quality trusted contributors to do more and more
> for our community and their employers (if any).
> 
> > 
> > If people want their code to be merged more quickly, then they can do so by
> > helping address the underlying problems, e.g. write tests that actually try to
> > break their feature instead of doing the bare minimum, review each others code,
> > clean up the existing code (and tests!), etc...  There's a reason vPMU features
> > tend to not get a lot of reviews; KVM doesn't even get the basics right, so there's
> > not a lot of interest in trying to enable fancy, complex features.
> 
> I'd like know more about "KVM doesn't even get the basics right" on vPMU. :D

https://lore.kernel.org/all/CABOYuvbPL0DeEgV4gsC+v786xfBAo3T6+7XQr7cVVzbaoFoEAg@mail.gmail.com
https://lore.kernel.org/all/YofCfNsl6O45hYr0@google.com

Re: [PATCH 1/2] KVM: vmx, pmu: accept 0 for absent MSRs when host-initiated

[Paolo Bonzini]

On 6/15/22 20:52, Sean Christopherson wrote:
> On Thu, Jun 02, 2022, Like Xu wrote:
>> I actually agree and understand the situation of maintainers/reviewers.
>> No one wants to maintain flawed code, especially in this community
>> where the majority of previous contributors disappeared after the code
>> was merged in. The existing heavy maintenance burden is already visible.

I don't think this is true.  I think it's relatively rare for 
contributors to disappear.

>> Thus we may have a maintainer/reviewers scalability issue. Due to missing
>> trust, competence or mastery of rules, most of the patches sent to the list
>> have no one to point out their flaws.
> 
> Then write tests and run the ones that already exist.  Relying purely on reviewers
> to detect flaws does not and cannot scale.  I agree that we currently have a
> scalability issue, but I have different views on how to improve things.
> 
>> I have privately received many complaints about the indifference of our
>> community, which is distressing.

You're welcome to expand on these complaints.  But I suspect that a lot 
of these would come from people that have been told "review other 
people's work", "write tests" and/or "you submitted a broken patch" before.

"Let's try to accept" is basically what I did for PEBS and LBR, both of 
which I merged basically out of guilt after a little-more-than-cursory 
review.  It turns out that both of them were broken in ways that weren't 
subtle at all; and as a result, other work already queued to 5.19 had to 
be bumped to 5.20.

Honestly I should have complained and un-merged them right after seeing 
the msr.flat failure.  Or perhaps I should have just said "write tests 
and then I'll consider the series", but I "tried to accept" and we can 
already see it was a failure.

>> Obviously, "try to accept" is not a 100% commitment and it will fail with high
>> probability, but such a stance (along with standard clarifications and requirements)
>> from reviewers and maintainers will make the contributors more concerned,
>> attract potential volunteers, and focus the efforts of our nominated reviewers.

If it "fails with high probability", all that happened was a waste of 
time for everyone involved.  Including the submitter who has waited for 
weeks for a reviews only to be told "test X fails".

> I completely agree on needing better transparency for the lifecycle of patches
> going through the KVM tree.  First and foremost, there need to be formal, documented
> rules for the "official" kvm/* branches, e.g. everything in kvm/queue passes ABC
> tests, everything in kvm/next also passes XYZ tests.  That would also be a good
> place to document expectations, how things works, etc...

Agreed.  I think this is a more general problem with Linux development 
and I will propose this for maintainer summit.

But again, the relationship between contributors and maintainers should 
be of mutual benefit.  Rules help contributors, but contributors should 
themselves behave and not throw broken patches at maintainers.  And 
again, guess what the best way is to tell maintainers your patch is not 
broken?  Include a test.  It shows that you are paying attention.

> I fully realize that writing tests is not glamorous, and that some of KVM's tooling
> and infrastructure is lacking,

I wouldn't say lacking.  Sure it's complicated, but between selftests 
and kvm-unit-tests the tools *are* there.  selftests that allow you to 
test migration at an instruction boundary, for example, are not that 
hard to write and were very important for features such as nested state 
and AMX.  They're not perfect, but they go a long way towards giving 
confidence in the code; and it's easier to catch weird ioctl policies 
from reviewing comprehensive tests than from reviewing the actual KVM code.

We're not talking of something like SEV or TDX here, we're talking about 
very boring MSR emulation and only slightly less boring PMU passthrough.

Paolo

Re: [PATCH 1/2] KVM: vmx, pmu: accept 0 for absent MSRs when host-initiated

[Sean Christopherson]

On Thu, Jun 16, 2022, Paolo Bonzini wrote:
> On 6/15/22 20:52, Sean Christopherson wrote:
> > I completely agree on needing better transparency for the lifecycle of patches
> > going through the KVM tree.  First and foremost, there need to be formal, documented
> > rules for the "official" kvm/* branches, e.g. everything in kvm/queue passes ABC
> > tests, everything in kvm/next also passes XYZ tests.  That would also be a good
> > place to document expectations, how things works, etc...
> 
> Agreed.  I think this is a more general problem with Linux development and I
> will propose this for maintainer summit.

I believe the documentation side of things is an acknowledged gap, people just need
to actually write the documentation, e.g. Boris and Thomas documented the tip-tree
under Documentation/process/maintainer-tip.rst and stubbed in maintainer-handbooks.rst.

As for patch lifecycle, I would love to have something like tip-bot (can we just
steal whatever scripts they use?) that explicitly calls out the branch, commit,
committer, date, etc...  IMO that'd pair nicely with adding kvm/pending, as the
bot/script could provide updates when a patch is first added to kvm/pending, then
again when it got moved to kvm/queue or dropped because it was broken, etc...