Re: [Xen-devel] [PATCH v5] x86: properly handle MSI-X unmask operation from guests

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.11.13 at 03:05, "Wu, Feng" <feng.wu@xxxxxxxxx> wrote:
> patch revision history
> ----------------------
> v1: Initial patch to handle this issue involving changing the hypercall 
> interface
> v2:Totally handled inside hypervisor.
> v3:Change some logics of handling msi-x pending unmask operations.
> v4:Some changes related to coding style according to Andrew Cooper's 
> comments
> v5:Some changes according to Jan's comments, including
>       a) remove "valid" field, use "ctrl_address" itself to judge if there is 
> a valid
>       pending msix unmask operation
>     b) Call msix_post_handler() when (p->state == STATE_IOREQ_NONE), which
>       means it gets executed only upon completion of I/O

I'm fine with this now, except for some cosmetic things which I
would take the liberty of changing on the fly while committing,
assuming there aren't going to be other comments requiring
another revision.

> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -298,7 +298,11 @@ void hvm_io_assist(ioreq_t *p)
>      }
> 
>      if ( p->state == STATE_IOREQ_NONE )
> +    {
> +        if ( !msix_post_handler(curr) )
> +            gdprintk(XENLOG_WARNING, "msix_post_handler error\n");

There's really no point in issuing the warning here rather than in
the function itself; the function could therefore return "void".

Further, the function name is bogus: It suggests to deal with
some "post" operation. I'd rename it to msix_write_completion()
or some such.

> @@ -528,3 +531,15 @@ void msixtbl_pt_cleanup(struct domain *d)
>      spin_unlock(&d->arch.hvm_domain.msixtbl_list_lock);
>      local_irq_restore(flags);
>  }
> +
> +bool_t msix_post_handler(struct vcpu *v)
> +{
> +    unsigned long ctrl_address = v->arch.pending_msix_unmask.ctrl_address;
> +
> +    if ( ctrl_address == 0 )
> +        return 1;
> +
> +    v->arch.pending_msix_unmask.ctrl_address = 0;
> +    return msixtbl_write(v, ctrl_address, 4, 0) == X86EMUL_OKAY;
> +}
> +

Stray blank line.

> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -375,6 +375,11 @@ struct pv_vcpu
>      spinlock_t shadow_ldt_lock;
>  };
> 
> +struct pending_msix_unmask_info
> +{
> +    unsigned long ctrl_address;
> +};

Hardly worth a separate structure.

Jan


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