Re: [Xen-devel] [PATCH] x86/hvm: more sure APIC assist is aborted if guest EOIs APIC

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 18 January 2018 09:39
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH] x86/hvm: more sure APIC assist is aborted if guest EOIs
> APIC
> 
> >>> On 18.01.18 at 10:06, <Paul.Durrant@xxxxxxxxxx> wrote:
> >>  -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: 18 January 2018 08:33
> >> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> >> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; xen-
> >> devel@xxxxxxxxxxxxxxxxxxxx
> >> Subject: Re: [PATCH] x86/hvm: more sure APIC assist is aborted if guest
> EOIs
> >> APIC
> >>
> >> >>> On 17.01.18 at 13:53, <paul.durrant@xxxxxxxxxx> wrote:
> >> > --- a/xen/arch/x86/hvm/vlapic.c
> >> > +++ b/xen/arch/x86/hvm/vlapic.c
> >> > @@ -422,6 +422,13 @@ void vlapic_EOI_set(struct vlapic *vlapic)
> >> >      if ( vector == -1 )
> >> >          return;
> >> >
> >> > +    /*
> >> > +     * It is possible that APIC assist has been enabled by the guest but
> >> > +     * it has chosen not to use it, by EOIing normally. It is therefore
> >> > +     * necessary to abort any APIC assist that may have been started
> >> > +     * to avoid confusing the state machine.
> >> > +     */
> >> > +    viridian_abort_apic_assist(vlapic_vcpu(vlapic));
> >> >      vlapic_clear_vector(vector, &vlapic->regs->data[APIC_ISR]);
> >>
> >> Fundamentally fine, but is clearing bit 0 in the shared location a
> >> valid thing to do in this case? Plus shouldn't you pass in the vector,
> >> so the abort would only happen for the very vector that is pending?
> >
> > I believe that it is ok.
> >
> >> Iirc with nested interrupts normal EOI might still need to be used
> >> by Windows (as only a single vector can be in flight in the assist), so
> >> clearing the assist for the wrong vector might actually cause harm.
> >>
> >
> > If there are nested interrupts then the assist should be aborted anyway.
> > Assist only helps if there is one interrupt pending so should not have been
> > started if something higher priority already in the ISR
> 
> I was thinking about the opposite case - assist active with a higher
> priority interrupt coming in (and then being EOI-ed). But I guess
> that's already being taken care of by the existing call to
> viridian_abort_apic_assist().
> 

Yes, that's right.

> > so, if we see an EOI
> > for a vector that is not the one latched in assist I think this is also a 
> > bug
> > in the state machine. I can add verification of that, if you'd like.
> 
> At least one-time warnings would seem helpful here, so we have
> some sort of indication that some assumption is being violated.
> 

Ok. I'll re-work it so that abort is passed a vector and warns on mismatch.

  Paul

> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel