Re: [Xen-devel] guest being crashed from viridian_start_apic_assist()

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 03 June 2016 14:06
> To: Paul Durrant
> Cc: xen-devel
> Subject: RE: guest being crashed from viridian_start_apic_assist()
> 
> >>> On 03.06.16 at 14:11, <Paul.Durrant@xxxxxxxxxx> wrote:
> >>  -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: 03 June 2016 10:59
> >> To: Paul Durrant
> >> Cc: xen-devel
> >> Subject: RE: guest being crashed from viridian_start_apic_assist()
> >>
> >> >>> On 03.06.16 at 11:39, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> >> Sent: 03 June 2016 10:04
> >> >>
> >> >> to test a guest ACPI change I went through all Windows versions that
> >> >> I have installed anywhere, to find 32-bit Win7 dying (the other
> >> >> variants I tried worked fine), albeit retrying a couple of times I then
> >> >> didn't see this a second time. Is this something you've observed
> >> >> elsewhere? The APIC assist logic isn't that complicated, and I can't
> >> >> really spot any race in there...
> >> >
> >> > How did it die?
> >>
> >> From the single domain_crash() in that function. No other information
> >> was available (apart from the register dump, which doesn't look like
> >> it would be very useful for analysis here).
> >>
> >
> > There is an inconsistency between the test to abort a previous assist, which
> > is based on:
> >
> >     irr = vlapic_find_highest_irr(vlapic);
> >
> >     isr = vlapic_find_highest_isr(vlapic);
> >     if ( (isr & 0xf0) >= (irr & 0xf0) )
> >
> > and the test to start a new one:
> >
> >     isr = vlapic_find_lowest_vector(&vlapic->regs->data[APIC_ISR]);
> >     if ( isr >= 0 && isr < vector )
> >
> > I suspect that may be the problem you hit.
> 
> I don't think so. Of the second if(), the right side could actually be
> dropped, as the logic in vlapic_has_pending_irq() guarantees
> (vector & 0xf0) > (highest_isr & 0xf0), i.e. vector > highest_isr
> and hence necessarily vector > lowest_isr. That is, if
> vlapic_has_pending_irq() finds _some_ ISR bit set, it must be one
> below vector. Which means viridian_start_apic_assist() gets called
> solely when there's _no_ other ISR bit set.
> 

Yes, that test does appear to be unnecessary.

> What I find more problematic looking at those functions (but
> unrelated to the problem here afaict) is the
> vlapic_virtual_intr_delivery_enabled() related logic and its
> interaction with the Viridian APIC assist (leaving aside nested
> mode for the moment): vlapic_has_pending_irq() won't do
> anything with the APIC assist in that case, but if force_ack is
> true in vlapic_ack_pending_irq() there will be an interaction.
> 

...and the interaction would indeed be the crash you saw, I think, because you 
could start an APIC assist when vlapic_virtual_intr_delivery_enabled() is true, 
but never complete it because vlapic_has_pending_irq() bails before doing so. 
Thus, attempting the next APIC assist would lead to a domain_crash().

  Paul

> Jan


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