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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.03.17 at 04:23, <xuquan8@xxxxxxxxxx> wrote:
> On February 28, 2017 11:08 PM, Jan Beulich wrote:
>>>>> On 27.02.17 at 11:53, <xuquan8@xxxxxxxxxx> wrote:
>>> If guest is already in non-root mode, an posted interrupt will be
>>> directly delivered to guest (leaving softirq being set without
>>> actually incurring a VM-Exit - breaking desired softirq behavior).
>>
>>This is irritating - are you describing a problem you mean to fix with this 
>>patch,
>>without making clear what the fix is? Or are you describing state after this
>>patch (which the code below suggests), in which case I have to say no, we
>>certainly don't want this.
>>
> 
> Sorry. I knew you would not like any assumption in patch description.. 
> But I think this one really help community understand what the problem is( 
> this is also valuable).
> Also, as you know, I am a clumsy in patch description and always open to 
> your suggestion.
> :) please don't be irritated.. if you have a better description, I am always 
> open to it.

Well, the problem here is that a suitable patch description is vital to
understand what was wrong and why the new behavior is what we
want. Hence you really need to view me as the consumer of it, i.e.
you can't rely on me to describe your change and/or findings.

>>> Then further posted interrupts will skip the IPI, stay in PIR and not
>>> noted until another VM-Exit happens.
>>>
>>> When target vCPU is in non-root mode and running on remote CPU, posted
>>> way is triggered to inject interrupt without VM-Exit.
>>> Otherwise, set VCPU_KICK_SOFTIRQ bit to inject interrupt until another
>>> VM-Entry as a best effort.
>>
>>Furthermore you talking about non-root mode here doesn't fit with the code
>>change, as you can't find out whether the guest is in non-root mode.
>>
> 
> Reply in below..
> 
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -1839,17 +1839,14 @@ static void vmx_process_isr(int isr, struct
>>> vcpu *v)
>>>
>>>  static void __vmx_deliver_posted_interrupt(struct vcpu *v)  {
>>> -    bool_t running = v->is_running;
>>> +    unsigned int cpu = v->processor;
>>>
>>>      vcpu_unblock(v);
>>> -    if ( running && (in_irq() || (v != current)) )
>>> -    {
>>> -        unsigned int cpu = v->processor;
>>> -
>>> -        if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ,
>>&softirq_pending(cpu))
>>> -             && (cpu != smp_processor_id()) )
>>> -            send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
>>> -    }
>>> +    if ( v->is_running && (in_irq() || (v != current)) &&
>>> +         (cpu != smp_processor_id()) )
>>> +        send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
>>> +    else
>>> +        set_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu));
>>
>>Why don't you simply call raise_softirq() here?
>>
> 
> raise_softirq() is a better wrapper, but I didn't get it until you told me.
> 
> 
>>Plus, if we already want to fix this and _eliminate_ (rather than
>>shrink) any time windows, is namely looking at v->is_running really useful
>>here? By the time you do the next part of the conditional (or call into
>>send_IPI_mask()) that may already have changed. 
> 
> So far, I can't find a better one to check whether the vcpu is in non-root or 
> not.
> Any suggestion?

As said before - you can't find out easily, and if you managed to,
by the time you look at the result that result might be stale. Hence
the problem and/or patch description should be written in different
terms. You may, for example, explain (if that's the case, of course)
...

> I am afraid we could not eliminate any time windows, but try our best.

... that v->is_running set covers a superset of the time the vCPU
spends in non-root mode, with it being acceptable to _also_ do the
same actions if the flag is set but the vCPU is in root mode. In such
a scenario there would indeed be no time window left. But as I
continue to not fully understand you intentions, I can't judge
whether this applies here.

>>Similarly, while this isn't
>>something you change, I don't really understand the relevance of using 
>>in_irq()
>>here. Hence at the very least a code comment should be added imo, clarifying
>>what all this magic is about.
>>
> 
> 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.

Jan

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