Hi,
this patch fixes an issue on Nehalem cpus discussed on the list
http://lists.xensource.com/archives/html/xen-devel/2009-10/msg01015.html
and further
http://lists.xensource.com/archives/html/xen-devel/2009-11/msg00011.html
but never a fix found it''s way into xen sources. We had a fix in our
private
tree but now we use the official SLES xen and so we met the issue again :-(
We didn''t use the fix proposed on
http://lists.xensource.com/archives/html/xen-devel/2009-11/msg00100.html
because we saw some counter overflows got lost. So we tried the solution
proposed in the patch.
Thanks.
Dietmar
Fix an issue on Nehalem cpus where performance counter overflows may lead to
endless NMIs on this cpu.
Signed-off-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>
diff -r 7d2fdc083c9c -r 0ff5de47951c xen/arch/x86/hvm/vmx/vpmu_core2.c
--- a/xen/arch/x86/hvm/vmx/vpmu_core2.c Thu Nov 18 12:28:31 2010 +0000
+++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c Fri Nov 19 10:34:31 2010 +0100
@@ -36,6 +36,70 @@
#include <asm/hvm/vpmu.h>
#include <asm/hvm/vmx/vpmu_core2.h>
+/*
+ * QUIRK to workaround an issue on Nehalem processors currently seen
+ * on family 6 cpus E5520 (model 26) and X7542 (model 46).
+ * The issue leads to endless NMI loops on the processor.
+ * If a counter triggers an NMI and while the NMI handler is running another
+ * counter overflows the second counter triggers endless new NMIs.
+ * A solution is to read all flagged counters and if the value is 0 write
+ * 1 into it.
+ */
+static int is_nmi_quirk;
+
+static void check_nmi_quirk(void)
+{
+ u8 family = current_cpu_data.x86;
+ u8 cpu_model = current_cpu_data.x86_model;
+ is_nmi_quirk = 0;
+ if ( family == 6 )
+ {
+ if ( cpu_model == 46 || cpu_model == 26 )
+ is_nmi_quirk = 1;
+ }
+}
+
+static int core2_get_pmc_count(void);
+static void handle_nmi_quirk(u64 msr_content)
+{
+ int num_gen_pmc = core2_get_pmc_count();
+ int num_fix_pmc = 3;
+ int i;
+ u64 val;
+
+ if ( !is_nmi_quirk )
+ return;
+
+ val = msr_content & ((1 << num_gen_pmc) - 1);
+ for ( i = 0; i < num_gen_pmc; i++ )
+ {
+ if ( val & 0x1 )
+ {
+ u64 cnt;
+ rdmsrl(MSR_P6_PERFCTR0 + i, cnt);
+ if ( cnt == 0 )
+ wrmsrl(MSR_P6_PERFCTR0 + i, 1);
+ }
+ val >>= 1;
+ }
+ val = (msr_content >> 32) & ((1 << num_fix_pmc) - 1);
+ for ( i = 0; i < num_fix_pmc; i++ )
+ {
+ if ( val & 0x1 )
+ {
+ u64 cnt;
+ rdmsrl(MSR_CORE_PERF_FIXED_CTR0 + i, cnt);
+ if ( cnt == 0 )
+ wrmsrl(MSR_CORE_PERF_FIXED_CTR0 + i, 1);
+ }
+ val >>= 1;
+ }
+}
+
+#define CHECK_HANDLE_NMI_QUIRK(msr_content) \
+ if ( is_nmi_quirk ) \
+ handle_nmi_quirk(msr_content);
+
u32 core2_counters_msr[] = {
MSR_CORE_PERF_FIXED_CTR0,
MSR_CORE_PERF_FIXED_CTR1,
@@ -494,6 +558,9 @@
rdmsrl(MSR_CORE_PERF_GLOBAL_STATUS, msr_content);
if ( !msr_content )
return 0;
+
+ CHECK_HANDLE_NMI_QUIRK(msr_content)
+
core2_vpmu_cxt->global_ovf_status |= msr_content;
msr_content = 0xC000000700000000 | ((1 << core2_get_pmc_count()) -
1);
wrmsrl(MSR_CORE_PERF_GLOBAL_OVF_CTRL, msr_content);
@@ -515,6 +582,7 @@
static void core2_vpmu_initialise(struct vcpu *v)
{
+ check_nmi_quirk();
}
static void core2_vpmu_destroy(struct vcpu *v)
--
Company details: http://ts.fujitsu.com/imprint.html
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
>>> On 19.11.10 at 11:29, Dietmar Hahn <dietmar.hahn@ts.fujitsu.com> wrote: > +/* > + * QUIRK to workaround an issue on Nehalem processors currently seen > + * on family 6 cpus E5520 (model 26) and X7542 (model 46). > + * The issue leads to endless NMI loops on the processor. > + * If a counter triggers an NMI and while the NMI handler is running another > + * counter overflows the second counter triggers endless new NMIs. > + * A solution is to read all flagged counters and if the value is 0 write > + * 1 into it. > + */Two things I don''t understand here: One is that I can''t see how from the NMI handler control would get to core2_vpmu_do_interrupt() - afaics, this gets called only in the context of the (vectored) smp_pmu_apic_interrupt(). The other is that if nested interrupts occur, how would you prevent this by writing ones into zero counters? That is, in the best case I could see this shrinking the window within which unintended nested interrupts would occur. Or is it that the secondary interrupts only occur after the first one returned? Is this (mis-) behavior documented somewhere?> +static int is_nmi_quirk;bool_t __read_mostly?> + > +static void check_nmi_quirk(void) > +{ > + u8 family = current_cpu_data.x86; > + u8 cpu_model = current_cpu_data.x86_model; > + is_nmi_quirk = 0; > + if ( family == 6 ) > + { > + if ( cpu_model == 46 || cpu_model == 26 ) > + is_nmi_quirk = 1; > + } > +} > + > +static int core2_get_pmc_count(void); > +static void handle_nmi_quirk(u64 msr_content) > +{ > + int num_gen_pmc = core2_get_pmc_count(); > + int num_fix_pmc = 3; > + int i; > + u64 val; > + > + if ( !is_nmi_quirk ) > + return; > + > + val = msr_content & ((1 << num_gen_pmc) - 1);What''s the point of masking if the subsequent loop looks at the bottom so many bits only anyway?> + for ( i = 0; i < num_gen_pmc; i++ ) > + { > + if ( val & 0x1 ) > + { > + u64 cnt; > + rdmsrl(MSR_P6_PERFCTR0 + i, cnt); > + if ( cnt == 0 ) > + wrmsrl(MSR_P6_PERFCTR0 + i, 1); > + } > + val >>= 1; > + } > + val = (msr_content >> 32) & ((1 << num_fix_pmc) - 1);Same here.> + for ( i = 0; i < num_fix_pmc; i++ ) > + { > + if ( val & 0x1 ) > + { > + u64 cnt; > + rdmsrl(MSR_CORE_PERF_FIXED_CTR0 + i, cnt); > + if ( cnt == 0 ) > + wrmsrl(MSR_CORE_PERF_FIXED_CTR0 + i, 1); > + } > + val >>= 1; > + } > +} > + > +#define CHECK_HANDLE_NMI_QUIRK(msr_content) \ > + if ( is_nmi_quirk ) \ > + handle_nmi_quirk(msr_content); > +Why do you need a macro here if you use it only once?> u32 core2_counters_msr[] = { > MSR_CORE_PERF_FIXED_CTR0, > MSR_CORE_PERF_FIXED_CTR1, > @@ -494,6 +558,9 @@ > rdmsrl(MSR_CORE_PERF_GLOBAL_STATUS, msr_content); > if ( !msr_content ) > return 0; > + > + CHECK_HANDLE_NMI_QUIRK(msr_content) > + > core2_vpmu_cxt->global_ovf_status |= msr_content; > msr_content = 0xC000000700000000 | ((1 << core2_get_pmc_count()) - 1); > wrmsrl(MSR_CORE_PERF_GLOBAL_OVF_CTRL, msr_content);Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi, Jan, The actual handler core2_vpmu_do_interrupt is never called under a NMI handler context. The reason is that I don''t want to place such an interrupt on behalf of guest to be of so high a priority. So, even guest programs the virtual APIC to use NMI, the underlying HW does not. It uses a vector (0xF8? I don''t remember clearly). This issue this patch solves is likely a HW bug but I have not find any documented errata. On some NHM processors, when a PMI is received, you will observe the counter that triggers this interrupt is zero (which means it has just gone overflowed). If you unmask the PMI sources (as you might know, PMI gets automatically masked when received), you will immediately get another interrupt. This is how you get an interrupt loop here (not NMI loop, but PMI loop). The reason why native oprofile works is that oprofile actually reprogram the counter before unmask the interrupt. In our VPMU implementation in Xen, the PMI handler does nothing but pending an interrupt to guest, then it unmasks the PMI and return. I find that reprograming the counter to be 1 works around this issue (any number that is *not* zero should work actually). Another working around would be only unmasking the real PMI when guests unmask its virtual PMIs. This work around is not very promising, since by the time guests unmask the virtual PMIs, the vcpu might be migrated to other CPUs. You need complex tracking and an IPI to do the right unmask on the correct physical CPUs. Hope I can explain the whole matter clearly here. BTW: As I observed on other processors, when a PMI is received, the counter is never zero (some values just slightly greater than zero). Shan Haitao -----Original Message----- From: Jan Beulich [mailto:JBeulich@novell.com] Sent: Friday, November 19, 2010 7:07 PM To: Dietmar Hahn Cc: Shan, Haitao; xen-devel@lists.xensource.com Subject: Re: [Xen-devel] [PATCH] VPMU issue on Nehalem cpus>>> On 19.11.10 at 11:29, Dietmar Hahn <dietmar.hahn@ts.fujitsu.com> wrote: > +/* > + * QUIRK to workaround an issue on Nehalem processors currently seen > + * on family 6 cpus E5520 (model 26) and X7542 (model 46). > + * The issue leads to endless NMI loops on the processor. > + * If a counter triggers an NMI and while the NMI handler is running another > + * counter overflows the second counter triggers endless new NMIs. > + * A solution is to read all flagged counters and if the value is 0 write > + * 1 into it. > + */Two things I don''t understand here: One is that I can''t see how from the NMI handler control would get to core2_vpmu_do_interrupt() - afaics, this gets called only in the context of the (vectored) smp_pmu_apic_interrupt(). The other is that if nested interrupts occur, how would you prevent this by writing ones into zero counters? That is, in the best case I could see this shrinking the window within which unintended nested interrupts would occur. Or is it that the secondary interrupts only occur after the first one returned? Is this (mis-) behavior documented somewhere?> +static int is_nmi_quirk;bool_t __read_mostly?> + > +static void check_nmi_quirk(void) > +{ > + u8 family = current_cpu_data.x86; > + u8 cpu_model = current_cpu_data.x86_model; > + is_nmi_quirk = 0; > + if ( family == 6 ) > + { > + if ( cpu_model == 46 || cpu_model == 26 ) > + is_nmi_quirk = 1; > + } > +} > + > +static int core2_get_pmc_count(void); > +static void handle_nmi_quirk(u64 msr_content) > +{ > + int num_gen_pmc = core2_get_pmc_count(); > + int num_fix_pmc = 3; > + int i; > + u64 val; > + > + if ( !is_nmi_quirk ) > + return; > + > + val = msr_content & ((1 << num_gen_pmc) - 1);What''s the point of masking if the subsequent loop looks at the bottom so many bits only anyway?> + for ( i = 0; i < num_gen_pmc; i++ ) > + { > + if ( val & 0x1 ) > + { > + u64 cnt; > + rdmsrl(MSR_P6_PERFCTR0 + i, cnt); > + if ( cnt == 0 ) > + wrmsrl(MSR_P6_PERFCTR0 + i, 1); > + } > + val >>= 1; > + } > + val = (msr_content >> 32) & ((1 << num_fix_pmc) - 1);Same here.> + for ( i = 0; i < num_fix_pmc; i++ ) > + { > + if ( val & 0x1 ) > + { > + u64 cnt; > + rdmsrl(MSR_CORE_PERF_FIXED_CTR0 + i, cnt); > + if ( cnt == 0 ) > + wrmsrl(MSR_CORE_PERF_FIXED_CTR0 + i, 1); > + } > + val >>= 1; > + } > +} > + > +#define CHECK_HANDLE_NMI_QUIRK(msr_content) \ > + if ( is_nmi_quirk ) \ > + handle_nmi_quirk(msr_content); > +Why do you need a macro here if you use it only once?> u32 core2_counters_msr[] = { > MSR_CORE_PERF_FIXED_CTR0, > MSR_CORE_PERF_FIXED_CTR1, > @@ -494,6 +558,9 @@ > rdmsrl(MSR_CORE_PERF_GLOBAL_STATUS, msr_content); > if ( !msr_content ) > return 0; > + > + CHECK_HANDLE_NMI_QUIRK(msr_content) > + > core2_vpmu_cxt->global_ovf_status |= msr_content; > msr_content = 0xC000000700000000 | ((1 << core2_get_pmc_count()) - 1); > wrmsrl(MSR_CORE_PERF_GLOBAL_OVF_CTRL, msr_content);Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi Haitao, Am 20.11.2010 schrieb ""Shan, Haitao" <haitao.shan@intel.com>":> Hi, Jan, > > The actual handler core2_vpmu_do_interrupt is never called under a NMI handler context. The reason is that I don''t want to place such an interrupt on behalf of guest to be of so high a priority. So, even guest programs the virtual APIC to use NMI, the underlying HW does not. It uses a vector (0xF8? I don''t remember clearly). > > This issue this patch solves is likely a HW bug but I have not find any documented errata. On some NHM processors, when a PMI is received, you will observe the counter that triggers this interrupt is zero (which means it has just gone overflowed). If you unmask the PMI sources (as you might know, PMI gets automatically masked when received), you will immediately get another interrupt. This is how you get an interrupt loop here (not NMI loop, but PMI loop). > The reason why native oprofile works is that oprofile actually reprogram the counter before unmask the interrupt. In our VPMU implementation in Xen, the PMI handler does nothing but pending an interrupt to guest, then it unmasks the PMI and return. > I find that reprograming the counter to be 1 works around this issue (any number that is *not* zero should work actually). > Another working around would be only unmasking the real PMI when guests unmask its virtual PMIs. This work around is not very promising, since by the time guests unmask the virtual PMIs, the vcpu might be migrated to other CPUs. You need complex tracking and an IPI to do the right unmask on the correct physical CPUs. > > Hope I can explain the whole matter clearly here. > > > BTW: As I observed on other processors, when a PMI is received, the counter is never zero (some values just slightly greater than zero). > > Shan Haitaovery good explanation, many thanks! Dietmar.> > -----Original Message----- > From: Jan Beulich [mailto:JBeulich@novell.com] > Sent: Friday, November 19, 2010 7:07 PM > To: Dietmar Hahn > Cc: Shan, Haitao; xen-devel@lists.xensource.com > Subject: Re: [Xen-devel] [PATCH] VPMU issue on Nehalem cpus > > >>> On 19.11.10 at 11:29, Dietmar Hahn <dietmar.hahn@ts.fujitsu.com> wrote: > > +/* > > + * QUIRK to workaround an issue on Nehalem processors currently seen > > + * on family 6 cpus E5520 (model 26) and X7542 (model 46). > > + * The issue leads to endless NMI loops on the processor. > > + * If a counter triggers an NMI and while the NMI handler is running another > > + * counter overflows the second counter triggers endless new NMIs. > > + * A solution is to read all flagged counters and if the value is 0 write > > + * 1 into it. > > + */ > > Two things I don''t understand here: One is that I can''t see how > from the NMI handler control would get to > core2_vpmu_do_interrupt() - afaics, this gets called only in the > context of the (vectored) smp_pmu_apic_interrupt(). The other > is that if nested interrupts occur, how would you prevent this > by writing ones into zero counters? That is, in the best case I > could see this shrinking the window within which unintended > nested interrupts would occur. Or is it that the secondary > interrupts only occur after the first one returned? Is this (mis-) > behavior documented somewhere? > > > +static int is_nmi_quirk; > > bool_t __read_mostly? > > > + > > +static void check_nmi_quirk(void) > > +{ > > + u8 family = current_cpu_data.x86; > > + u8 cpu_model = current_cpu_data.x86_model; > > + is_nmi_quirk = 0; > > + if ( family == 6 ) > > + { > > + if ( cpu_model == 46 || cpu_model == 26 ) > > + is_nmi_quirk = 1; > > + } > > +} > > + > > +static int core2_get_pmc_count(void); > > +static void handle_nmi_quirk(u64 msr_content) > > +{ > > + int num_gen_pmc = core2_get_pmc_count(); > > + int num_fix_pmc = 3; > > + int i; > > + u64 val; > > + > > + if ( !is_nmi_quirk ) > > + return; > > + > > + val = msr_content & ((1 << num_gen_pmc) - 1); > > What''s the point of masking if the subsequent loop looks at the > bottom so many bits only anyway? > > > + for ( i = 0; i < num_gen_pmc; i++ ) > > + { > > + if ( val & 0x1 ) > > + { > > + u64 cnt; > > + rdmsrl(MSR_P6_PERFCTR0 + i, cnt); > > + if ( cnt == 0 ) > > + wrmsrl(MSR_P6_PERFCTR0 + i, 1); > > + } > > + val >>= 1; > > + } > > + val = (msr_content >> 32) & ((1 << num_fix_pmc) - 1); > > Same here. > > > + for ( i = 0; i < num_fix_pmc; i++ ) > > + { > > + if ( val & 0x1 ) > > + { > > + u64 cnt; > > + rdmsrl(MSR_CORE_PERF_FIXED_CTR0 + i, cnt); > > + if ( cnt == 0 ) > > + wrmsrl(MSR_CORE_PERF_FIXED_CTR0 + i, 1); > > + } > > + val >>= 1; > > + } > > +} > > + > > +#define CHECK_HANDLE_NMI_QUIRK(msr_content) \ > > + if ( is_nmi_quirk ) \ > > + handle_nmi_quirk(msr_content); > > + > > Why do you need a macro here if you use it only once? > > > u32 core2_counters_msr[] = { > > MSR_CORE_PERF_FIXED_CTR0, > > MSR_CORE_PERF_FIXED_CTR1, > > @@ -494,6 +558,9 @@ > > rdmsrl(MSR_CORE_PERF_GLOBAL_STATUS, msr_content); > > if ( !msr_content ) > > return 0; > > + > > + CHECK_HANDLE_NMI_QUIRK(msr_content) > > + > > core2_vpmu_cxt->global_ovf_status |= msr_content; > > msr_content = 0xC000000700000000 | ((1 << core2_get_pmc_count()) - 1); > > wrmsrl(MSR_CORE_PERF_GLOBAL_OVF_CTRL, msr_content); > > Jan > > >-- Dietmar Hahn TSP ES&S SWE OS FUJITSU Fujitsu Technology Solutions Domagkstraße 28, D-80807 München, Germany Tel: +49 (89) 3222 2952 Email: dietmar.hahn@ts.fujitsu.com Web: http://ts.fujitsu.com Company details: http://ts.fujitsu.com/imprint.html _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
am 19.11.2010 schrieb "Jan Beulich <JBeulich@novell.com>":> >>> On 19.11.10 at 11:29, Dietmar Hahn <dietmar.hahn@ts.fujitsu.com> wrote: > > +/* > > + * QUIRK to workaround an issue on Nehalem processors currently seen > > + * on family 6 cpus E5520 (model 26) and X7542 (model 46). > > + * The issue leads to endless NMI loops on the processor. > > + * If a counter triggers an NMI and while the NMI handler is running another > > + * counter overflows the second counter triggers endless new NMIs. > > + * A solution is to read all flagged counters and if the value is 0 write > > + * 1 into it. > > + */I will try to improve the comment.> > Two things I don''t understand here: One is that I can''t see how > from the NMI handler control would get to > core2_vpmu_do_interrupt() - afaics, this gets called only in the > context of the (vectored) smp_pmu_apic_interrupt(). The other > is that if nested interrupts occur, how would you prevent this > by writing ones into zero counters? That is, in the best case I > could see this shrinking the window within which unintended > nested interrupts would occur. Or is it that the secondary > interrupts only occur after the first one returned? Is this (mis-) > behavior documented somewhere? > > > +static int is_nmi_quirk; > > bool_t __read_mostly?Yes, would be better.> > > + > > +static void check_nmi_quirk(void) > > +{ > > + u8 family = current_cpu_data.x86; > > + u8 cpu_model = current_cpu_data.x86_model; > > + is_nmi_quirk = 0; > > + if ( family == 6 ) > > + { > > + if ( cpu_model == 46 || cpu_model == 26 ) > > + is_nmi_quirk = 1; > > + } > > +} > > + > > +static int core2_get_pmc_count(void); > > +static void handle_nmi_quirk(u64 msr_content) > > +{ > > + int num_gen_pmc = core2_get_pmc_count(); > > + int num_fix_pmc = 3; > > + int i; > > + u64 val; > > + > > + if ( !is_nmi_quirk ) > > + return; > > + > > + val = msr_content & ((1 << num_gen_pmc) - 1); > > What''s the point of masking if the subsequent loop looks at the > bottom so many bits only anyway?Bits 0-31 flag the overflow of the general counters (currently max 4) and 32-63 flag the overflow of the fixed counter (currently max 3). Yes the first mask is not necessary, maybe a comment would be better?> > > + for ( i = 0; i < num_gen_pmc; i++ ) > > + { > > + if ( val & 0x1 ) > > + { > > + u64 cnt; > > + rdmsrl(MSR_P6_PERFCTR0 + i, cnt); > > + if ( cnt == 0 ) > > + wrmsrl(MSR_P6_PERFCTR0 + i, 1); > > + } > > + val >>= 1; > > + } > > + val = (msr_content >> 32) & ((1 << num_fix_pmc) - 1); > > Same here. > > > + for ( i = 0; i < num_fix_pmc; i++ ) > > + { > > + if ( val & 0x1 ) > > + { > > + u64 cnt; > > + rdmsrl(MSR_CORE_PERF_FIXED_CTR0 + i, cnt); > > + if ( cnt == 0 ) > > + wrmsrl(MSR_CORE_PERF_FIXED_CTR0 + i, 1); > > + } > > + val >>= 1; > > + } > > +} > > + > > +#define CHECK_HANDLE_NMI_QUIRK(msr_content) \ > > + if ( is_nmi_quirk ) \ > > + handle_nmi_quirk(msr_content); > > + > > Why do you need a macro here if you use it only once?I did this only to have as few impact on the existing code as possible. I will remove it. Thanks. Dietmar.> > > u32 core2_counters_msr[] = { > > MSR_CORE_PERF_FIXED_CTR0, > > MSR_CORE_PERF_FIXED_CTR1, > > @@ -494,6 +558,9 @@ > > rdmsrl(MSR_CORE_PERF_GLOBAL_STATUS, msr_content); > > if ( !msr_content ) > > return 0; > > + > > + CHECK_HANDLE_NMI_QUIRK(msr_content) > > + > > core2_vpmu_cxt->global_ovf_status |= msr_content; > > msr_content = 0xC000000700000000 | ((1 << core2_get_pmc_count()) - 1); > > wrmsrl(MSR_CORE_PERF_GLOBAL_OVF_CTRL, msr_content); > > Jan-- Company details: http://ts.fujitsu.com/imprint.html _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> On 22.11.10 at 08:06, Dietmar Hahn <dietmar.hahn@ts.fujitsu.com> wrote: > am 19.11.2010 schrieb "Jan Beulich <JBeulich@novell.com>": >> >>> On 19.11.10 at 11:29, Dietmar Hahn <dietmar.hahn@ts.fujitsu.com> wrote: >> > +static int core2_get_pmc_count(void); >> > +static void handle_nmi_quirk(u64 msr_content) >> > +{ >> > + int num_gen_pmc = core2_get_pmc_count(); >> > + int num_fix_pmc = 3; >> > + int i; >> > + u64 val; >> > + >> > + if ( !is_nmi_quirk ) >> > + return; >> > + >> > + val = msr_content & ((1 << num_gen_pmc) - 1); >> >> What''s the point of masking if the subsequent loop looks at the >> bottom so many bits only anyway? > > Bits 0-31 flag the overflow of the general counters (currently max 4) and > 32-63 > flag the overflow of the fixed counter (currently max 3). > Yes the first mask is not necessary, maybe a comment would be better?Neither is the second mask (below) - the shift is all that''s really needed. Afaic, a comment doesn''t seem necessary, but Keir may by of different opinion here.>> >> > + for ( i = 0; i < num_gen_pmc; i++ ) >> > + { >> > + if ( val & 0x1 ) >> > + { >> > + u64 cnt; >> > + rdmsrl(MSR_P6_PERFCTR0 + i, cnt); >> > + if ( cnt == 0 ) >> > + wrmsrl(MSR_P6_PERFCTR0 + i, 1); >> > + } >> > + val >>= 1; >> > + } >> > + val = (msr_content >> 32) & ((1 << num_fix_pmc) - 1); >> >> Same here. >> >> > + for ( i = 0; i < num_fix_pmc; i++ ) >> > + { >> > + if ( val & 0x1 ) >> > + { >> > + u64 cnt; >> > + rdmsrl(MSR_CORE_PERF_FIXED_CTR0 + i, cnt); >> > + if ( cnt == 0 ) >> > + wrmsrl(MSR_CORE_PERF_FIXED_CTR0 + i, 1); >> > + } >> > + val >>= 1; >> > + } >> > +}Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 22/11/2010 09:19, "Jan Beulich" <JBeulich@novell.com> wrote:>>>> + val = msr_content & ((1 << num_gen_pmc) - 1); >>> >>> What''s the point of masking if the subsequent loop looks at the >>> bottom so many bits only anyway? >> >> Bits 0-31 flag the overflow of the general counters (currently max 4) and >> 32-63 >> flag the overflow of the fixed counter (currently max 3). >> Yes the first mask is not necessary, maybe a comment would be better? > > Neither is the second mask (below) - the shift is all that''s really > needed. Afaic, a comment doesn''t seem necessary, but Keir > may by of different opinion here.It''s clear from the code that the mask operation is unnecessary. No code comment required. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel