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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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