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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

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

Jan


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