Re: [Xen-devel] [PATCH for-4.13] x86/vmx: always sync PIR to IRR before vmentry

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Nov 18, 2019 at 01:01:58PM +0100, Jan Beulich wrote:
> On 18.11.2019 11:16, Roger Pau Monne wrote:
> > When using posted interrupts on Intel hardware it's possible that the
> > vCPU resumes execution with a stale local APIC IRR register because
> > depending on the interrupts to be injected vlapic_has_pending_irq
> > might not be called, and thus PIR won't be synced into IRR.
> > 
> > Fix this by making sure PIR is always synced to IRR in vmx_intr_assist
> > regardless of what interrupts are pending.
> 
> For this part, did you consider pulling ahead to the beginning
> of hvm_vcpu_has_pending_irq() its call to vlapic_has_pending_irq()?

I assumed the order in hvm_vcpu_has_pending_irq is there for a reason.
I could indeed move vlapic_has_pending_irq to the top, but then either
the result is discarded if for example a NMI is pending injection
(in which case there's no need to go through all the logic in
vlapic_has_pending_irq), or we invert the priority of event
injection.

I have to admit I have doubts about the code in
hvm_vcpu_has_pending_irq. I'm not sure what's the motivation for
example to give priority to HVMIRQ_callback_vector over other vectors
from the lapic.

> I ask because ...
> 
> > --- a/xen/arch/x86/hvm/vmx/intr.c
> > +++ b/xen/arch/x86/hvm/vmx/intr.c
> > @@ -232,6 +232,14 @@ void vmx_intr_assist(void)
> >      enum hvm_intblk intblk;
> >      int pt_vector;
> >  
> > +    if ( cpu_has_vmx_posted_intr_processing )
> > +        /*
> > +         * Always force PIR to be synced to IRR before vmentry, this is 
> > also
> > +         * done by vlapic_has_pending_irq but it's possible other pending
> > +         * interrupts prevent the execution of that function.
> > +         */
> > +        vmx_sync_pir_to_irr(v);
> 
> ... this addition looks more like papering over some issue than
> actually taking care of it.

Xen needs to make sure PIR is synced to IRR before entering
non-root mode. I could place the call somewhere else, or alternatively
Xen could disable interrupts, send a self-ipi with the posted vector
and enter non-root mode. That should IMO force a resync of PIR to IRR
when resuming vCPU execution, but is overly complicated.

> Then again I wonder whether the PIR->IRR sync is actually
> legitimate to perform when v != current.

IMO this is fine as long as the vCPU is not running, as in that case
the hardware is not in control of IRR.

> If it's not, then there
> might be a wider set of problems (see e.g.
> hvm_local_events_need_delivery()). But of course the adjustment
> to hvm_vcpu_has_pending_irq() could also be to make the call
> early only when v == current.

I don't think we should be that restrictive, v == current ||
!vcpu_runable(v) ought to be safe. I've also forgot to send my
pre-patch to introduce an assert to that effect:

https://lists.xenproject.org/archives/html/xen-devel/2019-11/msg00635.html

> A last question is that on the consequences of overly aggressive
> sync-ing - that'll harm performance, but shouldn't affect
> correctness if I'm not mistaken.

That's correct, as long as the vcpu is the current one or it's not
running.

Roger.

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