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

["Xu, Dongxiao" <dongxiao.xu@xxxxxxxxx>]

> -----Original Message-----
> From: xen-devel-bounces@xxxxxxxxxxxxx
> [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On Behalf Of Jan Beulich
> Sent: Monday, November 11, 2013 9:19 PM
> To: Wu, Feng
> Cc: xen-devel; Zhang, Xiantao
> Subject: Re: [Xen-devel] [PATCH] x86: properly handle MSI-X unmask operation
> from guests
> 
> >>> On 11.11.13 at 13:33, "Wu, Feng" <feng.wu@xxxxxxxxx> wrote:
> > --- a/tools/libxc/xc_domain.c
> > +++ b/tools/libxc/xc_domain.c
> > @@ -1342,6 +1342,8 @@ int xc_domain_update_msi_irq(
> >      uint32_t gvec,
> >      uint32_t pirq,
> >      uint32_t gflags,
> > +    uint32_t vector_ctrl,
> > +    int nr_entry,
> >      uint64_t gtable)
> >  {
> 
> With there being out of tree consumers (like upstream qemu), you
> can't just add two parameters here.
> 
> > +int msixtbl_unmask(struct vcpu *v, unsigned long table_base,
> > +                         unsigned int nr_entry)
> > +{
> > +    unsigned long ctrl_address;
> > +
> > +    ctrl_address = table_base + nr_entry * PCI_MSIX_ENTRY_SIZE +
> > +                   PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET;
> > +
> > +    if (msixtbl_write(v, ctrl_address, 4, 0) != X86EMUL_OKAY )
> 
> So once again you clear the mask bit with no consideration
> whatsoever as to the state Xen want the mask bit to be in. Did
> you not read through the history of all these MSI-X related
> changes? Otherwise you should have known that it is precisely
> this which we must not allow.
> 
> > --- a/xen/drivers/passthrough/io.c
> > +++ b/xen/drivers/passthrough/io.c
> > @@ -193,6 +193,13 @@ int pt_irq_create_bind(
> >          spin_unlock(&d->event_lock);
> >          if ( dest_vcpu_id >= 0 )
> >              hvm_migrate_pirqs(d->vcpu[dest_vcpu_id]);
> > +
> > +        if ((pt_irq_bind->u.msi.vector_ctrl & PCI_MSIX_VECTOR_BITMASK)
> == 0) {
> > +            rc = msixtbl_unmask(d->vcpu[0], pt_irq_bind->u.msi.gtable,
> > +                           pt_irq_bind->u.msi.nr_entry);
> > +            if (rc)
> > +                return -EBUSY;
> > +        }
> 
> Without explanation in the commit message I don't see why this
> adjustment is needed.
> 
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -503,6 +503,8 @@ struct xen_domctl_bind_pt_irq {
> >          struct {
> >              uint8_t gvec;
> >              uint32_t gflags;
> > +            uint32_t vector_ctrl;
> > +            int nr_entry;
> >              uint64_aligned_t gtable;
> >          } msi;
> >      } u;
> 
> Andrew already told you: _If_ you need to change this, it has to
> be accompanied by an interface version change.
> 
> But I this should be done entirely inside the hypervisor - after all
> it is the hypervisor that forwards the request to qemu, so upon
> completion of the request it could as well do the necessary
> unmasking (as long as it doesn't need the interrupt masked for
> internal purposes).

Currently, Feng's patch handles the unmask operation when QEMU issues the MSI-X 
update hypercall (XEN_DOMCTL_bind_pt_irq).
However, if add the unmask operation in Xen's generic I/O emulation code path, 
we may need to introduce a new MSI-X specific callback function, which is 
supposed to be executed after QEMU I/O operation is done. Is this what you mean?

Thanks,
Dongxiao

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

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