Re: [Xen-devel] [PATCH v2] x86/apicv: enhance posted-interrupt processing

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.03.17 at 07:23, <chao.gao@xxxxxxxxx> wrote:
> On Wed, Mar 01, 2017 at 12:38:39AM -0700, Jan Beulich wrote:
>>>>> On 01.03.17 at 04:23, <xuquan8@xxxxxxxxxx> wrote:
>>> Gone through the code, in_irq() means that the cpu is dispatching an 
>>> interrupt..
>>> I am really hesitated whether to drop ' (in_irq() || (v != current)) ' or 
>>> not in v2, but I found there is a same one in vcpu_kick()..
>>> So I leave it as is for further discussion. Now I tend to drop it.
>>
>>Well, it being that way in vcpu_kick() rather suggests that you
>>also want the same thing here - after all this looks to be an
>>open-coded slight variation of that function. _That's_ likely
>>what is missing to be said here in a comment (and you wouldn't
>>even need to repeat any of the commentary there [which sadly
>>looks to be somewhat stale], but simply refer to it). However,
>>this also points out that your local variable are likely wrong:
>>v->is_running wants evaluating before calling vcpu_pause(),
>>while v->processor wants to be evaluated only after the call.
>>
>>The main thing to explain then is why v->processor changing
>>after having evaluated it is not a problem. While relatively
>>obvious in vcpu_kick() - the field changing means the vCPU
>>is running, and getting it to run is all vcpu_kick() wants to
>>achieve -, the goal here - avoiding to miss delivering an
>>interrupt - is different, and so is rationalizing the correctness
>>of the code.
> 
> Good point. I ignore v->processor maybe change. I have thought over
>  __vmx_deliver_posted_interrupt() again and want to share you my
> opinion. 
> First of all, __vmx_deliver_posted_interrupt() is to let the target
> vCPU sync PIR to vIRR ASAP.
> different strategies we will used to deal with different cases.
> One is we just unblock the target vCPU when the vCPU is blocked. This can
> make sure the vCPU will go to  vmx_intr_assist() where we achieve the goal.
> The second one is the vCPU is runnable, we will achieve the goal 
> automatically
> when the vCPU is chosen to run.
> The third one is the vCPU is running and running on the same pCPU with the 
> source vCPU. It just wants to notify itself. Just raise a softirq is enough.
> The fourth one is the vCPU is running on other pCPU. To notify the target 
> vCPU,
> we send a IPI to the target PCPU which the vCPU is on. Note that when the 
> notification
> arrives to the target PCPU, the target vCPU maybe is blocked, runnable, 
> running in root mode,
> or running in non-root mode. If the target vCPU is running in non-root mode, 
> hardware
> will sync PIR to vIRR. If the target vCPU is in non-root mode, the Interrupt 
> handler
> starts to run. To make sure, we can go back to vmx_intr_assist(), I have 
> suggested that
> the interrupt handler should raise a softirq. If the target vCPU is 
> runnable, we will
> raise a softirq to a wrong vCPU. Maybe it isn't a big issue. If the target 
> vCPU is
> blocked, since before we block a vCPU we will check events through 
> local_events_need_delivery()
> , the goal must has been achieved. Also incur a IPI and softirq to a wrong 
> vCPU.

Looks to cover all cases, so it just needs converting into suitable
pieces of code comments and patch description.

Jan


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