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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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
> container.
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 
> > *params)
> >           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.

-boris


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel