Re: [Xen-devel] [PATCH v3] x86/apicv: Enhance posted-interrupt processing

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.03.17 at 05:28, <chao.gao@xxxxxxxxx> wrote:
> On Thu, Mar 02, 2017 at 02:41:55AM -0700, Jan Beulich wrote:
>>>>> On 02.03.17 at 02:49, <chao.gao@xxxxxxxxx> wrote:
>>
>>The patch title, btw, makes it looks like this isn't a bug fix, which is
>>contrary to the understanding I've gained so far.
> 
> Thanks to your patience and your changing so much description for me. Also 
> to the questions you asked. I agree to the comments i don't reply to.
> How about this:
> Fix a wrongly IPI suppression during posted interrupt delivery

fix wrong IPI suppression during posted interrupt delivery

>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -1842,13 +1842,59 @@ static void __vmx_deliver_posted_interrupt(struct 
>>> vcpu *v)
>>>      bool_t running = v->is_running;
>>>  
>>>      vcpu_unblock(v);
>>> +    /*
>>> +     * The underlying 'IF' excludes two cases which we don't need further
>>
>>Who or what is 'IF'?
>>
> 
> I meaned the 'if sentence'.

I'm sorry, but this doesn't make it any more clear. DYM some if()
statement? But then why "underlying"? Was that meant to be
"following"?

>>> +     * operation to make sure the target vCPU (@v) will sync PIR to vIRR 
>>> ASAP.
>>> +     * Specifically, the two cases are:
>>> +     * 1. The target vCPU is not running, meaning it is blocked or 
>>> runnable.
>>> +     * Since we have unblocked the target vCPU above, the target vCPU will
>>> +     * sync PIR to vIRR when it is chosen to run.
>>> +     * 2. The target vCPU is the current vCPU and in_irq() is false. It 
>>> means
>>> +     * the function is called in noninterrupt context.
>>
>>     * 2. The target vCPU is the current vCPU and we're in
>>     * non-interrupt context.
>>
>>> Since we don't call
>>> +     * the function in noninterrupt context after the last time a vCPU 
>>> syncs
>>> +     * PIR to vIRR, excluding this case is also safe.
>>
>>It is not really clear to me what "the function" here refers to. Surely
>>the function the comment is in won't call itself, no matter what
>>context.
> 
> Yes, it is not clear. How about:
> The target vCPU is the current vCPU and we're in non-interrupt context.
> we can't be in the window between the call to vmx_intr_assist()

"we can't be" looks misleading. Perhaps "We're not being called from
the window ..."?

> and interrupts getting disabled since no existed code reachs here in
> non-interrupt context. Considering that, it is safe to just leave without
> further operation. 
> 
> Do you think this is correct? I have an assumption here to explain why
> (in_irq() || (v != current)) is reasonable. It is totally my guess.

As suggested earlier, feel free to simply refer to vcpu_kick() doing
the same.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel