Re: [Xen-devel] [PATCH v12 for-xen-4.5 19/20] x86/VPMU: NMI-based VPMU support

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.10.14 at 02:18, <boris.ostrovsky@xxxxxxxxxx> wrote:

> On 09/30/2014 04:37 AM, Jan Beulich wrote:
>>   
>> +static void vpmu_send_interrupt(struct vcpu *v)
>> +{
>> +    struct vlapic *vlapic;
>> +    u32 vlapic_lvtpc;
>> +
>> +    ASSERT( is_hvm_vcpu(v) );
>> +
>> +    vlapic = vcpu_vlapic(v);
>> +    if ( !is_vlapic_lvtpc_enabled(vlapic) )
>> +        return;
>> +
>> +    vlapic_lvtpc = vlapic_get_reg(vlapic, APIC_LVTPC);
>> +    if ( GET_APIC_DELIVERY_MODE(vlapic_lvtpc) == APIC_MODE_FIXED )
>> +        vlapic_set_irq(vcpu_vlapic(v), vlapic_lvtpc & APIC_VECTOR_MASK, 0);
>> +    else
>> +        v->nmi_pending = 1;
>> Is APIC_MODE_NMI guaranteed to be the only alternative to
>> APIC_MODE_FIXED here (even for a buggy guest)? I don't recall
>> having seen code preventing other modes to be set, but even if
>> such code exists, an ASSERT() here seems quite desirable to me
>> (perhaps after re-structuring this to a switch() this could also be
>> a debug log message).
> 
> This was a simple code move from original VPMU implementation 
> (vpmu_do_interrupt()) to a separate routine, only to avoid duplication 
> of this code since it may now be called from different places.
> 
> This is HVM guest's view of LVTPC, not Xen's, and has nothing to do with 
> VPMU interrupt mode (i.e. vector vs NMI) if that's what you were 
> thinking about.

Indeed I subsequently realized that you just moved broken code.
But I'd really have expected you to notice the brokenness and
either fix it along with moving it, or include a separate prereq
patch to address this (which I have now done:
http://lists.xenproject.org/archives/html/xen-devel/2014-10/msg00014.html).

Jan


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