Re: [PATCH v3 10/10] x86/MSI-X: provide hypercall interface for mask-all control

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 05/06/15 12:28, Jan Beulich wrote:
> Qemu shouldn't be fiddling with this bit directly, as the hypervisor
> may (and now does) use it for its own purposes. Provide it with a
> replacement interface, allowing the hypervisor to track host and guest
> masking intentions independently (clearing the bit only when both want
> it clear).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Whether the permission check should really be an XSM_TARGET one needs
> to be determined: That allowing the guest to issue the hypercalls on
> itself means permitting it to bypass the device model, and thus render
> device model state stale.

I concur.

On the other hand, there should be nothing the guest could do with
access to these hypercalls which would damage Xen itself.

>
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1807,6 +1807,17 @@ int xc_physdev_unmap_pirq(xc_interface *
>                            int domid,
>                            int pirq);
>  
> +int xc_physdev_msix_enable(xc_interface *xch,
> +                           int segment,
> +                           int bus,
> +                           int devfn,
> +                           int on);
> +int xc_physdev_msix_mask_all(xc_interface *xch,
> +                             int segment,
> +                             int bus,
> +                             int devfn,
> +                             int mask);
> +
>  int xc_hvm_set_pci_intx_level(
>      xc_interface *xch, domid_t dom,
>      uint8_t domain, uint8_t bus, uint8_t device, uint8_t intx,
> --- a/tools/libxc/xc_physdev.c
> +++ b/tools/libxc/xc_physdev.c
> @@ -112,3 +112,38 @@ int xc_physdev_unmap_pirq(xc_interface *
>      return rc;
>  }
>  
> +int xc_physdev_msix_enable(xc_interface *xch,
> +                           int segment,
> +                           int bus,
> +                           int devfn,
> +                           int on)
> +{
> +    struct physdev_pci_device dev = {
> +        .seg = segment,
> +        .bus = bus,
> +        .devfn = devfn
> +    };
> +
> +    return do_physdev_op(xch,
> +                         on ? PHYSDEVOP_msix_enable
> +                            : PHYSDEVOP_msix_disable,
> +                         &dev, sizeof(dev));
> +}

xc_physdev_misx_enable(xch, X, Y, Z, 0) is slightly misleading to read...

> +
> +int xc_physdev_msix_mask_all(xc_interface *xch,
> +                             int segment,
> +                             int bus,
> +                             int devfn,
> +                             int mask)
> +{
> +    struct physdev_pci_device dev = {
> +        .seg = segment,
> +        .bus = bus,
> +        .devfn = devfn
> +    };
> +
> +    return do_physdev_op(xch,
> +                         mask ? PHYSDEVOP_msix_mask_all
> +                              : PHYSDEVOP_msix_unmask_all,
> +                         &dev, sizeof(dev));
> +}

And equally xc_physdev_misx_mask_all(xch, X, Y, Z, 0).

I think it would be cleaner to have something like

xc_physdev_msix_manage(xch, X, Y, Z, PHYSDEVOP_msix_XXX)

which results in far more readable client code.

Otherwise, the rest looks fine.