[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v18 05/16] x86/VPMU: Interface for setting PMU mode and flags

On 02/20/2015 08:59 AM, Jan Beulich wrote:
On 16.02.15 at 23:26, <boris.ostrovsky@xxxxxxxxxx> wrote:
--- a/xen/arch/x86/hvm/svm/vpmu.c
+++ b/xen/arch/x86/hvm/svm/vpmu.c
@@ -253,6 +253,26 @@ static int amd_vpmu_save(struct vpmu_struct *vpmu)
      return 1;
+static void amd_vpmu_unload(struct vpmu_struct *vpmu)
+    struct vcpu *v;
+    if ( vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED | VPMU_FROZEN) )
+    {
+        unsigned int i;
+        for ( i = 0; i < num_counters; i++ )
+            wrmsrl(ctrls[i], 0);
+        context_save(vpmu);
This assumes vpmu_vcpu(vpmu) == current, yet it looks like you're
also calling it under different circumstances now.

This doesn't make this assumption. PMU MSRs may be loaded with values that belong to a VCPU that is not currently running if, for example, current VCPU is not using PMU. And flags test guarantees that we won't stomp on someone else's registers.

I could make a test here and write zeroes to the MSRs only if vpmu is current but I don't like leaving these MSRs with what is essentially garbage values. This routine is executed very rarely so overhead is negligible.

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1183,11 +1183,10 @@ int vmx_read_guest_msr(u32 msr, u64 *val)
      return -ESRCH;
-int vmx_write_guest_msr(u32 msr, u64 val)
+int vmx_write_guest_msr_vcpu(struct vcpu *v, u32 msr, u64 val)
-    struct vcpu *curr = current;
-    unsigned int i, msr_count = curr->arch.hvm_vmx.msr_count;
-    struct vmx_msr_entry *msr_area = curr->arch.hvm_vmx.msr_area;
+    unsigned int i, msr_count = v->arch.hvm_vmx.msr_count;
+    struct vmx_msr_entry *msr_area = v->arch.hvm_vmx.msr_area;
for ( i = 0; i < msr_count; i++ )
Same here - while the code itself is only accessing memory, it
remains unclear whether correct behavior results when the subject
vCPU is actually executing.

Not sure what you mean here. The changes are specifically for updating MSR values (cached in VMCS) of possibly non-current VCPU.

In both cases, if there are restrictions on the conditions under
which unload can validly be done, I think you should ASSERT()
those conditions (at once making them explicit).

+long do_xenpmu_op(unsigned int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) 
+    int ret;
+    struct xen_pmu_params pmu_params;
+    if ( vpmu_disabled )
+        return -EINVAL;

And I agree with Dietmar that keeping opt_vpmu_enabled instead
of introducing vpmu_disabled would seem more natural, the more
that the default is disabled.

I can switch polarity back to enabled since both of you think it makes more sense but I don't think I should keep "opt_" prefix since this flag can be modified independently of what boot option says (e.g. when watchdog is on).

+/* Context switch */
+static inline void vpmu_switch_from(struct vpmu_struct *prev,
+                                    struct vpmu_struct *next)
+    if ( vpmu_mode & (XENPMU_MODE_SELF | XENPMU_MODE_HV) )
+        vpmu_save(prev);
+static inline void vpmu_switch_to(struct vpmu_struct *prev,
+                                  struct vpmu_struct *next)
+    if ( vpmu_mode & (XENPMU_MODE_SELF | XENPMU_MODE_HV) )
+        vpmu_load(next);
+    else if ( vpmu_is_set(next, VPMU_CONTEXT_LOADED | VPMU_RUNNING) )
+        vpmu_unload(next);
I don't recall there having been an unload path here, and I don't
see this being explained anywhere either. Wouldn't that more
naturally be done in the switch-from function?

I had this for a couple of revisions now. And I had a very specific reason for (1) having it and (2) having it here (it had something to do with the fact that 'next' may still be LOADED after we've changed vpmu_mode to, say, OFF) . But I can't remember what it was now. If I remember why I'll add a comment, otherwise I'll drop this.

Apart from that it's also not clear why both prev and next get
passed to both of these functions. Iirc a later patch may make
use of that, but then that later patch should add the second
function parameter.

Right, I was thinking of using both parameters at some point and left them here. Only one is needed.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.