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

Re: [Xen-devel] [PATCH v6 17/19] x86/VPMU: NMI-based VPMU support

On 05/26/2014 11:55 AM, Jan Beulich wrote:

+static void vpmu_send_nmi(struct vcpu *v)
+    struct vlapic *vlapic;
+    u32 vlapic_lvtpc;
+    unsigned char int_vec;
+    ASSERT( is_hvm_vcpu(v) );
+    vlapic = vcpu_vlapic(v);
+    if ( !is_vlapic_lvtpc_enabled(vlapic) )
+        return;
+    vlapic_lvtpc = vlapic_get_reg(vlapic, APIC_LVTPC);
+    int_vec = vlapic_lvtpc & APIC_VECTOR_MASK;
+    if ( GET_APIC_DELIVERY_MODE(vlapic_lvtpc) == APIC_MODE_FIXED )
+        vlapic_set_irq(vcpu_vlapic(v), int_vec, 0);
+    else
+        v->nmi_pending = 1;
I realize this is only the result of code movement, but what is this
if/else pair doing?

To be honest, I don't know. This code has been in VPMU since day one and I just moved it here.

I am not sure whether 'if' clause has ever been tested since perf (and before that, oprofile) use NMIs for PMU interrupts. But I should probably at least rename the routine to something like send_pmu_interrupt (and move int_vec calculation into the 'if' clause).

+    if ( (vpmu_mode & XENPMU_MODE_PRIV) ||
+         (sampled->domain->domain_id >= DOMID_FIRST_RESERVED) )
+        v = hardware_domain->vcpu[smp_processor_id() %
+                                  hardware_domain->max_vcpus];
+    else
+    {
+        if ( is_hvm_domain(sampled->domain) )
+        {
+            vpmu_send_nmi(sampled);
+            return;
+        }
+        v = sampled;
+    }
+    regs = &v->arch.vpmu.xenpmu_data->pmu.r.regs;
+    if ( !is_pv_domain(sampled->domain) )
Even if it means the same, has_hvm_container_vcpu(sampled)
please: You're guarding an operation here that requires a HVM

I am removing PVH-enabling patch (which currently comes after this one) so all is_*_domain() will hopefully make sense at the time they are added.

+    {
+        struct segment_register cs;
+        hvm_get_segment_register(sampled, x86_seg_cs, &cs);
I hope you understand that what you read here is only implicitly (due
to the softirq getting serviced before the guest gets re-entered) the
current value. Or wait - is it at all? What if a scheduler softirq first
causes the guest to be de-scheduled and another CPU manages to
pick it up before you get here?

Based on a similar comment from Kevin I added a check for pending PMU interrupts (i.e. a call to this routine) in vpmu_save() which is called during context switch. We should therefore always pick the current value of guest's CS.

@@ -472,6 +574,21 @@ static int pvpmu_init(struct domain *d, xen_pmu_params_t 
          return -EINVAL;
+ if ( !pvpmu_initted )
+    {
+        if (reserve_lapic_nmi() == 0)
Coding style.

+            set_nmi_callback(pmu_nmi_interrupt);
+        else
+        {
+            printk("Failed to reserve PMU NMI\n");
+            put_page(page);
+            return -EBUSY;
+        }
+        open_softirq(PMU_SOFTIRQ, pmu_softnmi);
+        pvpmu_initted = 1;
Is it excluded that you get two racing pvpmu_init() calls (i.e. are
these exclusively coming from e.g. a domctl)? If not, better
serialization would be needed here.

In Linux the first call (it's not a domctl but a dedicated hypercall) is done by boot CPU pre-SMP so I think we should be safe.

But then there may be non-Linux users so a lock wouldn't hurt.


Xen-devel mailing list



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