[PATCH v2 0/3] VPMU fixes

[PATCH v2 0/3] VPMU fixes

[Boris Ostrovsky]

* Limit VPMU support to PMU versions 2, 3 and 4 (emulated at version 3 level)
* Always implement family 6 VPMU quirk.
  ==>  Intel folks: is the quirk needed for all family 6 processors or can we
                    limit it to certain models?
* Update (or rather restore) arch VPMU files maintainership

Boris Ostrovsky (3):
  x86/VPMU: Support only versions 2 through 4 of architectural
    performance     monitoring
  x86/VPMU: No need to check whether VPMU quirk is needed on Intel
  MAINTAINERS: Restore original maintainership of arch VPMU files

 MAINTAINERS                   |  2 +
 xen/arch/x86/cpu/vpmu_intel.c | 91 ++++++++++++-------------------------------
 2 files changed, 26 insertions(+), 67 deletions(-)

-- 
1.8.1.4

[PATCH v2 1/3] x86/VPMU: Support only versions 2 through 4 of architectural performance monitoring

[Boris Ostrovsky]

We need to have at least version 2 since it's the first version to
support various control and status registers (such as
MSR_CORE_PERF_GLOBAL_CTRL) that VPMU relies on always having.

Since we don't fully support version 4 yet report it as version 3 in
CPUID.

With explicit testing for PMU version we can now remove CPUID model
check.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---

v2:
* Support PMU version 4 (emulated at version 3 level)
* Minor code adjustments


 xen/arch/x86/cpu/vpmu_intel.c | 73 ++++++++++++++-----------------------------
 1 file changed, 23 insertions(+), 50 deletions(-)

diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
index d5ea7fe..a267a3c 100644
--- a/xen/arch/x86/cpu/vpmu_intel.c
+++ b/xen/arch/x86/cpu/vpmu_intel.c
@@ -733,11 +733,11 @@ static void core2_vpmu_do_cpuid(unsigned int input,
                                 unsigned int *eax, unsigned int *ebx,
                                 unsigned int *ecx, unsigned int *edx)
 {
-    if (input == 0x1)
+    switch ( input )
     {
-        struct vpmu_struct *vpmu = vcpu_vpmu(current);
+    case 0x1:
 
-        if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) )
+        if ( vpmu_is_set(vcpu_vpmu(current), VPMU_CPU_HAS_DS) )
         {
             /* Switch on the 'Debug Store' feature in CPUID.EAX[1]:EDX[21] */
             *edx |= cpufeat_mask(X86_FEATURE_DS);
@@ -746,6 +746,13 @@ static void core2_vpmu_do_cpuid(unsigned int input,
             if ( cpu_has(&current_cpu_data, X86_FEATURE_DSCPL) )
                 *ecx |= cpufeat_mask(X86_FEATURE_DSCPL);
         }
+        break;
+
+    case 0xa:
+        /* Since we don't fully emulate version 4 report version 3 */
+        if ( MASK_EXTR(*eax, PMU_VERSION_MASK) == 4 )
+            *eax = (*eax & ~PMU_VERSION_MASK) | MASK_INSR(3, PMU_VERSION_MASK);
+        break;
     }
 }
 
@@ -955,59 +962,25 @@ int vmx_vpmu_initialise(struct vcpu *v)
 int __init core2_vpmu_init(void)
 {
     u64 caps;
+    unsigned int version = 0;
 
-    if ( current_cpu_data.x86 != 6 )
+    if ( current_cpu_data.cpuid_level >= 0xa )
+        version = MASK_EXTR(cpuid_eax(0xa), PMU_VERSION_MASK);
+
+    if ( version == 4 )
+        printk(XENLOG_INFO "VPMU: PMU version 4 is not fully supported. "
+               "Emulating version 3\n");
+    else if ( (version != 2) && (version != 3) )
     {
-        printk(XENLOG_WARNING "VPMU: only family 6 is supported\n");
+        printk(XENLOG_WARNING "VPMU: PMU version %u is not supported\n",
+               version);
         return -EINVAL;
     }
 
-    switch ( current_cpu_data.x86_model )
+    if ( current_cpu_data.x86 != 6 )
     {
-        /* Core2: */
-        case 0x0f: /* original 65 nm celeron/pentium/core2/xeon, "Merom"/"Conroe" */
-        case 0x16: /* single-core 65 nm celeron/core2solo "Merom-L"/"Conroe-L" */
-        case 0x17: /* 45 nm celeron/core2/xeon "Penryn"/"Wolfdale" */
-        case 0x1d: /* six-core 45 nm xeon "Dunnington" */
-
-        case 0x2a: /* SandyBridge */
-        case 0x2d: /* SandyBridge, "Romley-EP" */
-
-        /* Nehalem: */
-        case 0x1a: /* 45 nm nehalem, "Bloomfield" */
-        case 0x1e: /* 45 nm nehalem, "Lynnfield", "Clarksfield", "Jasper Forest" */
-        case 0x2e: /* 45 nm nehalem-ex, "Beckton" */
-
-        /* Westmere: */
-        case 0x25: /* 32 nm nehalem, "Clarkdale", "Arrandale" */
-        case 0x2c: /* 32 nm nehalem, "Gulftown", "Westmere-EP" */
-        case 0x2f: /* 32 nm Westmere-EX */
-
-        case 0x3a: /* IvyBridge */
-        case 0x3e: /* IvyBridge EP */
-
-        /* Haswell: */
-        case 0x3c:
-        case 0x3f:
-        case 0x45:
-        case 0x46:
-
-        /* Broadwell */
-        case 0x3d:
-        case 0x4f:
-        case 0x56:
-
-        /* future: */
-        case 0x4e:
-
-        /* next gen Xeon Phi */
-        case 0x57:
-            break;
-
-        default:
-            printk(XENLOG_WARNING "VPMU: Unsupported CPU model %#x\n",
-                   current_cpu_data.x86_model);
-            return -EINVAL;
+        printk(XENLOG_WARNING "VPMU: only family 6 is supported\n");
+        return -EINVAL;
     }
 
     arch_pmc_cnt = core2_get_arch_pmc_count();
-- 
1.8.1.4

Re: [PATCH v2 1/3] x86/VPMU: Support only versions 2 through 4 of architectural performance monitoring

[Jan Beulich]

>>> On 01.12.15 at 17:50, <boris.ostrovsky@oracle.com> wrote:
> @@ -746,6 +746,13 @@ static void core2_vpmu_do_cpuid(unsigned int input,
>              if ( cpu_has(&current_cpu_data, X86_FEATURE_DSCPL) )
>                  *ecx |= cpufeat_mask(X86_FEATURE_DSCPL);
>          }
> +        break;
> +
> +    case 0xa:
> +        /* Since we don't fully emulate version 4 report version 3 */
> +        if ( MASK_EXTR(*eax, PMU_VERSION_MASK) == 4 )

Considering that this may not be what physical CPUID returned, I
think you'd better use >= here.

> @@ -955,59 +962,25 @@ int vmx_vpmu_initialise(struct vcpu *v)
>  int __init core2_vpmu_init(void)
>  {
>      u64 caps;
> +    unsigned int version = 0;
>  
> -    if ( current_cpu_data.x86 != 6 )
> +    if ( current_cpu_data.cpuid_level >= 0xa )
> +        version = MASK_EXTR(cpuid_eax(0xa), PMU_VERSION_MASK);
> +
> +    if ( version == 4 )
> +        printk(XENLOG_INFO "VPMU: PMU version 4 is not fully supported. "
> +               "Emulating version 3\n");
> +    else if ( (version != 2) && (version != 3) )
>      {
> -        printk(XENLOG_WARNING "VPMU: only family 6 is supported\n");
> +        printk(XENLOG_WARNING "VPMU: PMU version %u is not supported\n",
> +               version);
>          return -EINVAL;
>      }

I'd appreciate if you would use switch() here.

Jan

Re: [PATCH v2 1/3] x86/VPMU: Support only versions 2 through 4 of architectural performance monitoring

[Tian, Kevin]

> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: Wednesday, December 02, 2015 12:50 AM
> 
> We need to have at least version 2 since it's the first version to
> support various control and status registers (such as
> MSR_CORE_PERF_GLOBAL_CTRL) that VPMU relies on always having.
> 
> Since we don't fully support version 4 yet report it as version 3 in
> CPUID.
> 
> With explicit testing for PMU version we can now remove CPUID model
> check.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

[PATCH v2 2/3] x86/VPMU: No need to check whether VPMU quirk is needed on Intel

[Boris Ostrovsky]

We only support family 6 so quirk handling is always needed.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/arch/x86/cpu/vpmu_intel.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
index a267a3c..86d390f 100644
--- a/xen/arch/x86/cpu/vpmu_intel.c
+++ b/xen/arch/x86/cpu/vpmu_intel.c
@@ -106,24 +106,11 @@ static const unsigned int regs_off =
  * 1 (or another value != 0) into it.
  * There exist no errata and the real cause of this behaviour is unknown.
  */
-bool_t __read_mostly is_pmc_quirk;
-
-static void check_pmc_quirk(void)
-{
-    if ( current_cpu_data.x86 == 6 )
-        is_pmc_quirk = 1;
-    else
-        is_pmc_quirk = 0;    
-}
-
 static void handle_pmc_quirk(u64 msr_content)
 {
     int i;
     u64 val;
 
-    if ( !is_pmc_quirk )
-        return;
-
     val = msr_content;
     for ( i = 0; i < arch_pmc_cnt; i++ )
     {
@@ -812,8 +799,7 @@ static int core2_vpmu_do_interrupt(struct cpu_user_regs *regs)
     rdmsrl(MSR_CORE_PERF_GLOBAL_STATUS, msr_content);
     if ( msr_content )
     {
-        if ( is_pmc_quirk )
-            handle_pmc_quirk(msr_content);
+        handle_pmc_quirk(msr_content);
         core2_vpmu_cxt->global_status |= msr_content;
         msr_content = 0xC000000700000000 | ((1 << arch_pmc_cnt) - 1);
         wrmsrl(MSR_CORE_PERF_GLOBAL_OVF_CTRL, msr_content);
@@ -998,8 +984,6 @@ int __init core2_vpmu_init(void)
               sizeof(uint64_t) * fixed_pmc_cnt +
               sizeof(struct xen_pmu_cntr_pair) * arch_pmc_cnt;
 
-    check_pmc_quirk();
-
     if ( sizeof(struct xen_pmu_data) + sizeof(uint64_t) * fixed_pmc_cnt +
          sizeof(struct xen_pmu_cntr_pair) * arch_pmc_cnt > PAGE_SIZE )
     {
-- 
1.8.1.4

Re: [PATCH v2 2/3] x86/VPMU: No need to check whether VPMU quirk is needed on Intel

[Tian, Kevin]

> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: Wednesday, December 02, 2015 12:50 AM
> 
> We only support family 6 so quirk handling is always needed.

what about you adding other family support in the future then you'll
add back below quirk check?

> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
>  xen/arch/x86/cpu/vpmu_intel.c | 18 +-----------------
>  1 file changed, 1 insertion(+), 17 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
> index a267a3c..86d390f 100644
> --- a/xen/arch/x86/cpu/vpmu_intel.c
> +++ b/xen/arch/x86/cpu/vpmu_intel.c
> @@ -106,24 +106,11 @@ static const unsigned int regs_off =
>   * 1 (or another value != 0) into it.
>   * There exist no errata and the real cause of this behaviour is unknown.
>   */
> -bool_t __read_mostly is_pmc_quirk;
> -
> -static void check_pmc_quirk(void)
> -{
> -    if ( current_cpu_data.x86 == 6 )
> -        is_pmc_quirk = 1;
> -    else
> -        is_pmc_quirk = 0;
> -}
> -
>  static void handle_pmc_quirk(u64 msr_content)
>  {
>      int i;
>      u64 val;
> 
> -    if ( !is_pmc_quirk )
> -        return;
> -
>      val = msr_content;
>      for ( i = 0; i < arch_pmc_cnt; i++ )
>      {
> @@ -812,8 +799,7 @@ static int core2_vpmu_do_interrupt(struct cpu_user_regs *regs)
>      rdmsrl(MSR_CORE_PERF_GLOBAL_STATUS, msr_content);
>      if ( msr_content )
>      {
> -        if ( is_pmc_quirk )
> -            handle_pmc_quirk(msr_content);
> +        handle_pmc_quirk(msr_content);
>          core2_vpmu_cxt->global_status |= msr_content;
>          msr_content = 0xC000000700000000 | ((1 << arch_pmc_cnt) - 1);
>          wrmsrl(MSR_CORE_PERF_GLOBAL_OVF_CTRL, msr_content);
> @@ -998,8 +984,6 @@ int __init core2_vpmu_init(void)
>                sizeof(uint64_t) * fixed_pmc_cnt +
>                sizeof(struct xen_pmu_cntr_pair) * arch_pmc_cnt;
> 
> -    check_pmc_quirk();
> -
>      if ( sizeof(struct xen_pmu_data) + sizeof(uint64_t) * fixed_pmc_cnt +
>           sizeof(struct xen_pmu_cntr_pair) * arch_pmc_cnt > PAGE_SIZE )
>      {
> --
> 1.8.1.4

Re: [PATCH v2 2/3] x86/VPMU: No need to check whether VPMU quirk is needed on Intel

[Boris Ostrovsky]

On 12/03/2015 01:18 AM, Tian, Kevin wrote:
>> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
>> Sent: Wednesday, December 02, 2015 12:50 AM
>>
>> We only support family 6 so quirk handling is always needed.
> what about you adding other family support in the future then you'll
> add back below quirk check?

Yes.

-boris

Re: [PATCH v2 2/3] x86/VPMU: No need to check whether VPMU quirk is needed on Intel

[Tian, Kevin]

> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: Thursday, December 03, 2015 11:47 PM
> 
> On 12/03/2015 01:18 AM, Tian, Kevin wrote:
> >> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> >> Sent: Wednesday, December 02, 2015 12:50 AM
> >>
> >> We only support family 6 so quirk handling is always needed.
> > what about you adding other family support in the future then you'll
> > add back below quirk check?
> 
> Yes.
> 

If that's the case I prefer to not changing family 6 check in this context,
otherwise it's easy to get unnoted later when adding another family
support.

Thanks
Kevin

[PATCH v2 3/3] MAINTAINERS: Restore original maintainership of arch VPMU files

[Boris Ostrovsky]

It was lost when vpmu* files were moved from xen/arch/x86/hvm/{vmx|svm}/ to
xen/arch/x86/cpu/

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: ian.campbell@citrix.com
Cc: ian.jackson@eu.citrix.com
Cc: tim@xen.org
Cc: suravee.suthikulpanit@amd.com
Cc: Aravind.Gopalakrishnan@amd.com
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index e376646..87bc106 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -117,6 +117,7 @@ M:	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
 M:	Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
 S:	Supported
 F:	xen/arch/x86/hvm/svm/
+F:  xen/arch/x86/cpu/vpmu_amd.c
 
 ARINC653 SCHEDULER
 M:	Josh Whitehead <josh.whitehead@dornerworks.com>
@@ -205,6 +206,7 @@ S:	Supported
 F:	xen/arch/x86/hvm/vmx/
 F:	xen/arch/x86/mm/p2m-ept.c
 F:	xen/include/asm-x86/hvm/vmx/
+F:  xen/arch/x86/cpu/vpmu_intel.c
 
 IOMMU VENDOR INDEPENDENT CODE
 M:	Jan Beulich <jbeulich@suse.com>
-- 
1.8.1.4

Re: [PATCH v2 3/3] MAINTAINERS: Restore original maintainership of arch VPMU files

[Tian, Kevin]

> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: Wednesday, December 02, 2015 12:50 AM
> 
> It was lost when vpmu* files were moved from xen/arch/x86/hvm/{vmx|svm}/ to
> xen/arch/x86/cpu/
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: ian.campbell@citrix.com
> Cc: ian.jackson@eu.citrix.com
> Cc: tim@xen.org
> Cc: suravee.suthikulpanit@amd.com
> Cc: Aravind.Gopalakrishnan@amd.com
> ---
>  MAINTAINERS | 2 ++
>  1 file changed, 2 insertions(+)

Acked-by: Kevin Tian <kevin.tian@intel.com> for VMX part.

Re: [PATCH v2 0/3] VPMU fixes

[Tian, Kevin]

> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: Wednesday, December 02, 2015 12:50 AM
> 
> * Limit VPMU support to PMU versions 2, 3 and 4 (emulated at version 3 level)
> * Always implement family 6 VPMU quirk.
>   ==>  Intel folks: is the quirk needed for all family 6 processors or can we
>                     limit it to certain models?

Let me confirm this information internally. btw could you provide a link
where you find out the original quirk information?

Thanks
Kevin

Re: [PATCH v2 0/3] VPMU fixes

[Dietmar Hahn]

Am Mittwoch 02 Dezember 2015, 02:20:49 schrieb Tian, Kevin:
> > From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> > Sent: Wednesday, December 02, 2015 12:50 AM
> > 
> > * Limit VPMU support to PMU versions 2, 3 and 4 (emulated at version 3 level)
> > * Always implement family 6 VPMU quirk.
> >   ==>  Intel folks: is the quirk needed for all family 6 processors or can we
> >                     limit it to certain models?
> 
> Let me confirm this information internally. btw could you provide a link
> where you find out the original quirk information?

http://old-list-archives.xenproject.org/xen-devel/2010-11/msg01157.html
Dietmar.

> 
> Thanks
> Kevin
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

-- 
Company details: http://ts.fujitsu.com/imprint.html

Re: [PATCH v2 0/3] VPMU fixes

[Tian, Kevin]

> From: Dietmar Hahn [mailto:dietmar.hahn@ts.fujitsu.com]
> Sent: Wednesday, December 02, 2015 5:21 PM
> 
> Am Mittwoch 02 Dezember 2015, 02:20:49 schrieb Tian, Kevin:
> > > From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> > > Sent: Wednesday, December 02, 2015 12:50 AM
> > >
> > > * Limit VPMU support to PMU versions 2, 3 and 4 (emulated at version 3 level)
> > > * Always implement family 6 VPMU quirk.
> > >   ==>  Intel folks: is the quirk needed for all family 6 processors or can we
> > >                     limit it to certain models?
> >
> > Let me confirm this information internally. btw could you provide a link
> > where you find out the original quirk information?
> 
> http://old-list-archives.xenproject.org/xen-devel/2010-11/msg01157.html
> Dietmar.
> 

Looks there's no formal errata recorded from original thread. Since the
patch sustains original behavior, I'm OK with this assumption.

Thanks
Kevin

Re: [PATCH v2 0/3] VPMU fixes

[Jan Beulich]

>>> On 03.12.15 at 07:16, <kevin.tian@intel.com> wrote:
>>  From: Dietmar Hahn [mailto:dietmar.hahn@ts.fujitsu.com]
>> Sent: Wednesday, December 02, 2015 5:21 PM
>> 
>> Am Mittwoch 02 Dezember 2015, 02:20:49 schrieb Tian, Kevin:
>> > > From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
>> > > Sent: Wednesday, December 02, 2015 12:50 AM
>> > >
>> > > * Limit VPMU support to PMU versions 2, 3 and 4 (emulated at version 3 level)
>> > > * Always implement family 6 VPMU quirk.
>> > >   ==>  Intel folks: is the quirk needed for all family 6 processors or can we
>> > >                     limit it to certain models?
>> >
>> > Let me confirm this information internally. btw could you provide a link
>> > where you find out the original quirk information?
>> 
>> http://old-list-archives.xenproject.org/xen-devel/2010-11/msg01157.html 
>> Dietmar.
> 
> Looks there's no formal errata recorded from original thread. Since the
> patch sustains original behavior, I'm OK with this assumption.

Except that the Link Dietmar provided makes it even more
likely that this could be limited to certain models (since in that
original patch version it covered just two). See also the vague
statement in 75a92f551a ("Currently only a few Intel models
have VPMU workaround turned on. It").

Jan

Re: [PATCH v2 0/3] VPMU fixes

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, December 03, 2015 6:36 PM
> 
> >>> On 03.12.15 at 07:16, <kevin.tian@intel.com> wrote:
> >>  From: Dietmar Hahn [mailto:dietmar.hahn@ts.fujitsu.com]
> >> Sent: Wednesday, December 02, 2015 5:21 PM
> >>
> >> Am Mittwoch 02 Dezember 2015, 02:20:49 schrieb Tian, Kevin:
> >> > > From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> >> > > Sent: Wednesday, December 02, 2015 12:50 AM
> >> > >
> >> > > * Limit VPMU support to PMU versions 2, 3 and 4 (emulated at version 3 level)
> >> > > * Always implement family 6 VPMU quirk.
> >> > >   ==>  Intel folks: is the quirk needed for all family 6 processors or can we
> >> > >                     limit it to certain models?
> >> >
> >> > Let me confirm this information internally. btw could you provide a link
> >> > where you find out the original quirk information?
> >>
> >> http://old-list-archives.xenproject.org/xen-devel/2010-11/msg01157.html
> >> Dietmar.
> >
> > Looks there's no formal errata recorded from original thread. Since the
> > patch sustains original behavior, I'm OK with this assumption.
> 
> Except that the Link Dietmar provided makes it even more
> likely that this could be limited to certain models (since in that
> original patch version it covered just two). See also the vague
> statement in 75a92f551a ("Currently only a few Intel models
> have VPMU workaround turned on. It").
> 

Well, I'm not sure about the whole history. If you look at Boris's
patch, the code already assumes quirk for all family 6 before his
change. 

Thanks
Kevin

Re: [PATCH v2 0/3] VPMU fixes

[Jan Beulich]

>>> On 03.12.15 at 11:46, <kevin.tian@intel.com> wrote:
>>  From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Thursday, December 03, 2015 6:36 PM
>> 
>> >>> On 03.12.15 at 07:16, <kevin.tian@intel.com> wrote:
>> >>  From: Dietmar Hahn [mailto:dietmar.hahn@ts.fujitsu.com]
>> >> Sent: Wednesday, December 02, 2015 5:21 PM
>> >>
>> >> Am Mittwoch 02 Dezember 2015, 02:20:49 schrieb Tian, Kevin:
>> >> > > From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
>> >> > > Sent: Wednesday, December 02, 2015 12:50 AM
>> >> > >
>> >> > > * Limit VPMU support to PMU versions 2, 3 and 4 (emulated at version 3 
> level)
>> >> > > * Always implement family 6 VPMU quirk.
>> >> > >   ==>  Intel folks: is the quirk needed for all family 6 processors or can 
> we
>> >> > >                     limit it to certain models?
>> >> >
>> >> > Let me confirm this information internally. btw could you provide a link
>> >> > where you find out the original quirk information?
>> >>
>> >> http://old-list-archives.xenproject.org/xen-devel/2010-11/msg01157.html 
>> >> Dietmar.
>> >
>> > Looks there's no formal errata recorded from original thread. Since the
>> > patch sustains original behavior, I'm OK with this assumption.
>> 
>> Except that the Link Dietmar provided makes it even more
>> likely that this could be limited to certain models (since in that
>> original patch version it covered just two). See also the vague
>> statement in 75a92f551a ("Currently only a few Intel models
>> have VPMU workaround turned on. It").
> 
> Well, I'm not sure about the whole history. If you look at Boris's
> patch, the code already assumes quirk for all family 6 before his
> change. 

That's why I was referring you to the older patch (also by him),
which converted from a model specific workaround to a universal
one.

Jan

Re: [PATCH v2 0/3] VPMU fixes

[Boris Ostrovsky]

On 12/03/2015 06:02 AM, Jan Beulich wrote:
>>>> On 03.12.15 at 11:46, <kevin.tian@intel.com> wrote:
>>>   From: Jan Beulich [mailto:JBeulich@suse.com]
>>> Sent: Thursday, December 03, 2015 6:36 PM
>>>
>>>>>> On 03.12.15 at 07:16, <kevin.tian@intel.com> wrote:
>>>>>   From: Dietmar Hahn [mailto:dietmar.hahn@ts.fujitsu.com]
>>>>> Sent: Wednesday, December 02, 2015 5:21 PM
>>>>>
>>>>> Am Mittwoch 02 Dezember 2015, 02:20:49 schrieb Tian, Kevin:
>>>>>>> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
>>>>>>> Sent: Wednesday, December 02, 2015 12:50 AM
>>>>>>>
>>>>>>> * Limit VPMU support to PMU versions 2, 3 and 4 (emulated at version 3
>> level)
>>>>>>> * Always implement family 6 VPMU quirk.
>>>>>>>    ==>  Intel folks: is the quirk needed for all family 6 processors or can
>> we
>>>>>>>                      limit it to certain models?
>>>>>> Let me confirm this information internally. btw could you provide a link
>>>>>> where you find out the original quirk information?
>>>>> http://old-list-archives.xenproject.org/xen-devel/2010-11/msg01157.html
>>>>> Dietmar.
>>>> Looks there's no formal errata recorded from original thread. Since the
>>>> patch sustains original behavior, I'm OK with this assumption.
>>> Except that the Link Dietmar provided makes it even more
>>> likely that this could be limited to certain models (since in that
>>> original patch version it covered just two). See also the vague
>>> statement in 75a92f551a ("Currently only a few Intel models
>>> have VPMU workaround turned on. It").
>> Well, I'm not sure about the whole history. If you look at Boris's
>> patch, the code already assumes quirk for all family 6 before his
>> change.
> That's why I was referring you to the older patch (also by him),
> which converted from a model specific workaround to a universal
> one.
>

These are two threads that are related to 75a92f551a:
http://lists.xenproject.org/archives/html/xen-devel/2013-05/msg01054.html
and
http://lists.xen.org/archives/html/xen-devel/2013-03/msg00998.html

The latter describes the actual problem that lead us to believe that the 
set of models that the original quirk covered was not enough. There was 
a suggestion that adding 43 (or 45) would be sufficient but then Konrad 
(who originally reported this) started hitting other problems and in the 
end it seems like we ended up with 75a92f551a as being the safest 
approach since Intel could not definitively confirm right set of models.

-boris

Re: [PATCH v2 0/3] VPMU fixes

[Tian, Kevin]

> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: Thursday, December 03, 2015 11:45 PM
> 
> On 12/03/2015 06:02 AM, Jan Beulich wrote:
> >>>> On 03.12.15 at 11:46, <kevin.tian@intel.com> wrote:
> >>>   From: Jan Beulich [mailto:JBeulich@suse.com]
> >>> Sent: Thursday, December 03, 2015 6:36 PM
> >>>
> >>>>>> On 03.12.15 at 07:16, <kevin.tian@intel.com> wrote:
> >>>>>   From: Dietmar Hahn [mailto:dietmar.hahn@ts.fujitsu.com]
> >>>>> Sent: Wednesday, December 02, 2015 5:21 PM
> >>>>>
> >>>>> Am Mittwoch 02 Dezember 2015, 02:20:49 schrieb Tian, Kevin:
> >>>>>>> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> >>>>>>> Sent: Wednesday, December 02, 2015 12:50 AM
> >>>>>>>
> >>>>>>> * Limit VPMU support to PMU versions 2, 3 and 4 (emulated at version 3
> >> level)
> >>>>>>> * Always implement family 6 VPMU quirk.
> >>>>>>>    ==>  Intel folks: is the quirk needed for all family 6 processors or can
> >> we
> >>>>>>>                      limit it to certain models?
> >>>>>> Let me confirm this information internally. btw could you provide a link
> >>>>>> where you find out the original quirk information?
> >>>>> http://old-list-archives.xenproject.org/xen-devel/2010-11/msg01157.html
> >>>>> Dietmar.
> >>>> Looks there's no formal errata recorded from original thread. Since the
> >>>> patch sustains original behavior, I'm OK with this assumption.
> >>> Except that the Link Dietmar provided makes it even more
> >>> likely that this could be limited to certain models (since in that
> >>> original patch version it covered just two). See also the vague
> >>> statement in 75a92f551a ("Currently only a few Intel models
> >>> have VPMU workaround turned on. It").
> >> Well, I'm not sure about the whole history. If you look at Boris's
> >> patch, the code already assumes quirk for all family 6 before his
> >> change.
> > That's why I was referring you to the older patch (also by him),
> > which converted from a model specific workaround to a universal
> > one.
> >
> 
> These are two threads that are related to 75a92f551a:
> http://lists.xenproject.org/archives/html/xen-devel/2013-05/msg01054.html
> and
> http://lists.xen.org/archives/html/xen-devel/2013-03/msg00998.html
> 
> The latter describes the actual problem that lead us to believe that the
> set of models that the original quirk covered was not enough. There was
> a suggestion that adding 43 (or 45) would be sufficient but then Konrad
> (who originally reported this) started hitting other problems and in the
> end it seems like we ended up with 75a92f551a as being the safest
> approach since Intel could not definitively confirm right set of models.
> 

I can help do another check with our HW guys. But I won't gate this
patch series being that:

- Looks there's no formal errata on this issue and my previous colleague 
(Haitao) didn't get it answered after his trying.

- We've using this safe approach for all family 6 models since 2013.

Thanks
Kevin

Re: [PATCH v2 0/3] VPMU fixes

[Tian, Kevin]

Add Andi who is our PMU expert. He will give some insight into this issue.

Thanks
Kevin

> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: Thursday, December 03, 2015 11:45 PM
> 
> On 12/03/2015 06:02 AM, Jan Beulich wrote:
> >>>> On 03.12.15 at 11:46, <kevin.tian@intel.com> wrote:
> >>>   From: Jan Beulich [mailto:JBeulich@suse.com]
> >>> Sent: Thursday, December 03, 2015 6:36 PM
> >>>
> >>>>>> On 03.12.15 at 07:16, <kevin.tian@intel.com> wrote:
> >>>>>   From: Dietmar Hahn [mailto:dietmar.hahn@ts.fujitsu.com]
> >>>>> Sent: Wednesday, December 02, 2015 5:21 PM
> >>>>>
> >>>>> Am Mittwoch 02 Dezember 2015, 02:20:49 schrieb Tian, Kevin:
> >>>>>>> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> >>>>>>> Sent: Wednesday, December 02, 2015 12:50 AM
> >>>>>>>
> >>>>>>> * Limit VPMU support to PMU versions 2, 3 and 4 (emulated at version 3
> >> level)
> >>>>>>> * Always implement family 6 VPMU quirk.
> >>>>>>>    ==>  Intel folks: is the quirk needed for all family 6 processors or can
> >> we
> >>>>>>>                      limit it to certain models?
> >>>>>> Let me confirm this information internally. btw could you provide a link
> >>>>>> where you find out the original quirk information?
> >>>>> http://old-list-archives.xenproject.org/xen-devel/2010-11/msg01157.html
> >>>>> Dietmar.
> >>>> Looks there's no formal errata recorded from original thread. Since the
> >>>> patch sustains original behavior, I'm OK with this assumption.
> >>> Except that the Link Dietmar provided makes it even more
> >>> likely that this could be limited to certain models (since in that
> >>> original patch version it covered just two). See also the vague
> >>> statement in 75a92f551a ("Currently only a few Intel models
> >>> have VPMU workaround turned on. It").
> >> Well, I'm not sure about the whole history. If you look at Boris's
> >> patch, the code already assumes quirk for all family 6 before his
> >> change.
> > That's why I was referring you to the older patch (also by him),
> > which converted from a model specific workaround to a universal
> > one.
> >
> 
> These are two threads that are related to 75a92f551a:
> http://lists.xenproject.org/archives/html/xen-devel/2013-05/msg01054.html
> and
> http://lists.xen.org/archives/html/xen-devel/2013-03/msg00998.html
> 
> The latter describes the actual problem that lead us to believe that the
> set of models that the original quirk covered was not enough. There was
> a suggestion that adding 43 (or 45) would be sufficient but then Konrad
> (who originally reported this) started hitting other problems and in the
> end it seems like we ended up with 75a92f551a as being the safest
> approach since Intel could not definitively confirm right set of models.
> 
> -boris
> 

Re: [PATCH v2 0/3] VPMU fixes

[Kleen, Andi]

I'm not aware of that specific quirk on Nehalem. Standard perf has a workaround for the errata below, but it sounds different from what you have.

But if it's a NHM/WSM problem your model numbers are certainly not enough.

You always need some kind of limiter for the PMI because it is possible to program the PMU that PMIs/NMIs appear back-to-back and lock up the system. But just programming them to 1 is not enough in this case, really have to be limited (perf both accounts and limits the frequency in time and the total CPU consumption of the PMI).
I think if you have such a generic limiter it would likely be able to handle this issue too.

Comments of the Nehalem workaround in the main perf code:

* Workaround for:
 *   Intel Errata AAK100 (model 26)
 *   Intel Errata AAP53  (model 30)
 *   Intel Errata BD53   (model 44)
 *
 * The official story:
 *   These chips need to be 'reset' when adding counters by programming the
 *   magic three (non-counting) events 0x4300B5, 0x4300D2, and 0x4300B1 either
 *   in sequence on the same PMC or on different PMCs.
 *
 * In practise it appears some of these events do in fact count, and
 * we need to programm all 4 events.

         * The Errata requires below steps:
         * 1) Clear MSR_IA32_PEBS_ENABLE and MSR_CORE_PERF_GLOBAL_CTRL;
         * 2) Configure 4 PERFEVTSELx with the magic events and clear
         *    the corresponding PMCx;
         * 3) set bit0~bit3 of MSR_CORE_PERF_GLOBAL_CTRL;
         * 4) Clear MSR_CORE_PERF_GLOBAL_CTRL;
         * 5) Clear 4 pairs of ERFEVTSELx and PMCx;

-Andi

Re: [PATCH v2 0/3] VPMU fixes

[Boris Ostrovsky]

On 12/07/2015 04:24 PM, Kleen, Andi wrote:
> I'm not aware of that specific quirk on Nehalem. Standard perf has a workaround for the errata below, but it sounds different from what you have.
>
> But if it's a NHM/WSM problem your model numbers are certainly not enough.

That's exactly the problem that we are having --- we don't know who 
exactly is affected, including whether it's limited to Nehalem.

>
> You always need some kind of limiter for the PMI because it is possible to program the PMU that PMIs/NMIs appear back-to-back and lock up the system. But just programming them to 1 is not enough in this case, really have to be limited (perf both accounts and limits the frequency in time and the total CPU consumption of the PMI).

Are you referring to 14c63f17b1 / update_perf_cpu_limits()?

> I think if you have such a generic limiter it would likely be able to handle this issue too.

In theory I think we shouldn't need to do this since when guest's VCPU 
is descheduled its PMU registers are no longer loaded into HW. So if the 
guest is programming PMU in such a way that it can no longer do any 
useful work --- well, that's the guest's problem.

The quirk that we have in Xen is apparently trying to prevent system 
lock-up.

I should also note that this only observed in HVM (i.e. with VMX) so 
perhaps this is something specific to virtualization. E.g. getting a PMI 
while executing a VMRUN or something...

>
> Comments of the Nehalem workaround in the main perf code:
>
> * Workaround for:
>   *   Intel Errata AAK100 (model 26)
>   *   Intel Errata AAP53  (model 30)
>   *   Intel Errata BD53   (model 44)
>   *
>   * The official story:
>   *   These chips need to be 'reset' when adding counters by programming the
>   *   magic three (non-counting) events 0x4300B5, 0x4300D2, and 0x4300B1 either
>   *   in sequence on the same PMC or on different PMCs.

These look like errata for *incorrect* counting and so it's up to guests 
to work around these (which the guests do, provided they have 
intel_pmu_nhm_workaround(), or an equivalent in another OS).


Thanks.
-boris


>   *
>   * In practise it appears some of these events do in fact count, and
>   * we need to programm all 4 events.
>
>           * The Errata requires below steps:
>           * 1) Clear MSR_IA32_PEBS_ENABLE and MSR_CORE_PERF_GLOBAL_CTRL;
>           * 2) Configure 4 PERFEVTSELx with the magic events and clear
>           *    the corresponding PMCx;
>           * 3) set bit0~bit3 of MSR_CORE_PERF_GLOBAL_CTRL;
>           * 4) Clear MSR_CORE_PERF_GLOBAL_CTRL;
>           * 5) Clear 4 pairs of ERFEVTSELx and PMCx;
>
> -Andi