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

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Saturday, November 23, 2013 12:19 AM
> To: Wu, Feng
> Cc: Nakajima, Jun; Auld, Will; Zhang, Xiantao; xen-devel
> Subject: Re: [PATCH v4] x86: properly handle MSI-X unmask operation from
> guests
> 
> >>> 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).


The boolean variant "valid", which is set in msixtbl_write(), means whether 
there is a
msix pending unmask, if there is, just clean this flag and unmask the msix in 
hardware,
otherwise we just do nothing. If removing "valid", how can I know whether there 
is a
msix pending unmask operation ? Thanks you!

> 
> Jan


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