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

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Tuesday, November 26, 2013 4:48 PM
> To: Wu, Feng
> Cc: Nakajima, Jun; Auld, Will; Zhang, Xiantao; xen-devel
> Subject: Re: [PATCH v5] x86: properly handle MSI-X unmask operation from
> guests
> 
> >>> 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.

Jan, thank you very much for your review of this patch! 
Also thank Andrew!
> 
> > --- 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