Re: [PATCH v4 1/6] target/i386: Don't save/restore PERF_GLOBAL_OVF_CTRL MSRs

[Fabiano Rosas <farosas@suse.de>]

Zide Chen <zide.chen@intel.com> writes:

> From: Dapeng Mi <dapeng1.mi@linux.intel.com>
>
> MSR_CORE_PERF_GLOBAL_OVF_CTRL and MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_CLR
> are write-only MSRs and reads always return zero.
>
> Saving and restoring these MSRs is therefore unnecessary.  Replace
> VMSTATE_UINT64 with VMSTATE_UNUSED in the VMStateDescription to ignore
> env.msr_global_ovf_ctrl during migration.  This avoids the need to bump
> version_id and does not introduce any migration incompatibility.
>
> Cc: Dongli Zhang <dongli.zhang@oracle.com>
> Cc: Sandipan Das <sandipan.das@amd.com>
> Fixes: e587632c228e ("target/i386/kvm: support perfmon-v2 for reset")
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> Co-developed-by: Zide Chen <zide.chen@intel.com>
> Signed-off-by: Zide Chen <zide.chen@intel.com>
> ---
> V3:
> - Remove MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_CLR as well.
> ---
>  target/i386/cpu.h     |  3 ---
>  target/i386/kvm/kvm.c | 10 ----------
>  target/i386/machine.c |  4 ++--
>  3 files changed, 2 insertions(+), 15 deletions(-)
>
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 6b500737c3be..ff44487d0b6d 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -507,11 +507,9 @@ typedef enum X86Seg {
>  #define MSR_CORE_PERF_FIXED_CTR_CTRL    0x38d
>  #define MSR_CORE_PERF_GLOBAL_STATUS     0x38e
>  #define MSR_CORE_PERF_GLOBAL_CTRL       0x38f
> -#define MSR_CORE_PERF_GLOBAL_OVF_CTRL   0x390
>  
>  #define MSR_AMD64_PERF_CNTR_GLOBAL_STATUS       0xc0000300
>  #define MSR_AMD64_PERF_CNTR_GLOBAL_CTL          0xc0000301
> -#define MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_CLR   0xc0000302
>  
>  #define MSR_K7_EVNTSEL0                 0xc0010000
>  #define MSR_K7_PERFCTR0                 0xc0010004
> @@ -2104,7 +2102,6 @@ typedef struct CPUArchState {
>      uint64_t msr_fixed_ctr_ctrl;
>      uint64_t msr_global_ctrl;
>      uint64_t msr_global_status;
> -    uint64_t msr_global_ovf_ctrl;
>      uint64_t msr_fixed_counters[MAX_FIXED_COUNTERS];
>      uint64_t msr_gp_counters[MAX_GP_COUNTERS];
>      uint64_t msr_gp_evtsel[MAX_GP_COUNTERS];
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index a29f757c168a..1ac1803e8a2e 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -4290,8 +4290,6 @@ static int kvm_put_msrs(X86CPU *cpu, KvmPutState level)
>              if (pmu_version > 1) {
>                  kvm_msr_entry_add(cpu, MSR_CORE_PERF_GLOBAL_STATUS,
>                                    env->msr_global_status);
> -                kvm_msr_entry_add(cpu, MSR_CORE_PERF_GLOBAL_OVF_CTRL,
> -                                  env->msr_global_ovf_ctrl);

Assuming there's no weird nested virtualization scenario and combination
of migration, resets and context-switching between L1/L2 that implicitly
relies on this (effective) clearing of the MSR to work properly.

Reviewed-by: Fabiano Rosas <farosas@suse.de>