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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> 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().

Please refer to the correct functions - there's no pt_irq_posted(), and
vmx_intr_assist() doesn't itself update the field.

> --- a/xen/arch/x86/hvm/vpt.c
> +++ 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;

Both should be const. Also you don't really need v anywhere below,
all uses are v->domain.

> -    unsigned int gsi, isa_irq, pin;
> +    unsigned int gsi, pin;
> +
> +    ASSERT(pt->source == PTSRC_isa);
> +    ASSERT(src == hvm_intsrc_lapic);
> +    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;
> +}
> +
> +static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
> +{
> +    struct vcpu *v = pt->vcpu;

There is not need for this local variable - it's used exactly once (and
then again only to get to the domain).

> +    unsigned int isa_irq;
>  
>      if ( pt->source == PTSRC_lapic )
>          return pt->irq;
>  
>      isa_irq = pt->irq;
> -    gsi = hvm_isa_irq_to_gsi(isa_irq);
>  
>      if ( src == hvm_intsrc_pic )
>          return (v->domain->arch.hvm_domain.vpic[isa_irq >> 3].irq_base
>                  + (isa_irq & 7));
>  
>      ASSERT(src == hvm_intsrc_lapic);
> -    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 -1;
> -    }
> -
> -    return vioapic->redirtbl[pin].fields.vector;
> +    return pt->latched_vector;
>  }



> @@ -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).

> @@ -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.)

> +        {
> +            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?

> @@ -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.

> --- 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".

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.

Jan

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