Re: [PATCH] target/i386: add -cpu,lbr=true support to enable guest LBR

["Xu, Like" <like.xu@intel.com>]

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@redhat.com>
>> Cc: Richard Henderson <rth@twiddle.net>
>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> Cc: Marcelo Tosatti <mtosatti@redhat.com>
>> Cc: qemu-devel@nongnu.org
>> Signed-off-by: Like Xu <like.xu@linux.intel.com>
>> ---
>>   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 ?

>
> 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.
>
> 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.

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.
>
>
>
>>               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.
>
>> +
>>   #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.
>
>>   
>>       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
>>