[PATCH 0/2] Two VPMU/watchdog-related patches

[PATCH 0/2] Two VPMU/watchdog-related patches

[Boris Ostrovsky]

The first patch is to disable VPMU when watchdog is on since both are using
APIC_LVTPC register and VPMU, when running, will overwrite watchdog's settings.

The second patch is update to the earlier (reverted) commit 8097616. We prevent
guests's updates to APIC_LVTPC when VPMU is disabled. Without this a guest may
redirect a watchdog interrupt to VPMU which will not be able to handle it.

I will need to update a couple of later patches in the VPMU series to keep this
logic. Jan, Andrew --- do you want me to resend the whole series again or only
unapplied patches (and keep original numbering)?

Boris Ostrovsky (2):
  x86/VPMU: Disable VPMU when NMI watchdog is on
  x86/VPMU: Handle APIC_LVTPC accesses

 xen/arch/x86/hvm/svm/vpmu.c       |  4 ----
 xen/arch/x86/hvm/vlapic.c         |  3 +++
 xen/arch/x86/hvm/vmx/vpmu_core2.c | 17 -----------------
 xen/arch/x86/hvm/vpmu.c           | 22 +++++++++++++++++++++-
 xen/arch/x86/nmi.c                | 10 +++++++++-
 xen/include/asm-x86/hvm/vpmu.h    |  2 ++
 6 files changed, 35 insertions(+), 23 deletions(-)

-- 
1.8.1.4

[PATCH 1/2] x86/VPMU: Disable VPMU when NMI watchdog is on

[Boris Ostrovsky]

NMI watchdog sets APIC_LVTPC register to generate an NMI when PMU counter
overflow occurs. This may be overwritten by VPMU code later, effectively
turning off the watchdog.

We should disable VPMU when NMI watchdog is running.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/arch/x86/hvm/vpmu.c        |  9 ++++++++-
 xen/arch/x86/nmi.c             | 10 +++++++++-
 xen/include/asm-x86/hvm/vpmu.h |  1 +
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/vpmu.c b/xen/arch/x86/hvm/vpmu.c
index 63b2158..ee05840 100644
--- a/xen/arch/x86/hvm/vpmu.c
+++ b/xen/arch/x86/hvm/vpmu.c
@@ -24,6 +24,7 @@
 #include <asm/regs.h>
 #include <asm/types.h>
 #include <asm/msr.h>
+#include <asm/nmi.h>
 #include <asm/hvm/support.h>
 #include <asm/hvm/vmx/vmx.h>
 #include <asm/hvm/vmx/vmcs.h>
@@ -37,7 +38,7 @@
  * "vpmu=off" : vpmu generally disabled
  * "vpmu=bts" : vpmu enabled and Intel BTS feature switched on.
  */
-static unsigned int __read_mostly opt_vpmu_enabled;
+unsigned int __read_mostly opt_vpmu_enabled;
 static void parse_vpmu_param(char *s);
 custom_param("vpmu", parse_vpmu_param);
 
@@ -59,6 +60,12 @@ static void __init parse_vpmu_param(char *s)
         }
         /* fall through */
     case 1:
+        if ( opt_watchdog )
+        {
+            printk("NMI watchdog is enabled. Disabling VPMU\n");
+            opt_vpmu_enabled = 0;
+            break;
+        }
         opt_vpmu_enabled |= VPMU_BOOT_ENABLED;
         break;
     }
diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
index 98c1e15..a0ade45 100644
--- a/xen/arch/x86/nmi.c
+++ b/xen/arch/x86/nmi.c
@@ -33,6 +33,7 @@
 #include <asm/debugger.h>
 #include <asm/div64.h>
 #include <asm/apic.h>
+#include <asm/hvm/vpmu.h>
 
 unsigned int nmi_watchdog = NMI_NONE;
 static unsigned int nmi_hz = HZ;
@@ -52,7 +53,7 @@ static void __init parse_watchdog(char *s)
     if ( !*s )
     {
         opt_watchdog = 1;
-        return;
+        goto out;
     }
 
     switch ( parse_bool(s) )
@@ -67,6 +68,13 @@ static void __init parse_watchdog(char *s)
 
     if ( !strcmp(s, "force") )
         watchdog_force = opt_watchdog = 1;
+
+ out:
+    if ( opt_watchdog && opt_vpmu_enabled )
+    {
+        printk("NMI watchdog is enabled. Disabling VPMU\n");
+        opt_vpmu_enabled = 0;
+    }
 }
 custom_param("watchdog", parse_watchdog);
 
diff --git a/xen/include/asm-x86/hvm/vpmu.h b/xen/include/asm-x86/hvm/vpmu.h
index ddc2748..3fee537 100644
--- a/xen/include/asm-x86/hvm/vpmu.h
+++ b/xen/include/asm-x86/hvm/vpmu.h
@@ -29,6 +29,7 @@
 #define VPMU_BOOT_ENABLED 0x1    /* vpmu generally enabled. */
 #define VPMU_BOOT_BTS     0x2    /* Intel BTS feature wanted. */
 
+extern unsigned int opt_vpmu_enabled;
 
 #define msraddr_to_bitpos(x) (((x)&0xffff) + ((x)>>31)*0x2000)
 #define vcpu_vpmu(vcpu)   (&((vcpu)->arch.hvm_vcpu.vpmu))
-- 
1.8.1.4

[PATCH 2/2] x86/VPMU: Handle APIC_LVTPC accesses

[Boris Ostrovsky]

Don't have the hypervisor update APIC_LVTPC when _it_ thinks the vector
should be updated. Instead, handle guest's APIC_LVTPC accesses and write what
the guest explicitly wanted (but only when VPMU is enabled).

This is updated version of commit 8097616fbdda that was reverted by
cc3404093c85. Unlike the previous version, we don't update APIC_LVTPC
when VPMU is disabled to avoid interfering with NMI watchdog (which
runs only when VPMU is off).

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>
Tested-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>
---
 xen/arch/x86/hvm/svm/vpmu.c       |  4 ----
 xen/arch/x86/hvm/vlapic.c         |  3 +++
 xen/arch/x86/hvm/vmx/vpmu_core2.c | 17 -----------------
 xen/arch/x86/hvm/vpmu.c           | 13 +++++++++++++
 xen/include/asm-x86/hvm/vpmu.h    |  1 +
 5 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/vpmu.c b/xen/arch/x86/hvm/svm/vpmu.c
index 19777e3..64dc167 100644
--- a/xen/arch/x86/hvm/svm/vpmu.c
+++ b/xen/arch/x86/hvm/svm/vpmu.c
@@ -302,8 +302,6 @@ static int amd_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content,
         if ( !acquire_pmu_ownership(PMU_OWNER_HVM) )
             return 1;
         vpmu_set(vpmu, VPMU_RUNNING);
-        apic_write(APIC_LVTPC, PMU_APIC_VECTOR);
-        vpmu->hw_lapic_lvtpc = PMU_APIC_VECTOR;
 
         if ( has_hvm_container_vcpu(v) &&
              !((struct amd_vpmu_context *)vpmu->context)->msr_bitmap_set )
@@ -314,8 +312,6 @@ static int amd_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content,
     if ( (get_pmu_reg_type(msr) == MSR_TYPE_CTRL) &&
         (is_pmu_enabled(msr_content) == 0) && vpmu_is_set(vpmu, VPMU_RUNNING) )
     {
-        apic_write(APIC_LVTPC, PMU_APIC_VECTOR | APIC_LVT_MASKED);
-        vpmu->hw_lapic_lvtpc = PMU_APIC_VECTOR | APIC_LVT_MASKED;
         vpmu_reset(vpmu, VPMU_RUNNING);
         if ( has_hvm_container_vcpu(v) &&
              ((struct amd_vpmu_context *)vpmu->context)->msr_bitmap_set )
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 8062f31..5da6d8f 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -38,6 +38,7 @@
 #include <asm/hvm/support.h>
 #include <asm/hvm/vmx/vmx.h>
 #include <asm/hvm/nestedhvm.h>
+#include <asm/hvm/vpmu.h>
 #include <public/hvm/ioreq.h>
 #include <public/hvm/params.h>
 
@@ -777,6 +778,8 @@ static int vlapic_reg_write(struct vcpu *v,
         }
         if ( (offset == APIC_LVTT) && !(val & APIC_LVT_MASKED) )
             pt_may_unmask_irq(NULL, &vlapic->pt);
+        if ( offset == APIC_LVTPC )
+            vpmu_lvtpc_update(val);
         break;
 
     case APIC_TMICT:
diff --git a/xen/arch/x86/hvm/vmx/vpmu_core2.c b/xen/arch/x86/hvm/vmx/vpmu_core2.c
index 4d0e9a8..7793145 100644
--- a/xen/arch/x86/hvm/vmx/vpmu_core2.c
+++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c
@@ -519,19 +519,6 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content,
     else
         vpmu_reset(vpmu, VPMU_RUNNING);
 
-    /* Setup LVTPC in local apic */
-    if ( vpmu_is_set(vpmu, VPMU_RUNNING) &&
-         is_vlapic_lvtpc_enabled(vcpu_vlapic(v)) )
-    {
-        apic_write_around(APIC_LVTPC, PMU_APIC_VECTOR);
-        vpmu->hw_lapic_lvtpc = PMU_APIC_VECTOR;
-    }
-    else
-    {
-        apic_write_around(APIC_LVTPC, PMU_APIC_VECTOR | APIC_LVT_MASKED);
-        vpmu->hw_lapic_lvtpc = PMU_APIC_VECTOR | APIC_LVT_MASKED;
-    }
-
     if ( type != MSR_TYPE_GLOBAL )
     {
         u64 mask;
@@ -697,10 +684,6 @@ static int core2_vpmu_do_interrupt(struct cpu_user_regs *regs)
             return 0;
     }
 
-    /* HW sets the MASK bit when performance counter interrupt occurs*/
-    vpmu->hw_lapic_lvtpc = apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED;
-    apic_write_around(APIC_LVTPC, vpmu->hw_lapic_lvtpc);
-
     return 1;
 }
 
diff --git a/xen/arch/x86/hvm/vpmu.c b/xen/arch/x86/hvm/vpmu.c
index ee05840..2ac9218 100644
--- a/xen/arch/x86/hvm/vpmu.c
+++ b/xen/arch/x86/hvm/vpmu.c
@@ -71,6 +71,19 @@ static void __init parse_vpmu_param(char *s)
     }
 }
 
+void vpmu_lvtpc_update(uint32_t val)
+{
+    struct vpmu_struct *vpmu;
+
+    if ( !opt_vpmu_enabled )
+        return;
+
+    vpmu = vcpu_vpmu(current);
+
+    vpmu->hw_lapic_lvtpc = PMU_APIC_VECTOR | (val & APIC_LVT_MASKED);
+    apic_write(APIC_LVTPC, vpmu->hw_lapic_lvtpc);
+}
+
 int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, uint64_t supported)
 {
     struct vpmu_struct *vpmu = vcpu_vpmu(current);
diff --git a/xen/include/asm-x86/hvm/vpmu.h b/xen/include/asm-x86/hvm/vpmu.h
index 3fee537..3f52847 100644
--- a/xen/include/asm-x86/hvm/vpmu.h
+++ b/xen/include/asm-x86/hvm/vpmu.h
@@ -105,6 +105,7 @@ static inline bool_t vpmu_are_all_set(const struct vpmu_struct *vpmu,
     return !!((vpmu->flags & mask) == mask);
 }
 
+void vpmu_lvtpc_update(uint32_t val);
 int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, uint64_t supported);
 int vpmu_do_rdmsr(unsigned int msr, uint64_t *msr_content);
 void vpmu_do_interrupt(struct cpu_user_regs *regs);
-- 
1.8.1.4

Re: [PATCH 1/2] x86/VPMU: Disable VPMU when NMI watchdog is on

[Andrew Cooper]

On 28/01/2015 19:56, Boris Ostrovsky wrote:
> NMI watchdog sets APIC_LVTPC register to generate an NMI when PMU counter
> overflow occurs. This may be overwritten by VPMU code later, effectively
> turning off the watchdog.
>
> We should disable VPMU when NMI watchdog is running.
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

I completely agree with the aim, but this patch is clunky and you have
missed the case where neither 'watchdog' nor 'vpmu' is specified on the
command line, but the booleans have been tweaked (which is the XenServer
way of choosing defaults while keeping the length of the command line down).

A more simple approach, which doesn't involve exposing opt_vpmu_enabled
or changing any nmi code, would be to have a check in vpmu_initialise()
which checks for opt_watchdog and opt_vpmu_enabled and bail.

Looking at the code in tree, it seems odd opt_vpmu_enabled is passed to
the sub initialise functions to be acted upon.  Is this something which
is cleaned up or changed in your series?  If not, it perhaps should be. 
Also, under what conditions would you expect this initialise function to
be called on a vcpu, and to find its vpmu already active?

~Andrew

Re: [PATCH 1/2] x86/VPMU: Disable VPMU when NMI watchdog is on

[Boris Ostrovsky]

On 01/28/2015 04:49 PM, Andrew Cooper wrote:
> On 28/01/2015 19:56, Boris Ostrovsky wrote:
>> NMI watchdog sets APIC_LVTPC register to generate an NMI when PMU counter
>> overflow occurs. This may be overwritten by VPMU code later, effectively
>> turning off the watchdog.
>>
>> We should disable VPMU when NMI watchdog is running.
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>
> I completely agree with the aim, but this patch is clunky and you have
> missed the case where neither 'watchdog' nor 'vpmu' is specified on the
> command line, but the booleans have been tweaked (which is the XenServer
> way of choosing defaults while keeping the length of the command line down).

You mean binary patching? Or does XenServer change those flags before 
compilation? Yes, I haven't thought about this.

>
> A more simple approach, which doesn't involve exposing opt_vpmu_enabled
> or changing any nmi code, would be to have a check in vpmu_initialise()
> which checks for opt_watchdog and opt_vpmu_enabled and bail.

Right, I can do that (in light of what you said above).

I want to have a warning during boot so that users know that even though 
they specified certain boot parameters these parameters are ignored. If 
we were to print this warning in vpmu_initialise() then it would not be 
displayed until first HVM guest is started.

OTOH, when the full series is applied vpmu initialization will be done 
in initcall, which is where we can warn.

>
> Looking at the code in tree, it seems odd opt_vpmu_enabled is passed to
> the sub initialise functions to be acted upon.  Is this something which
> is cleaned up or changed in your series?  If not, it perhaps should be.

These flags are indeed removed in the series.

> Also, under what conditions would you expect this initialise function to
> be called on a vcpu, and to find its vpmu already active?

You mean why it has
	if ( vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) )
	        vpmu_destroy(v);
	vpmu_clear(vpmu);
	vpmu->context = NULL;
at the top?

I am not sure why it's there. Migration or hotplug? Probably not since 
this is called via vcpu_initialise() and that is done only once.

I apparently kept this in my series as well, btw.

-boris

Re: [PATCH 1/2] x86/VPMU: Disable VPMU when NMI watchdog is on

[Andrew Cooper]

On 28/01/2015 22:33, Boris Ostrovsky wrote:
> On 01/28/2015 04:49 PM, Andrew Cooper wrote:
>> On 28/01/2015 19:56, Boris Ostrovsky wrote:
>>> NMI watchdog sets APIC_LVTPC register to generate an NMI when PMU
>>> counter
>>> overflow occurs. This may be overwritten by VPMU code later,
>>> effectively
>>> turning off the watchdog.
>>>
>>> We should disable VPMU when NMI watchdog is running.
>>>
>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>
>> I completely agree with the aim, but this patch is clunky and you have
>> missed the case where neither 'watchdog' nor 'vpmu' is specified on the
>> command line, but the booleans have been tweaked (which is the XenServer
>> way of choosing defaults while keeping the length of the command line
>> down).
>
> You mean binary patching? Or does XenServer change those flags before
> compilation? Yes, I haven't thought about this.

We patch the source before compiling.

https://github.com/xenserver/xen-4.5.pg/blob/master/master/xen-tweak-cmdline-defaults.patch

>
>>
>> A more simple approach, which doesn't involve exposing opt_vpmu_enabled
>> or changing any nmi code, would be to have a check in vpmu_initialise()
>> which checks for opt_watchdog and opt_vpmu_enabled and bail.
>
> Right, I can do that (in light of what you said above).
>
> I want to have a warning during boot so that users know that even
> though they specified certain boot parameters these parameters are
> ignored. If we were to print this warning in vpmu_initialise() then it
> would not be displayed until first HVM guest is started.
>
> OTOH, when the full series is applied vpmu initialization will be done
> in initcall, which is where we can warn.

This sounds like the best long term solution, in which case it is fine
to do the minimum required to make the following patch safe to use, and
let the cleanup/tweaking happen at appropriate later points in the series.

>
>>
>> Looking at the code in tree, it seems odd opt_vpmu_enabled is passed to
>> the sub initialise functions to be acted upon.  Is this something which
>> is cleaned up or changed in your series?  If not, it perhaps should be.
>
> These flags are indeed removed in the series.
>
>> Also, under what conditions would you expect this initialise function to
>> be called on a vcpu, and to find its vpmu already active?
>
> You mean why it has
>     if ( vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) )
>             vpmu_destroy(v);
>     vpmu_clear(vpmu);
>     vpmu->context = NULL;
> at the top?
>
> I am not sure why it's there. Migration or hotplug? Probably not since
> this is called via vcpu_initialise() and that is done only once.
>
> I apparently kept this in my series as well, btw.

Perhaps more fodder for an appropriate cleanup patch.

~Andrew

Re: [PATCH 1/2] x86/VPMU: Disable VPMU when NMI watchdog is on

[Boris Ostrovsky]

On 01/28/2015 05:41 PM, Andrew Cooper wrote:
> On 28/01/2015 22:33, Boris Ostrovsky wrote:
>> On 01/28/2015 04:49 PM, Andrew Cooper wrote:
>>> On 28/01/2015 19:56, Boris Ostrovsky wrote:
>>>> NMI watchdog sets APIC_LVTPC register to generate an NMI when PMU
>>>> counter
>>>> overflow occurs. This may be overwritten by VPMU code later,
>>>> effectively
>>>> turning off the watchdog.
>>>>
>>>> We should disable VPMU when NMI watchdog is running.
>>>>
>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>
>>> I completely agree with the aim, but this patch is clunky and you have
>>> missed the case where neither 'watchdog' nor 'vpmu' is specified on the
>>> command line, but the booleans have been tweaked (which is the XenServer
>>> way of choosing defaults while keeping the length of the command line
>>> down).
>>
>> You mean binary patching? Or does XenServer change those flags before
>> compilation? Yes, I haven't thought about this.
>
> We patch the source before compiling.
>
> https://github.com/xenserver/xen-4.5.pg/blob/master/master/xen-tweak-cmdline-defaults.patch


How does this enable NMI watchdog? This patch sets watchdog_force but 
that alone doesn't enable anything. Or are there other changes that set 
opt_watchdog (which I was going to test as you suggested below).

-boris

>
>>
>>>
>>> A more simple approach, which doesn't involve exposing opt_vpmu_enabled
>>> or changing any nmi code, would be to have a check in vpmu_initialise()
>>> which checks for opt_watchdog and opt_vpmu_enabled and bail.
>>

Re: [PATCH 0/2] Two VPMU/watchdog-related patches

[Jan Beulich]

>>> On 28.01.15 at 20:56, <boris.ostrovsky@oracle.com> wrote:
> I will need to update a couple of later patches in the VPMU series to keep 
> this
> logic. Jan, Andrew --- do you want me to resend the whole series again or only
> unapplied patches (and keep original numbering)?

Only the un-applied ones, and properly re-numbered from 1 please
(after all during earlier extension of the series you also didn't keep
the numbering consistent).

Jan

Re: [PATCH 1/2] x86/VPMU: Disable VPMU when NMI watchdog is on

[Jan Beulich]

>>> On 28.01.15 at 20:56, <boris.ostrovsky@oracle.com> wrote:
> @@ -59,6 +60,12 @@ static void __init parse_vpmu_param(char *s)
>          }
>          /* fall through */
>      case 1:
> +        if ( opt_watchdog )
> +        {
> +            printk("NMI watchdog is enabled. Disabling VPMU\n");
> +            opt_vpmu_enabled = 0;
> +            break;
> +        }
>          opt_vpmu_enabled |= VPMU_BOOT_ENABLED;
>          break;
>      }

Not only to address Andrew's concerns this needs to be changed:
Logging messages from cmdline argument parsing functions is only
marginally useful - they won't appear on the serial console. But
afaict that'll go away anyway by consolidating the patch into simply
checking opt_watchdog from vPMU code.

Jan

Re: [PATCH 2/2] x86/VPMU: Handle APIC_LVTPC accesses

[Jan Beulich]

>>> On 28.01.15 at 20:56, <boris.ostrovsky@oracle.com> wrote:
> Don't have the hypervisor update APIC_LVTPC when _it_ thinks the vector
> should be updated. Instead, handle guest's APIC_LVTPC accesses and write 
> what
> the guest explicitly wanted (but only when VPMU is enabled).
> 
> This is updated version of commit 8097616fbdda that was reverted by
> cc3404093c85. Unlike the previous version, we don't update APIC_LVTPC
> when VPMU is disabled to avoid interfering with NMI watchdog (which
> runs only when VPMU is off).
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Acked-by: Kevin Tian <kevin.tian@intel.com>
> Reviewed-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>
> Tested-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>

Even leaving aside the functionality change, on an updated patch
the previous version of which needed to be reverted, retaining
_any_ such tags is wrong from my pov. I asked you before to be
more conservative with retaining tags, and I'm now going to reserve
the right to no longer point out such issues, but silently drop such
patches coming from you.

Jan

Re: [PATCH 1/2] x86/VPMU: Disable VPMU when NMI watchdog is on

[Andrew Cooper]

On 28/01/15 23:36, Boris Ostrovsky wrote:
> On 01/28/2015 05:41 PM, Andrew Cooper wrote:
>> On 28/01/2015 22:33, Boris Ostrovsky wrote:
>>> On 01/28/2015 04:49 PM, Andrew Cooper wrote:
>>>> On 28/01/2015 19:56, Boris Ostrovsky wrote:
>>>>> NMI watchdog sets APIC_LVTPC register to generate an NMI when PMU
>>>>> counter
>>>>> overflow occurs. This may be overwritten by VPMU code later,
>>>>> effectively
>>>>> turning off the watchdog.
>>>>>
>>>>> We should disable VPMU when NMI watchdog is running.
>>>>>
>>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>>
>>>> I completely agree with the aim, but this patch is clunky and you have
>>>> missed the case where neither 'watchdog' nor 'vpmu' is specified on
>>>> the
>>>> command line, but the booleans have been tweaked (which is the
>>>> XenServer
>>>> way of choosing defaults while keeping the length of the command line
>>>> down).
>>>
>>> You mean binary patching? Or does XenServer change those flags before
>>> compilation? Yes, I haven't thought about this.
>>
>> We patch the source before compiling.
>>
>> https://github.com/xenserver/xen-4.5.pg/blob/master/master/xen-tweak-cmdline-defaults.patch
>>
>
>
> How does this enable NMI watchdog? This patch sets watchdog_force but
> that alone doesn't enable anything. Or are there other changes that
> set opt_watchdog (which I was going to test as you suggested below).
>

It doesn't.  I was just citing a plausible example.  (For reasons which
temporally elude me, "watchdog" is still a set as a proper item on the
command line).  Apologies if I was not clear.

~Andrew

Re: [PATCH 1/2] x86/VPMU: Disable VPMU when NMI watchdog is on

[Boris Ostrovsky]

On 01/29/2015 06:46 AM, Jan Beulich wrote:
>>>> On 28.01.15 at 20:56, <boris.ostrovsky@oracle.com> wrote:
>> @@ -59,6 +60,12 @@ static void __init parse_vpmu_param(char *s)
>>           }
>>           /* fall through */
>>       case 1:
>> +        if ( opt_watchdog )
>> +        {
>> +            printk("NMI watchdog is enabled. Disabling VPMU\n");
>> +            opt_vpmu_enabled = 0;
>> +            break;
>> +        }
>>           opt_vpmu_enabled |= VPMU_BOOT_ENABLED;
>>           break;
>>       }
>
> Not only to address Andrew's concerns this needs to be changed:
> Logging messages from cmdline argument parsing functions is only
> marginally useful - they won't appear on the serial console. But
> afaict that'll go away anyway by consolidating the patch into simply
> checking opt_watchdog from vPMU code.



Not on the console, but the do show up in the log:

root@haswell> xl dmesg|grep -i vpmu
(XEN) NMI watchdog is enabled. Disabling VPMU
(XEN) Command line: placeholder conring_size=512k loglvl=all 
sync_console=true flask_enforcing=1 com1=38400,8n1,pci console=com1 
watchdog vpmu
root@haswell>

But regardless, I am going to do the opt_watchdog check in VPMU init code.

-boris

Re: [PATCH 2/2] x86/VPMU: Handle APIC_LVTPC accesses

[Boris Ostrovsky]

On 01/29/2015 06:54 AM, Jan Beulich wrote:
>>>> On 28.01.15 at 20:56, <boris.ostrovsky@oracle.com> wrote:
>> Don't have the hypervisor update APIC_LVTPC when _it_ thinks the vector
>> should be updated. Instead, handle guest's APIC_LVTPC accesses and write
>> what
>> the guest explicitly wanted (but only when VPMU is enabled).
>>
>> This is updated version of commit 8097616fbdda that was reverted by
>> cc3404093c85. Unlike the previous version, we don't update APIC_LVTPC
>> when VPMU is disabled to avoid interfering with NMI watchdog (which
>> runs only when VPMU is off).
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Acked-by: Kevin Tian <kevin.tian@intel.com>
>> Reviewed-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>
>> Tested-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>
>
> Even leaving aside the functionality change, on an updated patch
> the previous version of which needed to be reverted, retaining
> _any_ such tags is wrong from my pov. I asked you before to be
> more conservative with retaining tags, and I'm now going to reserve
> the right to no longer point out such issues, but silently drop such
> patches coming from you.


I dropped your tag from this patch.

I kept Kevin's since he ACKed Intel changes and they are the same in 
this version.

Dietmar's tag was left here since he tested this patch without watchdog 
anyway and the 2-line addition here would not change anything.

However, if you insist on dropping all tags even for minor changes like 
that (and "determining what "minor" is is a judgment call) I will 
certainly do that in the future.


-boris

Re: [PATCH 2/2] x86/VPMU: Handle APIC_LVTPC accesses

[Jan Beulich]

>>> On 29.01.15 at 16:28, <boris.ostrovsky@oracle.com> wrote:
> However, if you insist on dropping all tags even for minor changes like 
> that (and "determining what "minor" is is a judgment call) I will 
> certainly do that in the future.

I don't mind you (and others) keeping them for minor changes. But a
small change isn't necessarily a minor one - that's why I pointed out
that this patch having caused a regression and hence having got
reverted is a relevant aspect here. Neither tester nor reviewer (in
this case the same person) spotted the issue up front. Having kept
Kevin is indeed a possibly acceptable thing (and I thought about
making this point in my earlier reply, but then decided it would only
risk confusion wrt the point I was trying to make).

Jan