From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42131) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V2MoP-00084C-UV for qemu-devel@nongnu.org; Thu, 25 Jul 2013 10:46:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V2MoM-0007nC-MB for qemu-devel@nongnu.org; Thu, 25 Jul 2013 10:46:37 -0400 Received: from cantor2.suse.de ([195.135.220.15]:49303 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V2MoM-0007mr-BU for qemu-devel@nongnu.org; Thu, 25 Jul 2013 10:46:34 -0400 Message-ID: <51F13A46.7010003@suse.de> Date: Thu, 25 Jul 2013 16:46:30 +0200 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <1374676642-2412-1-git-send-email-pbonzini@redhat.com> <51EFEC3B.7060805@suse.de> <20130725101412.GD22735@redhat.com> In-Reply-To: <20130725101412.GD22735@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] kvm: migrate vPMU state List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gleb Natapov , Paolo Bonzini Cc: qemu-devel@nongnu.org, ehabkost@redhat.com Am 25.07.2013 12:14, schrieb Gleb Natapov: > On Wed, Jul 24, 2013 at 05:01:15PM +0200, Andreas F=C3=A4rber 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 statu= s >>> MSR can be written and global overflow control MSR does not clear bit= s >>> 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-existe= nt >>> 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 >>> --- >>> 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 >>> =20 >>> +#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 >>> =20 >>> +#define MSR_P6_EVNTSEL0 0x186 >>> + >>> #define MSR_IA32_PERF_STATUS 0x198 >>> =20 >>> #define MSR_IA32_MISC_ENABLE 0x1a0 >>> @@ -342,6 +346,14 @@ >>> =20 >>> #define MSR_MTRRdefType 0x2ff >>> =20 >>> +#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 o= ne >> 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 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg