Re: [Qemu-devel] [PATCH] kvm: migrate vPMU state

[Andreas Färber]

Am 25.07.2013 12:14, schrieb Gleb Natapov:
> On Wed, Jul 24, 2013 at 05:01:15PM +0200, Andreas Färber wrote:
>> Am 24.07.2013 16:37, schrieb Paolo Bonzini:
>>> This requires kernel 3.10 but it is otherwise quite simple to do.
>>> The kernel pays attention to MSRs writes that are host initiated, and
>>> disables all side effects of the PMU registers (e.g. the global status
>>> MSR can be written and global overflow control MSR does not clear bits
>>> in the global status MSR).
>>>
>>> Only two bits are interesting.  First, the number of general-purpose
>>> counters must be fetched from CPUID so that we do not read non-existent
>>> MSRs.  It need not be part of the migration stream.
>>>
>>> Second, to avoid any possible side effects during the setting of MSRs
>>> I stop the PMU while setting the counters and event selector MSRs.
>>> Stopping the PMU snapshots the counters and ensures that no strange
>>> races can happen if the counters were saved close to their overflow
>>> value.
>>>
>>> Signed-off-by: Paolo Bonzini <address@hidden>
>>> ---
>>>  target-i386/cpu.h     | 23 +++++++++++++
>>>  target-i386/kvm.c     | 93 
>>> ++++++++++++++++++++++++++++++++++++++++++++++++---
>>>  target-i386/machine.c | 44 ++++++++++++++++++++++++
>>>  3 files changed, 155 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
>>> index 058c57f..522eed4 100644
>>> --- a/target-i386/cpu.h
>>> +++ b/target-i386/cpu.h
>>> @@ -304,6 +304,8 @@
>>>  #define MSR_TSC_ADJUST                  0x0000003b
>>>  #define MSR_IA32_TSCDEADLINE            0x6e0
>>>  
>>> +#define MSR_P6_PERFCTR0                    0xc1
>>> +
>>>  #define MSR_MTRRcap                        0xfe
>>>  #define MSR_MTRRcap_VCNT           8
>>>  #define MSR_MTRRcap_FIXRANGE_SUPPORT       (1 << 8)
>>> @@ -317,6 +319,8 @@
>>>  #define MSR_MCG_STATUS                  0x17a
>>>  #define MSR_MCG_CTL                     0x17b
>>>  
>>> +#define MSR_P6_EVNTSEL0                    0x186
>>> +
>>>  #define MSR_IA32_PERF_STATUS            0x198
>>>  
>>>  #define MSR_IA32_MISC_ENABLE               0x1a0
>>> @@ -342,6 +346,14 @@
>>>  
>>>  #define MSR_MTRRdefType                    0x2ff
>>>  
>>> +#define MSR_CORE_PERF_FIXED_CTR0   0x309
>>> +#define MSR_CORE_PERF_FIXED_CTR1   0x30a
>>> +#define MSR_CORE_PERF_FIXED_CTR2   0x30b
>>> +#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_MC0_CTL                        0x400
>>>  #define MSR_MC0_STATUS                     0x401
>>>  #define MSR_MC0_ADDR                       0x402
>> [snip]
>>
>> We have a mix of lines with tab indentation and with space indentation
>> here - should new sections be using spaces to satisfy checkpatch.pl?

For this I was expecting a v2 or a rationale why not.

>> New msr_* fields would be candidates for X86CPU, but there's already one
>> in CPUX86State. :/
>>
> Is this NAK?

No, it was more an implicit question of whether Paolo had considered a
different placement?

Since there is a uint64_t msr_ia32_misc_enable visible as precedent, I
was thinking it would be better for consistency to do such movements as
follow-up, if at all. However checking on that now, I do not have any
msr_* there in my repo, and it is after CPU_COMMON, thus not reset -
Paolo, what do you think?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg