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

Re: [Xen-devel] [PATCH v21 11/14] x86/VPMU: Handle PMU interrupts for PV(H) guests

On 05/18/2015 05:43 AM, Dietmar Hahn wrote:
Am Freitag 08 Mai 2015, 17:06:11 schrieb Boris Ostrovsky:

+    if ( !is_hvm_vcpu(sampling) )
+    {
+        /* PV(H) guest */
+        const struct cpu_user_regs *cur_regs;
+        uint64_t *flags = &vpmu->xenpmu_data->pmu.pmu_flags;
+        domid_t domid = DOMID_SELF;
+        if ( !vpmu->xenpmu_data )
+            return;
+        if ( is_pvh_vcpu(sampling) &&
+             !vpmu->arch_vpmu_ops->do_interrupt(regs) )

Here you expect vpmu->arch_vpmu_ops != NULL but ...

+            return;
+        if ( *flags & PMU_CACHED )
+            return;


+        return;
+    }

      if ( vpmu->arch_vpmu_ops )

... here is a check.
Maybe this check here is unnecessary because you would never get this interrupt
without having arch_vpmu_ops != NULL to switch on the machinery?

There are some other locations too with checks before calling
vpmu->arch_vpmu_ops->... and some without. Maybe it would make sense to force
always a complete set of arch_vpmu_ops - functions to avoid this?

I was actually thinking about (eventually) dropping ops tests and checking that all of them exist during VPMU initialization.

As for this particular test, it may be worth moving it to the beginning of the routine, mostly to guard against spurious interrupts (but also to avoid performing it more than once)


-void vpmu_load(struct vcpu *v)
+int vpmu_load(struct vcpu *v, bool_t verify)

vpmu_load uses "verify" but within the arch_vpmu_load functions
(core2_vpmu_load() and amd_vpmu_load()) you use "from_guest" for the same
meaning. This is a little bit confusing.
Always using "verify" would be clearer I think.

Then this will not be consistent with the save part (which doesn't use the flag to verify the context but rather to only state that the routine should copy it). So I think renaming 'verify' to 'from_guest' and keeping arch ops as they are now would be more consistent.


Xen-devel mailing list



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