Hi Eduardo,
On 2020/9/28 23:41, Eduardo Habkost wrote:
On Mon, Sep 28, 2020 at 10:51:03PM +0800, Xu, Like wrote:
Hi Eduardo,
Thanks for your detailed review.
On 2020/9/25 6:05, Eduardo Habkost wrote:
I've just noticed this on my review queue (apologies for the long
delay). Comments below:
On Sun, Jul 26, 2020 at 11:32:20PM +0800, Like Xu wrote:
The LBR feature would be enabled on the guest if:
- the KVM is enabled and the PMU is enabled and,
- the msr-based-feature IA32_PERF_CAPABILITIES is supporterd and,
- the supported returned value for lbr_fmt from this msr is not zero.
The LBR feature would be disabled on the guest if:
- the msr-based-feature IA32_PERF_CAPABILITIES is unsupporterd OR,
- qemu set the IA32_PERF_CAPABILITIES msr feature without lbr_fmt values OR,
- the requested guest vcpu model doesn't support PDCM.
Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
Cc: Richard Henderson <rth@xxxxxxxxxxx>
Cc: Eduardo Habkost <ehabkost@xxxxxxxxxx>
Cc: "Michael S. Tsirkin" <mst@xxxxxxxxxx>
Cc: Marcel Apfelbaum <marcel.apfelbaum@xxxxxxxxx>
Cc: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
Cc: qemu-devel@xxxxxxxxxx
Signed-off-by: Like Xu <like.xu@xxxxxxxxxxxxxxx>
---
hw/i386/pc.c | 1 +
target/i386/cpu.c | 24 ++++++++++++++++++++++--
target/i386/cpu.h | 2 ++
target/i386/kvm.c | 7 ++++++-
4 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3d419d5991..857aff75bb 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -318,6 +318,7 @@ GlobalProperty pc_compat_1_5[] = {
{ "Nehalem-" TYPE_X86_CPU, "min-level", "2" },
{ "virtio-net-pci", "any_layout", "off" },
{ TYPE_X86_CPU, "pmu", "on" },
+ { TYPE_X86_CPU, "lbr", "on" },
Why is this line here?
I'll remove it.
{ "i440FX-pcihost", "short_root_bus", "0" },
{ "q35-pcihost", "short_root_bus", "0" },
};
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 588f32e136..c803994887 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1142,8 +1142,8 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
[FEAT_PERF_CAPABILITIES] = {
.type = MSR_FEATURE_WORD,
.feat_names = {
- NULL, NULL, NULL, NULL,
- NULL, NULL, NULL, NULL,
+ "lbr-fmt-bit-0", "lbr-fmt-bit-1", "lbr-fmt-bit-2", "lbr-fmt-bit-3",
+ "lbr-fmt-bit-4", "lbr-fmt-bit-5", NULL, NULL,
What about a separate "lbr-fmt" int property instead of
individual bit properties?
I'm not sure if you mean adding a "separate lbr-fmt int property"
like "uint64_t tcg_features" to 'struct FeatureWordInfo'.
Would you mind providing more implementation hints,
considering the PEBS_FMT will be added later ?
You can add a regular uint8_t field to X86CPU, use
DEFINE_PROP_UINT8 at x86_cpu_properties[], and just validate/copy
the bits to cpu->features[FEAT_PERF_CAPABILITIES][bits 0:5] on
x86_cpu_realizefn().
Thanks, I'll apply it and enable "-cpu,lbr-fmt=*" from command line.
What happens if LBR_FMT on the host (returned by
kvm_arch_get_supported_msr_feature(MSR_IA32_PERF_CAPABILITIES) is
different than the one configured for the guest?
To enable guest LBR, guest LBR_FMT must be the same as host LBR_FMT.
Can KVM emulate
a CPU with different LBR_FMT, or it must match the host?
It must match the host since the LBR registers are model specified.
OK, this means the value set in cpu->features[] need to be
validated against the host in x86_cpu_filter_features().
It can be similar to what's done for intel-pt bits, but instead
of comparing to constants (the intel-pt bits in CPUID are
constant today), you can compare the host value with
cpu->features[FEAT_PERF_CAPABILITIES].
I assume you mean
env->features[FEAT_PERF_CAPABILITIES]
for
cpu->features[FEAT_PERF_CAPABILITIES].
Thanks, I'll apply it.
Maybe a FeatureWordInfo.validate_feature(X86CPU *, FeatureWord)
callback could be added, so we could just define separate
validation functions for each feature word, to be called
automatically by x86_cpu_filter_features(). This could be done
as a follow-up patch, though.
Sure, let me see if there is extra value in adding separate
verification functions for each feature word.
If LBR_FMT must always match the host, the feature needs to block
live migration.
It's migrable enough of the perf cap LBR version matches,
don't need full model number match.
As long as the requirements are validated inside
x86_cpu_filter_features(), it should be OK to make it migratable.
Thanks.
For example it's fine to migrate from SKY to CLX.
I guess this is already the case because PDCM is
cleared if !cpu->enable_pmu. Adding PDCM to .unmigratable_flags
is probably a good idea, though.
I'm trying to make LBR migration-friendly as much as possible w/ your help.
If Arch LBR is enabled for SPR guest, the situation will be different
hence adding PDCM to .unmigratable_flags may not help it.
OK, in this case forget what I said about setting it on
.unmigratable_flags. The constraints for making the feature
migratable should be same ones mentioned for intel-pt at:
https://lore.kernel.org/qemu-devel/20200923141502.GO2044576@xxxxxxxxxxx/
Thanks for clarification.
Please let me know if the v2 patch doesn't follow you guide:
https://lore.kernel.org/qemu-devel/20200929061217.118440-1-like.xu@xxxxxxxxxxxxxxx/
NULL, NULL, NULL, NULL,
NULL, "full-width-write", NULL, NULL,
NULL, NULL, NULL, NULL,
@@ -4224,6 +4224,12 @@ static bool lmce_supported(void)
return !!(mce_cap & MCG_LMCE_P);
}
+static inline bool lbr_supported(void)
+{
+ return kvm_enabled() && (kvm_arch_get_supported_msr_feature(kvm_state,
+ MSR_IA32_PERF_CAPABILITIES) & PERF_CAP_LBR_FMT);
+}
You can rewrite this is an accelerator-independent way as:
(x86_cpu_get_supported_feature_word(FEAT_PERF_CAPABILITIES) & PERF_CAP_LBR_FMT)
Thanks, I'll apply it.
However, is this really supposed to return false if LBR_FMT is 000000?
I think it's fine to return false if LBR_FMT is 000000.
Don't we want to support hosts that have PDCM
(CPUID[1].ECX[bit 15]) = 1 and
IA32_PERF_CAPABILITIES.LBR_FMT[bits 5:0] = 000000 ?
Real hardware may always has PDCM and non-zero LBR_FMT.
If we are talking about supporting guest with PDCM and zero LBR_FMT,
it has been supported due to the present of "full-width-write" bit.
+
#define CPUID_MODEL_ID_SZ 48
/**
@@ -4327,6 +4333,9 @@ static void max_x86_cpu_initfn(Object *obj)
}
object_property_set_bool(OBJECT(cpu), "pmu", true, &error_abort);
+ if (lbr_supported()) {
+ object_property_set_bool(OBJECT(cpu), "lbr", true, &error_abort);
Why is this necessary?
If kvm_arch_get_supported_msr_feature(MSR_IA32_PERF_CAPABILITIES)
return the PERF_CAP_LBR_FMT bits set,
x86_cpu_get_supported_feature_word() will return those bits, and
they will be automatically set at
env->features[FEAT_PERF_CAPABILITIES].
Thanks, I'll remove it.
+ }
}
static const TypeInfo max_x86_cpu_type_info = {
@@ -5535,6 +5544,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
}
if (!cpu->enable_pmu) {
*ecx &= ~CPUID_EXT_PDCM;
+ if (cpu->enable_lbr) {
+ warn_report("LBR is unsupported since guest PMU is disabled.");
+ exit(1);
+ }
}
break;
case 2:
@@ -6553,6 +6566,12 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
}
}
+ if (!cpu->max_features && cpu->enable_lbr &&
Why do we need to check for !cpu->max_features here?
I'll remove it.
+ !(env->features[FEAT_1_ECX] & CPUID_EXT_PDCM)) {
+ warn_report("requested vcpu model doesn't support PDCM for LBR.");
+ exit(1);
Please report errors using error_setg(errp, ...) instead.
I'll apply it.
+ }
+
if (cpu->ucode_rev == 0) {
/* The default is the same as KVM's. */
if (IS_AMD_CPU(env)) {
@@ -7187,6 +7206,7 @@ static Property x86_cpu_properties[] = {
#endif
DEFINE_PROP_INT32("node-id", X86CPU, node_id, CPU_UNSET_NUMA_NODE_ID),
DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false),
+ DEFINE_PROP_BOOL("lbr", X86CPU, enable_lbr, false),
When exactly do we want to set lbr=off explicitly? What's the
expected outcome when lbr=off?
We set pmu=off explicitly, so does lbr=off.
When set lbr=off, the LBR-related registers accesses from guest bring #GP
and expected outcome is just like pmu=off.
How are those registers enumerated? Maybe I'm looking at an
outdated version of the Intel SDM or I couldn't find the right
section.
For model-specified LBR, please refer to:
17.4 LAST BRANCH, INTERRUPT, AND EXCEPTION RECORDING OVERVIEW
For Architecture LBR, please refer to:
CHAPTER 7 ARCHITECTURAL LAST BRANCH RECORDS (LBRS)
in the "JUNE 2020" of Intel® Architecture Instruction Set Extensions \
and Future Features Programming Reference
Or, you can also ask me for any relevant details.
Thanks,
Like Xu
DEFINE_PROP_UINT32("hv-spinlocks", X86CPU, hyperv_spinlock_attempts,
HYPERV_SPINLOCK_NEVER_RETRY),
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index e1a5c174dc..a059913e26 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -357,6 +357,7 @@ typedef enum X86Seg {
#define ARCH_CAP_TSX_CTRL_MSR (1<<7)
#define MSR_IA32_PERF_CAPABILITIES 0x345
+#define PERF_CAP_LBR_FMT 0x3f
#define MSR_IA32_TSX_CTRL 0x122
#define MSR_IA32_TSCDEADLINE 0x6e0
@@ -1702,6 +1703,7 @@ struct X86CPU {
* capabilities) directly to the guest.
*/
bool enable_pmu;
+ bool enable_lbr;
This is a good place to document what enable_lbr=true|false
means (see questions above).
I'll document it here.
/* LMCE support can be enabled/disabled via cpu option 'lmce=on/off'. It is
* disabled by default to avoid breaking migration between QEMU with
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index b8455c89ed..feb33d5472 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -2690,8 +2690,10 @@ static void kvm_msr_entry_add_perf(X86CPU *cpu, FeatureWordArray f)
uint64_t kvm_perf_cap =
kvm_arch_get_supported_msr_feature(kvm_state,
MSR_IA32_PERF_CAPABILITIES);
-
if (kvm_perf_cap) {
+ if (!cpu->enable_lbr) {
+ kvm_perf_cap &= ~PERF_CAP_LBR_FMT;
+ }
Why is this necessary? If enable_lbr is false,
f[FEAT_PERF_CAPABILITIES] should not have those bits set at all.
I'll remove it.
kvm_msr_entry_add(cpu, MSR_IA32_PERF_CAPABILITIES,
kvm_perf_cap & f[FEAT_PERF_CAPABILITIES]);
}
@@ -2731,6 +2733,9 @@ static void kvm_init_msrs(X86CPU *cpu)
if (has_msr_perf_capabs && cpu->enable_pmu) {
kvm_msr_entry_add_perf(cpu, env->features);
+ } else if (!has_msr_perf_capabs && cpu->enable_lbr) {
+ warn_report("KVM doesn't support MSR_IA32_PERF_CAPABILITIES for LBR.");
+ exit(1);
This is not the appropriate place to check for unsupported
features. x86_cpu_realizefn() and/or x86_cpu_filter_features()
is.
Thanks, I'll apply it in the x86_cpu_filter_features().
Please let me if you have more comments.
Thanks,
Like Xu
}
if (has_msr_ucode_rev) {
--
2.21.3
