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

["Xuquan (Quan Xu)" <xuquan8@xxxxxxxxxx>]

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.


>> 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?

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


>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.

-Quan 

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