[PATCH v2 0/3] A few VPMU/watchdog-related patches

[PATCH v2 0/3] A few VPMU/watchdog-related patches

[Boris Ostrovsky]

Changes in v2:
* Added NMI cleanup patch
* Disaple VPMU in vpmu_initialise() instead of doing it in options parsing routines
* Update documentation by mentioning that VPMU doesn't work with watchdog
* Drop some sign-off area tags


The first patch is a light cleanup of nmi_watchdog usage. (I removed
NMI_INVALID definition but left NMI_IO_APIC. Do we actually need it? I
don't see anyone using it except for one place, which doesn't really
use it neither)

The second 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 last 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.

Boris Ostrovsky (3):
  x86/nmi: Cleanup usage of nmi_watchdog
  x86/VPMU: Disable VPMU when NMI watchdog is on
  x86/VPMU: Handle APIC_LVTPC accesses

 docs/misc/xen-command-line.markdown |  2 ++
 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             | 21 +++++++++++++++++++++
 xen/arch/x86/nmi.c                  |  6 +++---
 xen/arch/x86/traps.c                |  3 ++-
 xen/include/asm-x86/apic.h          |  1 -
 xen/include/asm-x86/hvm/vpmu.h      |  1 +
 9 files changed, 32 insertions(+), 26 deletions(-)

-- 
1.8.1.4

[PATCH v2 1/3] x86/nmi: Cleanup usage of nmi_watchdog

[Boris Ostrovsky]

Use NMI_NONE when testing whether NMI watchdog is off.

Remove unused NMI_INVALID macro.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/arch/x86/nmi.c         | 4 ++--
 xen/arch/x86/traps.c       | 3 ++-
 xen/include/asm-x86/apic.h | 1 -
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
index 98c1e15..2ab97a0 100644
--- a/xen/arch/x86/nmi.c
+++ b/xen/arch/x86/nmi.c
@@ -148,7 +148,7 @@ int __init check_nmi_watchdog (void)
     int cpu;
     bool_t ok = 1;
 
-    if ( !nmi_watchdog )
+    if ( nmi_watchdog == NMI_NONE )
         return 0;
 
     printk("Testing NMI watchdog on all CPUs:");
@@ -361,7 +361,7 @@ static int __pminit setup_p4_watchdog(void)
 
 void __pminit setup_apic_nmi_watchdog(void)
 {
-    if (!nmi_watchdog)
+    if ( nmi_watchdog == NMI_NONE )
         return;
 
     switch (boot_cpu_data.x86_vendor) {
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index ec324b0..f5516dc 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -3385,7 +3385,8 @@ void do_nmi(const struct cpu_user_regs *regs)
     if ( nmi_callback(regs, cpu) )
         return;
 
-    if ( !nmi_watchdog || (!nmi_watchdog_tick(regs) && watchdog_force) )
+    if ( (nmi_watchdog == NMI_NONE) ||
+         (!nmi_watchdog_tick(regs) && watchdog_force) )
         handle_unknown = 1;
 
     /* Only the BSP gets external NMIs from the system. */
diff --git a/xen/include/asm-x86/apic.h b/xen/include/asm-x86/apic.h
index 6697245..be9a535 100644
--- a/xen/include/asm-x86/apic.h
+++ b/xen/include/asm-x86/apic.h
@@ -221,7 +221,6 @@ extern unsigned int nmi_watchdog;
 #define NMI_NONE	0
 #define NMI_IO_APIC	1
 #define NMI_LOCAL_APIC	2
-#define NMI_INVALID	3
 
 #else /* !CONFIG_X86_LOCAL_APIC */
 static inline int lapic_suspend(void) {return 0;}
-- 
1.8.1.4

[PATCH v2 2/3] 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>
---
 docs/misc/xen-command-line.markdown | 2 ++
 xen/arch/x86/hvm/vpmu.c             | 8 ++++++++
 xen/arch/x86/nmi.c                  | 2 +-
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 2274e74..bc316be 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1346,6 +1346,8 @@ wrong behaviour (see handle\_pmc\_quirk()).
 If 'vpmu=bts' is specified the virtualisation of the Branch Trace Store (BTS)
 feature is switched on on Intel processors supporting this feature.
 
+Note that if **watchdog** option is also specified vpmu will be turned off.
+
 *Warning:*
 As the BTS virtualisation is not 100% safe and because of the nehalem quirk
 don't use the vpmu flag on production systems with Intel cpus!
diff --git a/xen/arch/x86/hvm/vpmu.c b/xen/arch/x86/hvm/vpmu.c
index 63b2158..68cb3f8 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>
@@ -219,6 +220,13 @@ void vpmu_initialise(struct vcpu *v)
     uint8_t vendor = current_cpu_data.x86_vendor;
     int ret;
 
+    /* NMI watchdog uses LVTPC and HW counter */
+    if ( opt_watchdog && opt_vpmu_enabled )
+    {
+        printk(XENLOG_G_WARNING "NMI watchdog is enabled. Turning VPMU off.\n");
+        opt_vpmu_enabled = 0;
+    }
+
     if ( is_pvh_vcpu(v) )
         return;
 
diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
index 2ab97a0..9b8eb50 100644
--- a/xen/arch/x86/nmi.c
+++ b/xen/arch/x86/nmi.c
@@ -42,7 +42,7 @@ static DEFINE_PER_CPU(struct timer, nmi_timer);
 static DEFINE_PER_CPU(unsigned int, nmi_timer_ticks);
 
 /* opt_watchdog: If true, run a watchdog NMI on each processor. */
-bool_t __initdata opt_watchdog = 0;
+bool_t __read_mostly opt_watchdog = 0;
 
 /* watchdog_force: If true, process unknown NMIs when running the watchdog. */
 bool_t watchdog_force = 0;
-- 
1.8.1.4

[PATCH v2 3/3] 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>
---
 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 68cb3f8..ee2b109 100644
--- a/xen/arch/x86/hvm/vpmu.c
+++ b/xen/arch/x86/hvm/vpmu.c
@@ -65,6 +65,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 ddc2748..9c4e65a 100644
--- a/xen/include/asm-x86/hvm/vpmu.h
+++ b/xen/include/asm-x86/hvm/vpmu.h
@@ -104,6 +104,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 v2 0/3] A few VPMU/watchdog-related patches

[Jan Beulich]

>>> On 29.01.15 at 18:58, <boris.ostrovsky@oracle.com> wrote:
> The first patch is a light cleanup of nmi_watchdog usage. (I removed
> NMI_INVALID definition but left NMI_IO_APIC. Do we actually need it? I
> don't see anyone using it except for one place, which doesn't really
> use it neither)

As we really should gain back support for NMI_IO_APIC (considering
that there are quite a few systems where the LAPIC variant doesn't
work), I think it shouldn't be dropped.

Jan

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

[Jan Beulich]

>>> On 29.01.15 at 18:58, <boris.ostrovsky@oracle.com> wrote:
> @@ -219,6 +220,13 @@ void vpmu_initialise(struct vcpu *v)
>      uint8_t vendor = current_cpu_data.x86_vendor;
>      int ret;
>  
> +    /* NMI watchdog uses LVTPC and HW counter */
> +    if ( opt_watchdog && opt_vpmu_enabled )
> +    {
> +        printk(XENLOG_G_WARNING "NMI watchdog is enabled. Turning VPMU off.\n");
> +        opt_vpmu_enabled = 0;
> +    }

I think this should be a boot time warning (and not guest level, so it
won't become suppressed due to rate limiting when default are in
effect), and I thought you said so on the previous version's thread
too. If there's no other suitable place, just add a small initcall.

> --- a/xen/arch/x86/nmi.c
> +++ b/xen/arch/x86/nmi.c
> @@ -42,7 +42,7 @@ static DEFINE_PER_CPU(struct timer, nmi_timer);
>  static DEFINE_PER_CPU(unsigned int, nmi_timer_ticks);
>  
>  /* opt_watchdog: If true, run a watchdog NMI on each processor. */
> -bool_t __initdata opt_watchdog = 0;
> +bool_t __read_mostly opt_watchdog = 0;

The above would eliminate the need for this change too.

Jan