* [PATCH 0/2] Two VPMU/watchdog-related patches @ 2015-01-28 19:56 Boris Ostrovsky 2015-01-28 19:56 ` [PATCH 1/2] x86/VPMU: Disable VPMU when NMI watchdog is on Boris Ostrovsky ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Boris Ostrovsky @ 2015-01-28 19:56 UTC (permalink / raw) To: JBeulich, andrew.cooper3 Cc: xen-devel, kevin.tian, boris.ostrovsky, dietmar.hahn 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] x86/VPMU: Disable VPMU when NMI watchdog is on 2015-01-28 19:56 [PATCH 0/2] Two VPMU/watchdog-related patches Boris Ostrovsky @ 2015-01-28 19:56 ` Boris Ostrovsky 2015-01-28 21:49 ` Andrew Cooper 2015-01-29 11:46 ` Jan Beulich 2015-01-28 19:56 ` [PATCH 2/2] x86/VPMU: Handle APIC_LVTPC accesses Boris Ostrovsky 2015-01-29 11:37 ` [PATCH 0/2] Two VPMU/watchdog-related patches Jan Beulich 2 siblings, 2 replies; 14+ messages in thread From: Boris Ostrovsky @ 2015-01-28 19:56 UTC (permalink / raw) To: JBeulich, andrew.cooper3 Cc: xen-devel, kevin.tian, boris.ostrovsky, dietmar.hahn 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] x86/VPMU: Disable VPMU when NMI watchdog is on 2015-01-28 19:56 ` [PATCH 1/2] x86/VPMU: Disable VPMU when NMI watchdog is on Boris Ostrovsky @ 2015-01-28 21:49 ` Andrew Cooper 2015-01-28 22:33 ` Boris Ostrovsky 2015-01-29 11:46 ` Jan Beulich 1 sibling, 1 reply; 14+ messages in thread From: Andrew Cooper @ 2015-01-28 21:49 UTC (permalink / raw) To: Boris Ostrovsky, JBeulich; +Cc: xen-devel, kevin.tian, dietmar.hahn 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] x86/VPMU: Disable VPMU when NMI watchdog is on 2015-01-28 21:49 ` Andrew Cooper @ 2015-01-28 22:33 ` Boris Ostrovsky 2015-01-28 22:41 ` Andrew Cooper 0 siblings, 1 reply; 14+ messages in thread From: Boris Ostrovsky @ 2015-01-28 22:33 UTC (permalink / raw) To: Andrew Cooper, JBeulich; +Cc: xen-devel, kevin.tian, dietmar.hahn 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] x86/VPMU: Disable VPMU when NMI watchdog is on 2015-01-28 22:33 ` Boris Ostrovsky @ 2015-01-28 22:41 ` Andrew Cooper 2015-01-28 23:36 ` Boris Ostrovsky 0 siblings, 1 reply; 14+ messages in thread From: Andrew Cooper @ 2015-01-28 22:41 UTC (permalink / raw) To: Boris Ostrovsky, JBeulich; +Cc: xen-devel, kevin.tian, dietmar.hahn 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] x86/VPMU: Disable VPMU when NMI watchdog is on 2015-01-28 22:41 ` Andrew Cooper @ 2015-01-28 23:36 ` Boris Ostrovsky 2015-01-29 11:54 ` Andrew Cooper 0 siblings, 1 reply; 14+ messages in thread From: Boris Ostrovsky @ 2015-01-28 23:36 UTC (permalink / raw) To: Andrew Cooper, JBeulich; +Cc: xen-devel, kevin.tian, dietmar.hahn 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. >> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] x86/VPMU: Disable VPMU when NMI watchdog is on 2015-01-28 23:36 ` Boris Ostrovsky @ 2015-01-29 11:54 ` Andrew Cooper 0 siblings, 0 replies; 14+ messages in thread From: Andrew Cooper @ 2015-01-29 11:54 UTC (permalink / raw) To: Boris Ostrovsky, JBeulich; +Cc: xen-devel, kevin.tian, dietmar.hahn 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] x86/VPMU: Disable VPMU when NMI watchdog is on 2015-01-28 19:56 ` [PATCH 1/2] x86/VPMU: Disable VPMU when NMI watchdog is on Boris Ostrovsky 2015-01-28 21:49 ` Andrew Cooper @ 2015-01-29 11:46 ` Jan Beulich 2015-01-29 15:25 ` Boris Ostrovsky 1 sibling, 1 reply; 14+ messages in thread From: Jan Beulich @ 2015-01-29 11:46 UTC (permalink / raw) To: Boris Ostrovsky; +Cc: andrew.cooper3, kevin.tian, dietmar.hahn, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] x86/VPMU: Disable VPMU when NMI watchdog is on 2015-01-29 11:46 ` Jan Beulich @ 2015-01-29 15:25 ` Boris Ostrovsky 0 siblings, 0 replies; 14+ messages in thread From: Boris Ostrovsky @ 2015-01-29 15:25 UTC (permalink / raw) To: Jan Beulich; +Cc: andrew.cooper3, kevin.tian, dietmar.hahn, xen-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] x86/VPMU: Handle APIC_LVTPC accesses 2015-01-28 19:56 [PATCH 0/2] Two VPMU/watchdog-related patches Boris Ostrovsky 2015-01-28 19:56 ` [PATCH 1/2] x86/VPMU: Disable VPMU when NMI watchdog is on Boris Ostrovsky @ 2015-01-28 19:56 ` Boris Ostrovsky 2015-01-29 11:54 ` Jan Beulich 2015-01-29 11:37 ` [PATCH 0/2] Two VPMU/watchdog-related patches Jan Beulich 2 siblings, 1 reply; 14+ messages in thread From: Boris Ostrovsky @ 2015-01-28 19:56 UTC (permalink / raw) To: JBeulich, andrew.cooper3 Cc: xen-devel, kevin.tian, boris.ostrovsky, dietmar.hahn 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] x86/VPMU: Handle APIC_LVTPC accesses 2015-01-28 19:56 ` [PATCH 2/2] x86/VPMU: Handle APIC_LVTPC accesses Boris Ostrovsky @ 2015-01-29 11:54 ` Jan Beulich 2015-01-29 15:28 ` Boris Ostrovsky 0 siblings, 1 reply; 14+ messages in thread From: Jan Beulich @ 2015-01-29 11:54 UTC (permalink / raw) To: Boris Ostrovsky; +Cc: andrew.cooper3, kevin.tian, dietmar.hahn, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] x86/VPMU: Handle APIC_LVTPC accesses 2015-01-29 11:54 ` Jan Beulich @ 2015-01-29 15:28 ` Boris Ostrovsky 2015-01-29 15:44 ` Jan Beulich 0 siblings, 1 reply; 14+ messages in thread From: Boris Ostrovsky @ 2015-01-29 15:28 UTC (permalink / raw) To: Jan Beulich; +Cc: andrew.cooper3, kevin.tian, dietmar.hahn, xen-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] x86/VPMU: Handle APIC_LVTPC accesses 2015-01-29 15:28 ` Boris Ostrovsky @ 2015-01-29 15:44 ` Jan Beulich 0 siblings, 0 replies; 14+ messages in thread From: Jan Beulich @ 2015-01-29 15:44 UTC (permalink / raw) To: Boris Ostrovsky; +Cc: andrew.cooper3, kevin.tian, dietmar.hahn, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] Two VPMU/watchdog-related patches 2015-01-28 19:56 [PATCH 0/2] Two VPMU/watchdog-related patches Boris Ostrovsky 2015-01-28 19:56 ` [PATCH 1/2] x86/VPMU: Disable VPMU when NMI watchdog is on Boris Ostrovsky 2015-01-28 19:56 ` [PATCH 2/2] x86/VPMU: Handle APIC_LVTPC accesses Boris Ostrovsky @ 2015-01-29 11:37 ` Jan Beulich 2 siblings, 0 replies; 14+ messages in thread From: Jan Beulich @ 2015-01-29 11:37 UTC (permalink / raw) To: Boris Ostrovsky; +Cc: andrew.cooper3, kevin.tian, dietmar.hahn, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-01-29 15:44 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-01-28 19:56 [PATCH 0/2] Two VPMU/watchdog-related patches Boris Ostrovsky 2015-01-28 19:56 ` [PATCH 1/2] x86/VPMU: Disable VPMU when NMI watchdog is on Boris Ostrovsky 2015-01-28 21:49 ` Andrew Cooper 2015-01-28 22:33 ` Boris Ostrovsky 2015-01-28 22:41 ` Andrew Cooper 2015-01-28 23:36 ` Boris Ostrovsky 2015-01-29 11:54 ` Andrew Cooper 2015-01-29 11:46 ` Jan Beulich 2015-01-29 15:25 ` Boris Ostrovsky 2015-01-28 19:56 ` [PATCH 2/2] x86/VPMU: Handle APIC_LVTPC accesses Boris Ostrovsky 2015-01-29 11:54 ` Jan Beulich 2015-01-29 15:28 ` Boris Ostrovsky 2015-01-29 15:44 ` Jan Beulich 2015-01-29 11:37 ` [PATCH 0/2] Two VPMU/watchdog-related patches Jan Beulich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.