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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.11.13 at 02:08, "Wu, Feng" <feng.wu@xxxxxxxxx> wrote:
> 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

So this is _much_ less intrusive than what you did before - good!

> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -297,6 +297,9 @@ void hvm_io_assist(ioreq_t *p)
>          break;
>      }
> 
> +    if ( msix_post_handler(curr) )
> +        gdprintk(XENLOG_ERR, "msix_post_handler error\n");
> +
>      if ( p->state == STATE_IOREQ_NONE )
>          vcpu_end_shutdown_deferral(curr);

I think the addition should be moved into the body of this if(),
so that it gets executed only upon completion of I/O, not when
it e.g. need retrying.

Also, XENLOG_ERR seems to heavy a message. XENLOG_WARN
would be the highest I'd accept.

> +int msix_post_handler(struct vcpu *v)
> +{
> +    int rc;
> +
> +    if ( v->arch.pending_msix_unmask.valid == 0 )

Iff you keep this (see below), then boolean checks are
conventionally done with ! rather than == 0.

> +        return 0;
> +
> +    v->arch.pending_msix_unmask.valid = 0;
> +
> +    rc = msixtbl_write(v, v->arch.pending_msix_unmask.ctrl_address, 4, 0);
> +    return rc != X86EMUL_OKAY ? -1 : 0;

Make the function return bool_t, and then simply 

   return msixtbl_write(v, v->arch.pending_msix_unmask.ctrl_address, 4, 0) == 
X86EMUL_OKAY;

> +struct pending_msix_unmask_info
> +{
> +    unsigned long ctrl_address;
> +    bool_t valid;
> +};
> +
>  struct arch_vcpu
>  {
>      /*
> @@ -439,6 +445,8 @@ struct arch_vcpu
> 
>      /* A secondary copy of the vcpu time info. */
>      XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
> +
> +    struct pending_msix_unmask_info pending_msix_unmask;

I don't think you need a separate boolean here - for one I don't
think the address can reasonably be zero, and even if you have
the bottom two bits available (as it being 4-byte aligned gets
checked before you consume it).

Jan


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