Re: [PATCH] i386: Disable BTS and PEBS

[Like Xu <like.xu.linux@gmail.com>]

On 18/7/2022 11:22 am, Zhenzhong Duan wrote:
> Since below KVM commit, KVM hided BTS as it's not supported yet.
> b9181c8ef356 ("KVM: x86/pmu: Avoid exposing Intel BTS feature")
> 
> After below KVM commit, it gave control of MSR_IA32_MISC_ENABLES to userspace.
> 9fc222967a39 ("KVM: x86: Give host userspace full control of MSR_IA32_MISC_ENABLES")
> 
> So qemu takes the responsibility to hide BTS.
> Without fix, we get below warning in guest kernel:
> 
> [] unchecked MSR access error: WRMSR to 0x1d9 (tried to write 0x00000000000001c0) at rIP: 0xffffffffaa070644 (native_write_msr+0x4/0x20)
> [] Call Trace:
> []  <TASK>
> []  intel_pmu_enable_bts+0x5d/0x70
> []  bts_event_add+0x77/0x90
> []  event_sched_in.isra.135+0x99/0x1e0
> 
> Tested-by: Xiangfei Ma <xiangfeix.ma@intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   target/i386/cpu.h     | 6 ++++--
>   target/i386/kvm/kvm.c | 4 ++++
>   2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 82004b65b944..8a83d0995c66 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -434,8 +434,10 @@ typedef enum X86Seg {
>   
>   #define MSR_IA32_MISC_ENABLE            0x1a0
>   /* Indicates good rep/movs microcode on some processors: */
> -#define MSR_IA32_MISC_ENABLE_DEFAULT    1
> -#define MSR_IA32_MISC_ENABLE_MWAIT      (1ULL << 18)
> +#define MSR_IA32_MISC_ENABLE_DEFAULT      1
> +#define MSR_IA32_MISC_ENABLE_BTS_UNAVAIL  (1ULL << 11)
> +#define MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL (1ULL << 12)
> +#define MSR_IA32_MISC_ENABLE_MWAIT        (1ULL << 18)
>   
>   #define MSR_MTRRphysBase(reg)           (0x200 + 2 * (reg))
>   #define MSR_MTRRphysMask(reg)           (0x200 + 2 * (reg) + 1)
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index f148a6d52fa4..002e0520dd76 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -2180,6 +2180,10 @@ void kvm_arch_reset_vcpu(X86CPU *cpu)
>   {
>       CPUX86State *env = &cpu->env;
>   
> +    /* Disable BTS feature which is unsupported on KVM */
> +    env->msr_ia32_misc_enable |= MSR_IA32_MISC_ENABLE_BTS_UNAVAIL;
> +    env->msr_ia32_misc_enable |= MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL;

Would it be more readable to group msr_ia32_misc_enable code into this function:

	static void x86_cpu_reset(DeviceState *dev)

and, why disable PEBS (we need it at least for "-cpu host,migratable=no") ?

Also, the behavior of MISC_ENABLE_EMON is also inconsistent with "pmu=off”.

> +
>       env->xcr0 = 1;
>       if (kvm_irqchip_in_kernel()) {
>           env->mp_state = cpu_is_bsp(cpu) ? KVM_MP_STATE_RUNNABLE :