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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.11.2019 15:20, Roger Pau Monné  wrote:
> On Mon, Nov 18, 2019 at 03:00:00PM +0100, Jan Beulich wrote:
>> On 18.11.2019 14:46, Roger Pau Monné  wrote:
>>> 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.
>>
>> Changing the order of events injected is not an option afaict. The
>> pointless processing done is a valid concern, yet the suggestion
>> was specifically to have (part of) this processing to occur early.
>> The discarding of the result, in turn, is not a problem afaict, as
>> a subsequent call will return the same result (unless a higher
>> priority interrupt has surfaced in the meantime).
> 
> Yes, that's fine. So you would prefer to move the call to
> vlapic_has_pending_irq before any exit path in
> hvm_vcpu_has_pending_irq?

"Prefer" isn't really the way I would put it. I'd like this to be
considered as an alternative because, as said, I think the current
placement look more like a plaster than a cure. I'm also open for
other suggestions. But first of all I'd like to see what the VMX
maintainers think.

>>>> 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.
>>
>> Here and ...
>>
>>>> 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.
>>
>> ... here I continue to be worried of races: Any check for a vCPU to
>> be non-running (or non-runnable) is stale the moment you inspect the
>> result of the check. Unless, of course, you suppress scheduling
>> (actions potentially making a vCPU runnable) during that time window.
> 
> Hm, it's indeed true that syncing PIR into IRR for a vCPU not running
> in the current pCPU is troublesome. Maybe syncing PIR into IRR should
> only be done when v == current?
> 
> The only alternative I can think of is something like:
> 
> if ( v != current )
>     vcpu_pause(v);
> sync_pir_irr(v);
> if ( v != current )
>     vcpu_unpause(v);
> 
> Is there a need to check the IRR of vCPUs that are not running, and
> does it matter if the IRR is not fully updated in that case?

That's one of the problems with function parameters decribed
"struct vcpu *v" - you don't know whether there's any expectation
that v == current at all times. And tracking down all uses of
certain functions can be rather tedious. IOW without quite a bit
of code auditing I'm afraid I can't tell whether there's possibly
some corner case where this might be needed.

Jan

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