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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

Jan


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