Re: [Xen-devel] [PATCH for-4.9 v2] hvm/vpt: fix inconsistent views of vIOAPIC in vmx_intr_assist()

[Chao Gao <chao.gao@xxxxxxxxx>]

On Fri, Apr 28, 2017 at 01:59:47PM +0800, Tian, Kevin wrote:
>> From: Gao, Chao
>> Sent: Thursday, April 27, 2017 10:47 AM
>> 
>> When injecting periodic timer interrupt in vmx_intr_assist(),
>> multi-read operations are done during one event delivery. For
>> example, if a periodic timer interrupt is from PIT, when set the
>> corresponding bit in vIRR, the corresponding RTE is accessed in
>> pt_update_irq(). When this function returns, it accesses the RTE
>> again to get the vector it sets in vIRR.  Between the two
>> accesses, the content of RTE may have been changed by another CPU
>> for no protection method in use. This case can incur the
>> assertion failure in vmx_intr_assist().  Also after a periodic
>> timer interrupt is injected successfully, pt_irq_posted() will
>> decrease the count (pending_intr_nr). But if the corresponding
>> RTE has been changed, pt_irq_posted() will not decrease the
>> count, resulting one more timer interrupt.
>> 
>> More discussion and history can be found in
>> 1.https://lists.xenproject.org/archives/html/xen-devel/2017-
>> 03/msg00906.html
>> 2.https://lists.xenproject.org/archives/html/xen-devel/2017-
>> 01/msg01019.html
>> 
>> This patch introduces a new field 'latched_vector' to structure
>> periodic_timer. The field is only valid between calling pt_update_irq()
>> and calling pt_irq_posted() in vmx_intr_assist(). The new field is set
>> to the vector we set in vIRR at the first access we describe above, is
>> used in the following two accesses through calling pt_irq_vector() and
>> finally cleared in pt_irq_posted() or updated in next calling
>> vmx_intr_assist(). The latching operation should be protected by
>> irq_lock to prevent other vCPUs changing the value we are interest in.
>> Thus, provide a -locked variant of hvm_isa_irq_assert() to avoid
>> deadlock.
>> 
>> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
>> +++ b/xen/arch/x86/hvm/vpt.c
>> @@ -75,33 +75,43 @@ void hvm_set_guest_time(struct vcpu *v, u64
>> guest_time)
>>      }
>>  }
>> 
>> -static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
>> +static void pt_set_latched_vector(struct periodic_time *pt, enum
>> hvm_intsrc src)
>>  {
>>      struct vcpu *v = pt->vcpu;
>>      struct hvm_vioapic *vioapic;
>> -    unsigned int gsi, isa_irq, pin;
>> +    unsigned int gsi, pin;
>> +
>> +    ASSERT(pt->source == PTSRC_isa);
>> +    ASSERT(src == hvm_intsrc_lapic);
>
>Do we really need add such limitation here? Is it simpler to rename
>original pt_irq_vector as pt_set_latched_vector by adding one
>line to record returned value for all conditions and then define
>pt_irq_vector simply as returning pt_latched_vector?

Almost agree. We don't latch vector for case 'handle_by_pic == true'.
Need be more careful for this case. For that case, the vector is
decided in hvm_vcpu_ack_pending_irq(). But at that place, we don't
know which periodic timer's latched_vector filed should be set.
Is traversal a clean way? or let pt_irq_vector() take 'hvm_intsrc_pic'
as a special case.

>
>> +    gsi = hvm_isa_irq_to_gsi(pt->irq);
>> +    vioapic = gsi_vioapic(v->domain, gsi, &pin);
>> +    if ( !vioapic )
>> +    {
>> +        dprintk(XENLOG_WARNING, "d%u: invalid GSI (%u) for platform
>> timer\n",
>> +                v->domain->domain_id, gsi);
>> +        domain_crash(v->domain);
>> +        return;
>> +    }
>> +
>> +    pt->latched_vector = vioapic->redirtbl[pin].fields.vector;
>> +}
>> +
>>      else
>>      {
>>          hvm_isa_irq_deassert(v->domain, irq);
>> -        hvm_isa_irq_assert(v->domain, irq);
>> +        spin_lock(&v->domain->arch.hvm_domain.irq_lock);
>> +        hvm_isa_irq_assert_locked(v->domain, irq);
>> +        if ( platform_legacy_irq(irq) && vlapic_accept_pic_intr(v) &&
>> +            (&v->domain->arch.hvm_domain)->vpic[irq >> 3].int_output )
>> +        {
>> +            handle_by_pic = true;
>> +            pt_set_latched_vector(earliest_pt, hvm_intsrc_lapic);
>> +        }
>
>I think you changed the original semantics here. Originally -1
>is returned when above condition is true otherwise pt_irq_vector
>is returned. The otherwise condition is what you would like to
>fix by latching vector. However your patch actually reverts
>the policy.

Indeed. will fix this. Thanks to figure this out.

Thanks
Chao

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