On 7/10/2024 2:45 AM, Sean Christopherson wrote:
> On Tue, Jul 09, 2024, Dapeng Mi wrote:
>> Return KVM_MSR_RET_INVALID instead of 0 to inject #GP to guest for all
>> invalid PMU MSRs access
>>
>> Currently KVM silently drops the access and doesn't inject #GP for some
>> invalid PMU MSRs like MSR_P6_PERFCTR0/MSR_P6_PERFCTR1,
>> MSR_P6_EVNTSEL0/MSR_P6_EVNTSEL1, but KVM still injects #GP for all other
>> invalid PMU MSRs. This leads to guest see different behavior on invalid
>> PMU access and may confuse guest.
> This is by design. I'm not saying it's _good_ design, but it is very much
> intended. More importantly, it's established behavior, i.e. having KVM inject
> #GP could break existing setups.
>
>> This behavior is introduced by the
>> 'commit 5753785fa977 ("KVM: do not #GP on perf MSR writes when vPMU is disabled")'
>> in 2012. This commit seems to want to keep back compatible with weird
>> behavior of some guests in vPMU disabled case,
> Ya, because at the time, guest kernels hadn't been taught to play nice with
> unexpected virtualization setups, i.e. VMs without PMUs.
>
>> but strongly suspect if it's still available nowadays.
> I don't follow this comment.
>
>> Since Perfmon v6 starts, the GP counters could become discontinuous on
>> HW, It's possible that HW doesn't support GP counters 0 and 1.
>> Considering this situation KVM should inject #GP for all invalid PMU MSRs
>> access.
> IIUC, the behavior you want is inject a #GP if the vCPU has a PMU and the MSR is
> not valid. We can do that and still maintain backwards compatibility, hopefully
> without too much ugliness (maybe even an improvement!).
>
> This? (completely untested)
Seems no better method. Would adopt this. Thanks.
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 5aa7581802f7..b5e95e5f1f32 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4063,9 +4063,8 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> case MSR_P6_PERFCTR0 ... MSR_P6_PERFCTR1:
> 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 (vcpu_to_pmu(vcpu)->version)
> + goto default_handler;
> if (data)
> kvm_pr_unimpl_wrmsr(vcpu, msr, data);
> break;
> @@ -4146,6 +4145,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> break;
> #endif
> default:
> +default_handler:
> if (kvm_pmu_is_valid_msr(vcpu, msr))
> return kvm_pmu_set_msr(vcpu, msr_info);
>
> @@ -4251,8 +4251,8 @@ 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))
> - return kvm_pmu_get_msr(vcpu, msr_info);
> + if (vcpu_to_pmu(vcpu)->version)
> + goto default_handler;
> msr_info->data = 0;
> break;
> case MSR_IA32_UCODE_REV:
> @@ -4505,6 +4505,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> break;
> #endif
> default:
> +default_handler:
> if (kvm_pmu_is_valid_msr(vcpu, msr_info->index))
> return kvm_pmu_get_msr(vcpu, msr_info);
>
> diff --git a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
> index 96446134c00b..0de606b542ac 100644
> --- a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
> @@ -344,7 +344,8 @@ static void guest_test_rdpmc(uint32_t rdpmc_idx, bool expect_success,
> static void guest_rd_wr_counters(uint32_t base_msr, uint8_t nr_possible_counters,
> uint8_t nr_counters, uint32_t or_mask)
> {
> - const bool pmu_has_fast_mode = !guest_get_pmu_version();
> + const u8 pmu_version = guest_get_pmu_version();
> + const bool pmu_has_fast_mode = !pmu_version;
> uint8_t i;
>
> for (i = 0; i < nr_possible_counters; i++) {
> @@ -363,12 +364,13 @@ static void guest_rd_wr_counters(uint32_t base_msr, uint8_t nr_possible_counters
> const bool expect_success = i < nr_counters || (or_mask & BIT(i));
>
> /*
> - * KVM drops writes to MSR_P6_PERFCTR[0|1] if the counters are
> - * unsupported, i.e. doesn't #GP and reads back '0'.
> + * KVM drops writes to MSR_P6_PERFCTR[0|1] if the vCPU doesn't
> + * have a PMU, i.e. doesn't #GP and reads back '0'.
> */
> const uint64_t expected_val = expect_success ? test_val : 0;
> - const bool expect_gp = !expect_success && msr != MSR_P6_PERFCTR0 &&
> - msr != MSR_P6_PERFCTR1;
> + const bool expect_gp = !expect_success &&
> + (pmu_version ||
> + (msr != MSR_P6_PERFCTR0 && msr != MSR_P6_PERFCTR1));
> uint32_t rdpmc_idx;
> uint8_t vector;
> uint64_t val;
>
