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:55, Roger Pau Monné  wrote:
> On Mon, Nov 18, 2019 at 03:19:50PM +0100, Jan Beulich wrote:
>> On 18.11.2019 15:03, Roger Pau Monné  wrote:
>>> On Mon, Nov 18, 2019 at 01:26:46PM +0100, Jan Beulich wrote:
>>>> On 18.11.2019 11:16, Roger Pau Monne wrote:
>>>>> @@ -1954,48 +1952,28 @@ static void __vmx_deliver_posted_interrupt(struct 
>>>>> vcpu *v)
>>>>>       * 2. The target vCPU is the current vCPU and we're in non-interrupt
>>>>>       * context.
>>>>>       */
>>>>> -    if ( running && (in_irq() || (v != current)) )
>>>>> -    {
>>>>> +    if ( vcpu_runnable(v) && v != current )
>>>>
>>>> I'm afraid you need to be more careful with the running vs runnable
>>>> distinction here. The comment above here becomes stale with the
>>>> change (also wrt the removal of in_irq(), which I'm at least uneasy
>>>> about), and the new commentary below also largely says/assumes
>>>> "running", not "runnable".
>>>
>>> I've missed to fix that comment, will take care in the next version.
>>> Note also that the comment is quite pointless, it only states what the
>>> code below is supposed to do, but doesn't give any reasoning as to why
>>> in_irq is relevant here.
>>
>> It's main "value" is to refer to vcpu_kick(), which has ...
>>
>>> TBH I'm not sure of the point of the in_irq check, I don't think it's
>>> relevant for the code here.
>>
>> ... a similar in_irq() check. Sadly that one, while having a bigger
>> comment, also doesn't explain what it's needed for. It looks like I
>> should recall the reason, but I'm sorry - I don't right now.
> 
> By reading the message of the commit that introduced the in_irq check
> in vcpu_kick:
> 
> "The drawback is that {vmx,svm}_intr_assist() now races new event
> notifications delivered by IRQ or IPI. We close down this race by
> having vcpu_kick() send a dummy softirq -- this gets picked up in
> IRQ-sage context and will cause retry of *_intr_assist(). We avoid
> delivering the softirq where possible by avoiding it when we are
> running in the non-IRQ context of the VCPU to be kicked."
> 
> AFAICT in the vcpu_kick case this is done because the softirq should
> only be raised when in IRQ context in order to trigger the code in
> vmx_do_vmentry to retry the call to vmx_intr_assist (this is relevant
> if vcpu_kick is issued from an irq handler executed after
> vmx_intr_assist and before the disabling interrupts in
> vmx_do_vmentry.
> 
> I think we need something along the lines of:
> 
> if ( v->is_running && v != current )
>     send_IPI_mask(cpumask_of(v->processor), posted_intr_vector);
> else if ( v == current && in_irq() && !softirq_pending(smp_processor_id()) )
>     raise_softirq(VCPU_KICK_SOFTIRQ);
> 
> So that vmx_intr_assist is also retried if a vector is signaled in PIR
> on the vCPU currently running between the call to vmx_intr_assist and
> the disabling of interrupts in vmx_do_vmentry.

Looks plausible.

Jan

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