Hi Paolo,
Thanks for your detailed comments.
On 2020/5/7 15:57, Paolo Bonzini wrote:
On 07/05/20 04:14, Like Xu wrote:
+static inline u64 vmx_get_perf_capabilities(void)
+{
+ u64 perf_cap = 0;
+
+ if (boot_cpu_has(X86_FEATURE_PDCM))
+ rdmsrl(MSR_IA32_PERF_CAPABILITIES, perf_cap);
+
+ /* Currently, KVM only supports Full-Width Writes. */
+ perf_cap &= PMU_CAP_FW_WRITES;
+
+ return perf_cap;
+}
+
Since counters are virtualized, it seems to me that you can support
PMU_CAP_FW_WRITES unconditionally, even if the host lacks it. So just
return PMU_CAP_FW_WRITES from this function.
Sure, let's export PMU_CAP_FW_WRITES unconditionally.
+ case MSR_IA32_PERF_CAPABILITIES:
+ return 1; /* RO MSR */
default:
You need to allow writes from the host if (data &
~vmx_get_perf_capabilities()) == 0.
Yes, it makes sense after I understand the intention of host_initiated.
- if (!msr_info->host_initiated)
+ if ((pmc = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0)) ||
+ (pmc = get_gp_pmc(pmu, msr, MSR_IA32_PMC0))) {
+ if (data & ~pmu->counter_bitmask[KVM_PMC_GP])
+ return 1;
+ if (!fw_writes_is_enabled(pmu))
data = (s64)(s32)data;
You are dropping the test on msr_info->host_initiated here, you should
keep it otherwise you allow full-width write to MSR_IA32_PERFCTR0 as
well. So:
#define MSR_PMC_FULL_WIDTH_BIT (MSR_IA32_PMC0 - MSR_IA32_PERFCTR0)
if (!msr_info->host_initiated && !(msr & MSR_PMC_FULL_WIDTH_BIT))
data = (s64)(s32)data;
Thanks, it looks good to me and I'll apply it.
+ case MSR_IA32_PERF_CAPABILITIES:
+ if (!nested)
+ return 1;
+ msr->data = vmx_get_perf_capabilities();
+ return 0;
The !nested check is wrong.
You're absolutely right about this.
+++ b/arch/x86/kvm/x86.c
@@ -1220,6 +1220,13 @@ static const u32 msrs_to_save_all[] = {
MSR_ARCH_PERFMON_EVENTSEL0 + 12, MSR_ARCH_PERFMON_EVENTSEL0 + 13,
MSR_ARCH_PERFMON_EVENTSEL0 + 14, MSR_ARCH_PERFMON_EVENTSEL0 + 15,
MSR_ARCH_PERFMON_EVENTSEL0 + 16, MSR_ARCH_PERFMON_EVENTSEL0 + 17,
+
+ MSR_IA32_PMC0, MSR_IA32_PMC0 + 1, MSR_IA32_PMC0 + 2,
+ MSR_IA32_PMC0 + 3, MSR_IA32_PMC0 + 4, MSR_IA32_PMC0 + 5,
+ MSR_IA32_PMC0 + 6, MSR_IA32_PMC0 + 7, MSR_IA32_PMC0 + 8,
+ MSR_IA32_PMC0 + 9, MSR_IA32_PMC0 + 10, MSR_IA32_PMC0 + 11,
+ MSR_IA32_PMC0 + 12, MSR_IA32_PMC0 + 13, MSR_IA32_PMC0 + 14,
+ MSR_IA32_PMC0 + 15, MSR_IA32_PMC0 + 16, MSR_IA32_PMC0 + 17,
};
This is not needed because the full-width content is already accessible
from the host via MSR_IA32_PERFCTRn.
That's true because they're just alias registers for MSR_IA32_PERFCTRn.
Given the bugs, it is clear that you should also modify the pmu.c
testcase for kvm-unit-tests to cover full-width writes (and especially
the non-full-width write behavior of MSR_IA32_PERFCTRn). Even before
the QEMU side is begin worked on, you can test it with "-cpu
host,migratable=off".
Sure, I added some testcases in pmu.c to cover this feature.
Please review the v3 patch
https://lore.kernel.org/kvm/20200508083218.120559-1-like...@linux.intel.com/
as well as the kvm-unit-tests testcase.
Thanks,
Like Xu
Thanks,
Paolo
