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

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

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: Sunday, November 24, 2013 12:20 AM
> To: Wu, Feng
> Cc: Jan Beulich; xen-devel; Auld, Will; Zhang, Xiantao; Nakajima, Jun
> Subject: Re: [Xen-devel] [PATCH v4] x86: properly handle MSI-X unmask
> operation from guests
> 
> On 23/11/13 01:40, Wu, Feng wrote:
> >
> >> -----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!
> 
> Address can't reasonably be 0, meaning that an address of 0 indicates
> that an unmask is not pending.

In that case, we should make v->arch.pending_msix_unmask.ctrl_address 0 in
msix_post_handler() after unmasking the msix with this address, right?

> 
> ~Andrew

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