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 02:36:11AM -0600, Jan Beulich wrote:
>>>> On 27.04.17 at 04:47, <chao.gao@xxxxxxxxx> wrote:
>> 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().
>
>Btw., irrespective of this I'm not convinced this patch really is
>4.9 material: While analyzing possible causes for the ASSERT()
>to trigger, you've found a way no real OS would ever use. And
>we'll need to do something about that ASSERT() anyway before
>4.9 goes out.

Fine to me.

>> @@ -253,6 +263,7 @@ int pt_update_irq(struct vcpu *v)
>>      struct periodic_time *pt, *temp, *earliest_pt;
>>      uint64_t max_lag;
>>      int irq, is_lapic;
>> +    bool handle_by_pic = false;
>
>What do you need this variable for? You can simply clear is_lapic
>instead. And if you needed it, please follow the naming of the
>other one (i.e. would want to be is_pic).

The interrupt source and interrupt controller is different here.
for interrupt source we have RTSRC_isa and RTSRC_lapic.
I think is_lapic is related to interrupt source.
and 'handle_by_pic' means that vpic relay this interrupt, which
is relevant to struct hvm_intsrc. Anyhow, it is not a big problem.

>
>> @@ -297,7 +308,15 @@ int pt_update_irq(struct vcpu *v)
>>      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 )
>
>Please don't pointlessly complicate expressions: There's no need
>to take the address of v->domain->arch.hvm_domain here. (I
>notice that it has been this way originally, but as you're touching
>it, you should also clean this up.)

will do.

>
>> +        {
>> +            handle_by_pic = true;
>> +            pt_set_latched_vector(earliest_pt, hvm_intsrc_lapic);
>
>I don't follow: If the interrupt is to be handled by the PIC, why
>do you need to latch the vector in that case at all, and even
>more so _only_ in that case? When delivered via PIC, the IO-APIC
>RTE won't have a valid vector field anyway (it's supposed to be
>in ExtINT mode; see __vlapic_accept_pic_intr()). Am I overlooking
>anything here?

Ok. I made a mistake here. Only need latching vector for the inverse case.

>
>> @@ -305,12 +324,7 @@ int pt_update_irq(struct vcpu *v)
>>       * IRR is returned and used to set eoi_exit_bitmap for virtual
>>       * interrupt delivery case. Otherwise return -1 to do nothing.  
>>       */ 
>> -    if ( !is_lapic &&
>> -         platform_legacy_irq(irq) && vlapic_accept_pic_intr(v) &&
>> -         (&v->domain->arch.hvm_domain)->vpic[irq >> 3].int_output )
>> -        return -1;
>> -    else 
>> -        return pt_irq_vector(earliest_pt, hvm_intsrc_lapic);
>> +    return handle_by_pic ? -1 : pt_irq_vector(earliest_pt, 
>> hvm_intsrc_lapic);
>
>Please either use unlikely() or invert the condition and swap the
>other two operands.

Agree.

>
>> --- a/xen/include/asm-x86/hvm/vpt.h
>> +++ b/xen/include/asm-x86/hvm/vpt.h
>> @@ -46,6 +46,12 @@ struct periodic_time {
>>  #define PTSRC_lapic  2 /* LAPIC time source */
>>      u8 source;                  /* PTSRC_ */
>>      u8 irq;
>> +    /*
>> +     * Vector set in vIRR in one interrupt delivery. Using this value can
>> +     * avoid reading the IOAPIC RTE again. Valid only when its source is
>> +     * 'PTSRC_isa' and handled by vlapic.
>> +     */
>> +    int16_t latched_vector;
>
>Why is this PTSRC_isa specific? In the description you say
>"for example, if a periodic timer interrupt is from PIT".

PTSRC_lapic means the pt is lapic interrupt timer. We can read 'irq'
field directly from structure periodic_timer. For other cases, namely
PTSRC_isa, we should get vector from vioapic or vpic. For cases handle
by vpic, the vector is generated in hvm_vcpu_ack_pending_irq(). I can
latched here. So we have two restrictions. The first one is easy to
remove. The latter I think needs some changes to
hvm_vcpu_ack_pending_irq().

>
>And then, if there is a requirement for this to be handled by the
>LAPIC, you don't need a 16-bit field: You can easily use 0 to
>indicate the field is not valid, as vectors below 0x10 are all invalid
>in that case.

will do.

Thanks
Chao

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