Re: [PATCH v2] KVM: X86: Emulate APERF/MPERF to report actual vCPU frequency

From: Like Xu
Date: Fri Dec 31 2021 - 02:48:47 EST

Next message: Jiasheng Jiang: "[PATCH] media: vidtv: Check for null return of vzalloc"
Previous message: Paul Menzel: "Re: [PATCH v3 3/3] ahci: AMD A85 FCH (Hudson D4): Skip 200 ms debounce delay in `sata_link_resume()`"
In reply to: Jim Mattson: "Re: [PATCH v2] KVM: X86: Emulate APERF/MPERF to report actual vCPU frequency"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 31/12/2021 9:29 am, Jim Mattson wrote:

On Thu, Dec 30, 2021 at 12:37 AM Like Xu <like.xu.linux@xxxxxxxxx> wrote:

On 30/12/2021 10:36 am, Jim Mattson wrote:

On Wed, Dec 29, 2021 at 4:28 PM Jim Mattson <jmattson@xxxxxxxxxx> wrote:

On Tue, Dec 28, 2021 at 8:06 PM Like Xu <like.xu.linux@xxxxxxxxx> wrote:

Hi Jim,

Thanks for your detailed comments.

On 29/12/2021 9:11 am, Jim Mattson wrote:

On Wed, Dec 22, 2021 at 5:34 AM Like Xu <like.xu.linux@xxxxxxxxx> wrote:

From: Like Xu <likexu@xxxxxxxxxxx>

The aperf/mperf are used to report current CPU frequency after 7d5905dc14a.
But guest kernel always reports a fixed vCPU frequency in the /proc/cpuinfo,
which may confuse users especially when turbo is enabled on the host or
when the vCPU has a noisy high power consumption neighbour task.

Most guests such as Linux will only read accesses to AMPERF msrs, where
we can passthrough registers to the vcpu as the fast-path (a performance win)
and once any write accesses are trapped, the emulation will be switched to
slow-path, which emulates guest APERF/MPERF values based on host values.
In emulation mode, the returned MPERF msr value will be scaled according
to the TSCRatio value.

As a minimum effort, KVM exposes the AMPERF feature when the host TSC
has CONSTANT and NONSTOP features, to avoid the need for more code
to cover various coner cases coming from host power throttling transitions.

The slow path code reveals an opportunity to refactor update_vcpu_amperf()
and get_host_amperf() to be more flexible and generic, to cover more
power-related msrs.

Requested-by: Dongli Cao <caodongli@xxxxxxxxxxxx>
Requested-by: Li RongQing <lirongqing@xxxxxxxxx>
Signed-off-by: Like Xu <likexu@xxxxxxxxxxx>
---
v1 -> v2 Changelog:
- Use MSR_TYPE_R to passthrough as a fast path;
- Use [svm|vmx]_set_msr for emulation as a slow path;
- Interact MPERF with TSC scaling (Jim Mattson);
- Drop bool hw_coord_fb_cap with cpuid check;
- Add TSC CONSTANT and NONSTOP cpuid check;
- Duplicate static_call(kvm_x86_run) to make the branch predictor happier;

Previous:
https://lore.kernel.org/kvm/20200623063530.81917-1-like.xu@xxxxxxxxxxxxxxx/

arch/x86/include/asm/kvm_host.h | 12 +++++
arch/x86/kvm/cpuid.c | 3 ++
arch/x86/kvm/cpuid.h | 22 +++++++++
arch/x86/kvm/svm/svm.c | 15 ++++++
arch/x86/kvm/svm/svm.h | 2 +-
arch/x86/kvm/vmx/vmx.c | 18 ++++++-
arch/x86/kvm/x86.c | 85 ++++++++++++++++++++++++++++++++-
7 files changed, 153 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index ce622b89c5d8..1cad3992439e 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -39,6 +39,8 @@

#define KVM_MAX_VCPUS 1024

+#define KVM_MAX_NUM_HWP_MSR 2
+
/*
* In x86, the VCPU ID corresponds to the APIC ID, and APIC IDs
* might be larger than the actual number of VCPUs because the
@@ -562,6 +564,14 @@ struct kvm_vcpu_hv_stimer {
bool msg_pending;
};

+/* vCPU thermal and power context */
+struct kvm_vcpu_hwp {
+ bool fast_path;
+ /* [0], APERF msr, increases with the current/actual frequency */
+ /* [1], MPERF msr, increases with a fixed frequency */

According to the SDM, volume 3, section 18.7.2,
* The TSC, IA32_MPERF, and IA32_FIXED_CTR2 operate at close to the
maximum non-turbo frequency, which is equal to the product of scalable
bus frequency and maximum non-turbo ratio.

For AMD, it will be the P0 frequency.

It's important to note that IA32_MPERF operates at close to the same
frequency of the TSC. If that were not the case, your comment
regarding IA32_APERF would be incorrect.

Yes, how does this look:

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index f8f978bc9ec3..d422bf8669ca 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -568,7 +568,7 @@ struct kvm_vcpu_hv_stimer {
struct kvm_vcpu_hwp {
bool fast_path;
/* [0], APERF msr, increases with the current/actual frequency */
- /* [1], MPERF msr, increases with a fixed frequency */
+ /* [1], MPERF msr, increases at the same fixed frequency as the TSC */
u64 msrs[KVM_MAX_NUM_HWP_MSR];
};

That looks fine from the Intel perspective. (Note that I have not
looked at AMD's documentation yet.)

FYI, AMD has something like {A,M}perfReadOnly msrs
while it‘s not friendly to the millions legacy guests.

To be honest, if you're going to enlighten the guests that use this
functionality, I can't see any reason to preserve IA32_MPERF or to
provide AMD's MPERF_RD_ONLY MSR. With an invariant TSC, these MSRs
seem to be completely redundant. Obviously, if you're going to

The read-only variants looks very promising for guest use.

virtualize CPUID.06H:ECX[0], then you have to provide IA32_MPERF. It
is unclear from AMD's documentation if CPUID.06H:ECX[0] implies the

The read-only variant was made public in March 2021, along with
CPUID Fn8000_0007_EDX[bit 10, EffFreqRO].

existence of the read-only variants in AMD's MSR space as well as the
standard IA32_MPERF and IA32_APERF MSRs in Intel's MSR space.

Yes. As early practice, I'm trying to advertise CPUID.06H:ECX[0] for both
AMD and Intel. As you point out, there are many opens with this version.

For example, suppose that the TSC frequency were 2.0 GHz, the
current/actual frequency were 2.2 GHz, and the IA32_MPERF frequency
were 133 MHz. In that case, the IA32_APERF MSR would increase at 146.3
MHz.

+ u64 msrs[KVM_MAX_NUM_HWP_MSR];
+};
+
/* Hyper-V synthetic interrupt controller (SynIC)*/
struct kvm_vcpu_hv_synic {
u64 version;
@@ -887,6 +897,8 @@ struct kvm_vcpu_arch {
/* AMD MSRC001_0015 Hardware Configuration */
u64 msr_hwcr;

+ struct kvm_vcpu_hwp hwp;
+
/* pv related cpuid info */
struct {
/*
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 0b920e12bb6d..e20e5e8c2b3a 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -739,6 +739,9 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
entry->eax = 0x4; /* allow ARAT */
entry->ebx = 0;
entry->ecx = 0;
+ /* allow aperf/mperf to report the true vCPU frequency. */
+ if (kvm_cpu_cap_has_amperf())
+ entry->ecx |= (1 << 0);
entry->edx = 0;
break;
/* function 7 has additional index. */
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index c99edfff7f82..741949b407b7 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -154,6 +154,28 @@ static inline int guest_cpuid_stepping(struct kvm_vcpu *vcpu)
return x86_stepping(best->eax);
}

+static inline bool kvm_cpu_cap_has_amperf(void)
+{
+ return boot_cpu_has(X86_FEATURE_APERFMPERF) &&
+ boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
+ boot_cpu_has(X86_FEATURE_NONSTOP_TSC);
+}
+
+static inline bool guest_support_amperf(struct kvm_vcpu *vcpu)
+{
+ struct kvm_cpuid_entry2 *best;
+
+ if (!kvm_cpu_cap_has_amperf())
+ return false;
+
+ best = kvm_find_cpuid_entry(vcpu, 0x6, 0);
+ if (!best || !(best->ecx & 0x1))
+ return false;
+
+ best = kvm_find_cpuid_entry(vcpu, 0x80000007, 0);
+ return best && (best->edx & (1 << 8));

Nit: Use BIT().

Applied.

+}
+
static inline bool guest_has_spec_ctrl_msr(struct kvm_vcpu *vcpu)
{
return (guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL) ||
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 5557867dcb69..2873c7f132bd 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -114,6 +114,8 @@ static const struct svm_direct_access_msrs {
{ .index = MSR_EFER, .always = false },
{ .index = MSR_IA32_CR_PAT, .always = false },
{ .index = MSR_AMD64_SEV_ES_GHCB, .always = true },
+ { .index = MSR_IA32_MPERF, .always = false },
+ { .index = MSR_IA32_APERF, .always = false },
{ .index = MSR_INVALID, .always = false },
};

@@ -1218,6 +1220,12 @@ static inline void init_vmcb_after_set_cpuid(struct kvm_vcpu *vcpu)
/* No need to intercept these MSRs */
set_msr_interception(vcpu, svm->msrpm, MSR_IA32_SYSENTER_EIP, 1, 1);
set_msr_interception(vcpu, svm->msrpm, MSR_IA32_SYSENTER_ESP, 1, 1);
+
+ if (guest_support_amperf(vcpu)) {
+ set_msr_interception(vcpu, svm->msrpm, MSR_IA32_MPERF, 1, 0);
+ set_msr_interception(vcpu, svm->msrpm, MSR_IA32_APERF, 1, 0);
+ vcpu->arch.hwp.fast_path = true;
+ }
}
}

@@ -3078,6 +3086,13 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
svm->msr_decfg = data;
break;
}
+ case MSR_IA32_APERF:
+ case MSR_IA32_MPERF:
+ if (vcpu->arch.hwp.fast_path) {
+ set_msr_interception(vcpu, svm->msrpm, MSR_IA32_MPERF, 0, 0);
+ set_msr_interception(vcpu, svm->msrpm, MSR_IA32_APERF, 0, 0);
+ }
+ return kvm_set_msr_common(vcpu, msr);
default:
return kvm_set_msr_common(vcpu, msr);
}
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 9f153c59f2c8..ad4659811620 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -27,7 +27,7 @@
#define IOPM_SIZE PAGE_SIZE * 3
#define MSRPM_SIZE PAGE_SIZE * 2

-#define MAX_DIRECT_ACCESS_MSRS 20
+#define MAX_DIRECT_ACCESS_MSRS 22
#define MSRPM_OFFSETS 16
extern u32 msrpm_offsets[MSRPM_OFFSETS] __read_mostly;
extern bool npt_enabled;
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 1d53b8144f83..8998042107d2 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -576,6 +576,9 @@ static bool is_valid_passthrough_msr(u32 msr)
case MSR_LBR_CORE_FROM ... MSR_LBR_CORE_FROM + 8:
case MSR_LBR_CORE_TO ... MSR_LBR_CORE_TO + 8:
/* LBR MSRs. These are handled in vmx_update_intercept_for_lbr_msrs() */
+ case MSR_IA32_MPERF:
+ case MSR_IA32_APERF:
+ /* AMPERF MSRs. These are passthrough when all access is read-only. */

Even if all accesses are read-only, these MSRs cannot be pass-through
when the 'Use TSC scaling' VM-execution control is set and the TSC
multiplier is anything other than 1.

If all accesses are read-only, rdmsr will not be trapped and in that case:

The value read is scaled by the TSCRatio value (MSR C000_0104h) for
guest reads, but the underlying counters are not affected. Reads in host
mode or writes to MPERF are not affected. [AMD APM 17.3.2]

It's nice of AMD to scale reads of IA32_MPERF. That certainly
simplifies the problem of virtualizing these MSRs. However, Intel is
not so kind.

What a pity. Maybe we can enable amperf for AMD guests as well as
Intel guests which has TSC multiplier is 1 as the first step.

Unfortunately, KVM_GET_SUPPORTED_CPUID is a device ioctl, so if you
declare support for CPUID.06H:ECX[0] there, then you have to support
it for all vCPU configurations on the host. Given the enormous

Thanks, that's a good rule for me to follow.

overhead of the slow path, perhaps KVM_GET_EMULATED_CPUID is a better
place to report it.

OK, I assume with KVM_GET_EMULATED_CPUID,
both slow and fast paths can be preserved.

Suppose, for example, that the vCPU has a TSC frequency of 2.2 GHz,
but it is running on a host with a TSC frequency of 2.0 GHz. The
effective IA32_MPERF frequency should be the same as the vCPU TSC
frequency (scaled by the TSC multiplier), rather than the host
IA32_MPERF frequency.

I guess that Intel's implementation will also imply the effect of
the TSC multiplier for guest reads. Please let me know if I'm wrong.

From the description of the "Use TSC scaling" VM-execution control in
Table 23-7: "This control determines whether executions of RDTSC,
executions of RDTSCP, and executions of RDMSR that read from the
IA32_TIME_STAMP_COUNTER MSR return a value modified by the TSC
multiplier field (see Section 23.6.5 and Section 24.3)."

If you want to scale guest reads of IA32_MPERF, you will have to
intercept them and perform the scaling in software.

I don't think slow-path-always is a good option for enablment and we could
probably request Intel to behave similarly for the IA32_MPERF guest reads.

Get that request in now, and you may see the feature in five or six
years! While it may be possible for Intel to do the scaling in a
microcode patch, I wouldn't hold my breath.

Uh, as an ex-employee, I definitely know what you mean.

return true;
}

@@ -2224,7 +2227,14 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
}
ret = kvm_set_msr_common(vcpu, msr_info);
break;
-
+ case MSR_IA32_APERF:
+ case MSR_IA32_MPERF:
+ if (vcpu->arch.hwp.fast_path) {
+ vmx_set_intercept_for_msr(vcpu, MSR_IA32_APERF, MSR_TYPE_RW, true);
+ vmx_set_intercept_for_msr(vcpu, MSR_IA32_MPERF, MSR_TYPE_RW, true);
+ }
+ ret = kvm_set_msr_common(vcpu, msr_info);
+ break;
default:
find_uret_msr:
msr = vmx_find_uret_msr(vmx, msr_index);
@@ -6928,6 +6938,12 @@ static int vmx_create_vcpu(struct kvm_vcpu *vcpu)
vmx_disable_intercept_for_msr(vcpu, MSR_CORE_C7_RESIDENCY, MSR_TYPE_R);
}

+ if (guest_support_amperf(vcpu)) {
+ vmx_disable_intercept_for_msr(vcpu, MSR_IA32_MPERF, MSR_TYPE_R);
+ vmx_disable_intercept_for_msr(vcpu, MSR_IA32_APERF, MSR_TYPE_R);
+ vcpu->arch.hwp.fast_path = true;
+ }
+
vmx->loaded_vmcs = &vmx->vmcs01;

if (cpu_need_virtualize_apic_accesses(vcpu)) {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 42bde45a1bc2..7a6355815493 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1376,6 +1376,8 @@ static const u32 msrs_to_save_all[] = {
MSR_F15H_PERF_CTL3, MSR_F15H_PERF_CTL4, MSR_F15H_PERF_CTL5,
MSR_F15H_PERF_CTR0, MSR_F15H_PERF_CTR1, MSR_F15H_PERF_CTR2,
MSR_F15H_PERF_CTR3, MSR_F15H_PERF_CTR4, MSR_F15H_PERF_CTR5,
+
+ MSR_IA32_APERF, MSR_IA32_MPERF,
};

static u32 msrs_to_save[ARRAY_SIZE(msrs_to_save_all)];
@@ -3685,6 +3687,16 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
return 1;
vcpu->arch.msr_misc_features_enables = data;
break;
+ case MSR_IA32_APERF:
+ case MSR_IA32_MPERF:
+ /* Ignore meaningless value overrides from user space.*/
+ if (msr_info->host_initiated)
+ return 0;

Without these meaningless overrides from userspace, how do we ensure
that the guest derives the correct IA32_APERF/IA32_MPERF ratio for a

The guest cares about the ratio of the two deltas rather than APERF/MPERF ratio.

Effective frequency = {(APERF − APERF_INIT) / (MPERF − MPERF_INIT)} * P0 frequency

My question was, "How do you ensure the deltas are correct when
APERF_INIT and MPERF_INIT are sampled before live migration and APERF
and MPERF are sampled after live migration?" (Using your equation
above.)

set of measurements that span a live migration? For that matter, how
do we ensure that the deltas are even positive?

Once we allow the user space to restore AMPERF msr values different from
the host values, the slow path will be walked and we try to avoid this kind
of case due to overhead, whatever for live migration or pCPU migration.

Nonetheless, your implementation does not work.

The fast path is a performance win and any exit-path approach
will break all the effort. The guests rely on statistical figures.

Yes, the fast path is a performance win. Unfortunately, it does not
implement the architectural specification.

If we do it as KVM_GET_EMULATED_CPUID, can we break it up a bit?

I would be hard-pressed to enumerate every x86 O/S that has been or
ever will be written, let alone make assertions about how they use
this feature.

Yes and no (in terms of various X86_FEATURE_HYPERVISOR usages).

For example, suppose that the VM has migrated from a host with an
IA32_MPERF value of 0x0000123456789abc to a host with an IA32_MPERF
value of 0x000000123456789a. If the guest sampled IA32_MPERF before
and after live migration, it would see the counter go backwards, which

Yes, it will happen since without more hints from KVM, the user space
can't be sure if the save/restore time is in the sample period of AMPERF.
And even worse, guest could manipulate reading order of the AMPERF.

The proposal is to *let it happen* because it causes no harm, in the meantime,
what the guest really cares about is the deltas ratio, not the accuracy of
individual msr values, and if the result in this sample is ridiculous, the guest
should go and pick the result from the next sample.

You do not get to define the architecture. The CPU vendors have
already done that. Your job is to adhere to the architectural
specification.

In principle I strongly agree.

As opposed to not having this feature, the end user is likely to accept
the occasional miss of a sample to trade with the performance devil.

Maybe we could add fault tolerance for AMPERF in the guest, something like
a retry mechnism or just discarding extreme values to follow statistical methods.

That sounds like a parairtual approach to me. There is nothing in the
architectural specification that suggests that such mechanisms are
necessary.

KVM doesn't reject the PV approach, does it?

Not at all. If you want to define this as a kvm paravirtual feature,
advertised in CPUID.40000001H, I would be open to letting you define
the specification the way you want.

We'd better not do that.

The good news is the robustness like Linux guest on this issue is appreciated.
(9a6c2c3c7a73ce315c57c1b002caad6fcc858d0f and more stuff)

Considering that the sampling period of amperf is relatively frequent compared
with the workload runtime and it statistically reports the right vCPU frequency,
do you think this meaningless proposal is acceptable or practicable ?

My opinion is that your proposal is unacceptable, but I am not a decision maker.

We do respect any comments in the community, especially yours in the context of TSC.
Thanks for your time and clear attitude.

TBH, I'm open to any better proposal, as a practice of "it's worth doing well".

For existing hardware features, doing it well begins by implementing
the specification. Once you have a working implementation, then you
can optimize.

OK, thanks for pointing it out!
With your detailed help, I'd like to try this solution first.

For new paravirtual features, doing it well essentially comes down to
developing a clean design that will stand the test of time and that
enlightened guests are likely to adopt.

should not happen.

+ if (!guest_support_amperf(vcpu))
+ return 1;
+ vcpu->arch.hwp.msrs[MSR_IA32_APERF - msr] = data;
+ vcpu->arch.hwp.fast_path = false;
+ break;
default:
if (kvm_pmu_is_valid_msr(vcpu, msr))
return kvm_pmu_set_msr(vcpu, msr_info);
@@ -4005,6 +4017,17 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
case MSR_K7_HWCR:
msr_info->data = vcpu->arch.msr_hwcr;
break;
+ case MSR_IA32_APERF:
+ case MSR_IA32_MPERF: {

]> + u64 value;

+
+ if (!msr_info->host_initiated && !guest_support_amperf(vcpu))
+ return 1;
+ value = vcpu->arch.hwp.msrs[MSR_IA32_APERF - msr_info->index];
+ msr_info->data = (msr_info->index == MSR_IA32_APERF) ? value :
+ kvm_scale_tsc(vcpu, value, vcpu->arch.tsc_scaling_ratio);

I think it makes more sense to perform the scaling before storing the
IA32_MPERF value in vcpu->arch.hwp.msrs[].

Emm, do you really need to add more instruction cycles in the each call
of update_vcpu_amperf() in the critical path vcpu_enter_guest(), since the
calls to kvm_get_msr_commom() are relatively sparse.

One possible alternative may be for kvm to take over the IA32_MPERF
and IA32_APERF MSRs on sched-in. That may result in less overhead.

For less overhead this seems true, but the amperf data belongs to the
last shced time slice, which violates accuracy.

I know it's not this easy, but as a strawman, suppose we did something
like the following:

Let me implement this idea and give you feedback in another mail thread.

At sched-in:
1. Save host APERF/MPERF values from the MSRs.
2. Load the "current" guest APERF/MPERF values into the MSRs (if the
vCPU configuration allows for unintercepted reads).

At sched-out:
1. Calculate the guest APERF/MPERF deltas for use in step 3.
2. Save the "current" guest APERF/MPERF values.
3. "Restore" the host APERF/MPERF values, but add in the deltas from step 1.

Without any writes to IA32_MPERF, I would expect these MSRs to be
synchronized across all logical processors, and the proposal above
would break that synchronization.

Will we get a functional error if we defer the kvm_scale_tsc() operation ?

If you accumulate IA32_MPERF cycles from multiple hosts with different
IA32_MPERF frequencies and you defer the kvm_scale_tsc operation,
then, yes, this is broken.

Yes, how about we defer it until before any steps leading to a change in TSC ?

+ break;
+ }
default:
if (kvm_pmu_is_valid_msr(vcpu, msr_info->index))
return kvm_pmu_get_msr(vcpu, msr_info);
@@ -9688,6 +9711,53 @@ void __kvm_request_immediate_exit(struct kvm_vcpu *vcpu)
}
EXPORT_SYMBOL_GPL(__kvm_request_immediate_exit);

+static inline void get_host_amperf(u64 msrs[])
+{
+ rdmsrl(MSR_IA32_APERF, msrs[0]);
+ rdmsrl(MSR_IA32_MPERF, msrs[1]);

Note that these RDMSRs should probably be appropriately fenced, to

I assume a fenceless version would work, as would arch_scale_freq_tick().
After all, both counters are moving fast between two rdmsr instructions
and there is also a strict margin of error as to which register is read first.

keep them from drifting apart in the OOO stream. On Intel, I believe
they should be preceded by "mfence; lfence" and followed by "lfence".

+}
+
+static inline u64 get_amperf_delta(u64 enter, u64 exit)
+{
+ if (likely(exit >= enter))
+ return exit - enter;
+
+ return ULONG_MAX - enter + exit;
+}
+
+static inline void update_vcpu_amperf(struct kvm_vcpu *vcpu, u64 adelta, u64 mdelta)
+{
+ u64 aperf_left, mperf_left, delta, tmp;
+
+ aperf_left = ULONG_MAX - vcpu->arch.hwp.msrs[0];
+ mperf_left = ULONG_MAX - vcpu->arch.hwp.msrs[1];
+
+ /* Fast path when neither MSR overflows */
+ if (adelta <= aperf_left && mdelta <= mperf_left) {
+ vcpu->arch.hwp.msrs[0] += adelta;
+ vcpu->arch.hwp.msrs[1] += mdelta;
+ return;
+ }
+
+ /* When either MSR overflows, both MSRs are reset to zero and continue to increment. */
+ delta = min(adelta, mdelta);
+ if (delta > aperf_left || delta > mperf_left) {
+ tmp = max(vcpu->arch.hwp.msrs[0], vcpu->arch.hwp.msrs[1]);
+ tmp = delta - (ULONG_MAX - tmp) - 1;
+ vcpu->arch.hwp.msrs[0] = tmp + adelta - delta;
+ vcpu->arch.hwp.msrs[1] = tmp + mdelta - delta;
+ return;
+ }

I don't believe that the math above is correct in the general case. It
appears to assume that the counters are running at the same frequency.

Are you saying that if the guest counter is not considered to be running
at the same frequency as the host, we need to wrap mdelta with
kvm_scale_tsc() to accumulate the mdelta difference for a vmentry/exit ?

No. I just think your math/logic is wrong. Consider the following example:

At time t0, IA32_MPERF is -1000, and IA32_APERF is -1999. At time t1,
IA32_MPERF and IA32_APERF are both 1. Even assuming a constant CPU
frequency between t0 and t1, the possible range of actual frequency
are from half the TSC frequency to double the TSC frequency,
exclusive. If IA32_APERF is counting at just over half the TSC
frequency, then IA32_MPERF will hit 0 first. In this case, at t1, the
MPERF delta will be 1001, and the APERF delta will be ~502. However,

Uh, you're right and I messed it up.

if IA32_APERF is counting at just under double the TSC frequency, then
IA32_APERF will hit 0 first, but just barely. In this case, at t1, the
MPERF delta will be ~1000, and the APERF delta will be 2000.

Your code only works in the latter case, where both IA32_APERF and
IA32_MPERF hit 0 at the same time. The fundamental problem is the
handling of the wrap-around in get_amperf_delta. You construct the

It's true and I have to rework this part.

With hwp.msrs[mperf] reset to 0, it is hard to emulate the current
value of hwp.msrs[aperf] and the value of aperf counter remaining.

How about we assume that this aperf counter increases uniformly
between the two calls to the get_host_amperf() ?

That's fine, but you still have no way of knowing which one hit 0
first. Now, if you sampled TSC as well, then you'd be able to tell.

It looks good to me.

Please note AMD doesn't have this kind of interlocking.

Wrap-around shouldn't really be an issue, unless someone writes an
unusually high value to one of these MSRs.

The tricky part is that the amperf msrs are writable:

[AMD]
aperf from 281474962776165 to 281474962906694
mperf from 18446744073709486893 to 157152
ratio = (281474962906694 - 281474962776165) / (157152 - 18446744073709486893)

[Intel]
aperf from 281474962775717 to 60192
mperf from 18446744073709486476 to 49329
ratio = (60192 - 281474962775717) / (49329 - 18446744073709486476)

Current host amperf driver do not appear to
sense and handle this behavioural difference.

wrap-around delta as if the counter went all the way to ULONG_MAX
before being reset to 0, yet, we know that one of the counters is not
likely to have made it that far.

The whole point of this exercise is that the counters do not always
run at the same frequency.

+
+ if (mdelta > adelta && mdelta > aperf_left) {
+ vcpu->arch.hwp.msrs[0] = 0;
+ vcpu->arch.hwp.msrs[1] = mdelta - mperf_left - 1;
+ } else {
+ vcpu->arch.hwp.msrs[0] = adelta - aperf_left - 1;
+ vcpu->arch.hwp.msrs[1] = 0;
+ }

I don't understand this code at all. It seems quite unlikely that you

The value of two msr's will affect the other when one overflows:

* When either MSR overflows, both MSRs are reset to zero and
continue to increment. [Intel SDM, CHAPTER 14, 14.2]

are ever going to catch a wraparound at just the right point for one
of the MSRs to be 0. Moreover, since the two counters are not counting
the same thing, it doesn't seem likely that it would ever be correct
to derive the guest's IA32_APERF value from IA32_MPERF or vice versa.

+}
+
/*
* Returns 1 to let vcpu_run() continue the guest execution loop without
* exiting to the userspace. Otherwise, the value will be returned to the
@@ -9700,7 +9770,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
dm_request_for_irq_injection(vcpu) &&
kvm_cpu_accept_dm_intr(vcpu);
fastpath_t exit_fastpath;
-
+ u64 before[2], after[2];
bool req_immediate_exit = false;

/* Forbid vmenter if vcpu dirty ring is soft-full */
@@ -9942,7 +10012,16 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
*/
WARN_ON_ONCE(kvm_apicv_activated(vcpu->kvm) != kvm_vcpu_apicv_active(vcpu));

- exit_fastpath = static_call(kvm_x86_run)(vcpu);
+ if (likely(vcpu->arch.hwp.fast_path)) {
+ exit_fastpath = static_call(kvm_x86_run)(vcpu);
+ } else {
+ get_host_amperf(before);
+ exit_fastpath = static_call(kvm_x86_run)(vcpu);
+ get_host_amperf(after);
+ update_vcpu_amperf(vcpu, get_amperf_delta(before[0], after[0]),
+ get_amperf_delta(before[1], after[1]));
+ }
+

The slow path is awfully expensive here. Shouldn't there also be an
option to do none of this, if the guest doesn't advertise CPUID.06H:
ECX[0]?

Yes, it looks pretty good to me and let me figure it out.

Your slow path seems fundamentally broken, in that IA32_MPERF only
counts while the vCPU thread is running. It should count all of the
time, just as the guest TSC does. For example, we offer a low-cost VM
that is throttled to run at most 50% of the time. Anyone looking at

I'm not sure if the "50% throttled" is equivalent to two vCPUs on one pCPU [1].

Effectively, it is the same.

the APERF/MPERF ratio for such a VM should see the 50% duty cycle
reflected as IA32_APERF advancing at half the frequency of IA32_MPERF.
However, if IA32_MPERF only advances when the vCPU thread is running,
the apparent performance will be inflated by 2x.

Actually, your fast path is similarly broken, in that IA32_APERF
should only count while the vCPU is running (or at least scheduled).
As it stands, the 50% duty cycle VM will get an inflated APERF/MPERF
ratio using the fast path, because it will be credited APERF cycles
while it is descheduled and other tasks are running. Per section

For fast-path, I have reproduced this with the pCPU oversubscription condition.

I'm not sure we can make it work completely,
or have to make it a restriction on the use of this feature.

If you find any more design flaws, please let me know.

As is often the case with performance monitoring, it is unclear to me
exactly what the semantics should be for APERF cycles in a virtualized
environment.

Yes, my other hand is working on the conjecture about guest un-halted cycles.

From the overcommit scenario, I think it is clear that the host's
APERF cycles should be allocated to at most one guest, and not
replicated for every vCPU thread sharing a logical processor. I think
it is also clear that APERF cycles accumulated while in VMX non-root
mode should be allocated to the running vCPU.

We're on the same page about this.

Beyond that, I have some uncertainty.

If a totally unrelated process, like 'ls' executes on the same logical
processor, I assume that its accumulated APERF cycles should not be
allocated to any VM. This leads me to think that, at most, the APERF
cycles between a vCPU thread's sched-in and its sched-out should be
allocated to that vCPU.

We have to answer the question that how much damage this does
to accuracy if we take the above sched-in/out approach.

Now come some harder questions. If the vCPU thread is executing CPUID
in a loop, should all of the APERF cycles spent transitioning into and
out of VMX non-root mode be allocated to the vCPU?

I would say yes.

What about the cycles spent emulating the CPUID instruction in kvm?

How about just the cycles of static_call(kvm_x86_run)(vcpu) is accumulated ?

Should they all > be allocated to the vCPU, or will that leave the guest with an
inflated idea of performance?

A littile bit inflation due to the loss of instruction level isolation.

What if "enable_unrestricted_guest" is
false, and we spend a lot of time emulating real-address mode in the
guest? Should those APERF cycles be allocated to the vCPU? Similarly,
what if we're running a nested guest, and all of the VMX instructions
in L1 have to be emulated by kvm? Should all of those APERF cycles be
allocated to the vCPU?

I hadn't thought about these last scenarios.
It's a very good starting point for brainstorming.

Clearly, there are some tradeoffs to be made between accuracy and performance.

We had to rethink the architectural definition of the non-root mode TSC/APEFF/MPERF/PMC
cycles in the KVM conetext, ensuring that our code implementation does not compromise
the programming expectations of any guests ideally.

What, exactly, are the requirements here?

Cc more people to get more input.

14.5.5 of the SDM, "The IA32_APERF counter does not count during
forced idle state." A vCPU thread being descheduled is the virtual
machine equivalent of a logical processor being forced idle by HDC.

if (likely(exit_fastpath != EXIT_FASTPATH_REENTER_GUEST))
break;

@@ -11138,6 +11217,8 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
vcpu->arch.xcr0 = XFEATURE_MASK_FP;
}

+ memset(vcpu->arch.hwp.msrs, 0, sizeof(vcpu->arch.hwp.msrs));
+
/* All GPRs except RDX (handled below) are zeroed on RESET/INIT. */
memset(vcpu->arch.regs, 0, sizeof(vcpu->arch.regs));
kvm_register_mark_dirty(vcpu, VCPU_REGS_RSP);
--
2.33.1

Next message: Jiasheng Jiang: "[PATCH] media: vidtv: Check for null return of vzalloc"
Previous message: Paul Menzel: "Re: [PATCH v3 3/3] ahci: AMD A85 FCH (Hudson D4): Skip 200 ms debounce delay in `sata_link_resume()`"
In reply to: Jim Mattson: "Re: [PATCH v2] KVM: X86: Emulate APERF/MPERF to report actual vCPU frequency"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]