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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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