From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v3 10/10] x86/MSI-X: provide hypercall interface for mask-all control Date: Fri, 5 Jun 2015 16:57:05 +0100 Message-ID: <5571C6D1.1070009@citrix.com> References: <55719F9D0200007800081425@mail.emea.novell.com> <5571A3F202000078000814CA@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Z0tze-00016F-12 for xen-devel@lists.xenproject.org; Fri, 05 Jun 2015 15:57:14 +0000 In-Reply-To: <5571A3F202000078000814CA@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , xen-devel Cc: Keir Fraser , Stefano Stabellini , Ian Jackson , Ian Campbell , Wei Liu , dgdegra@tycho.nsa.gov List-Id: xen-devel@lists.xenproject.org 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 > --- > 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.