[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH for-4.9 v2] hvm/vpt: fix inconsistent views of vIOAPIC in vmx_intr_assist()
>>> 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |