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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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).

Jan


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