From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Gibson Subject: Re: [RFC PATCH kernel] vfio-pci: Allow mapping MSIX BAR Date: Wed, 29 Nov 2017 19:52:07 +1100 Message-ID: <20171129085207.GK3023@umbus.fritz.box> References: <20171122040932.970-1-aik@ozlabs.ru> <20171121212846.4d567f2e@t450s.home> <20171122044455.GR2380@umbus.fritz.box> <20171121221438.5d1021f6@t450s.home> <20171122065123.GS2380@umbus.fritz.box> <20171129035705.GE3023@umbus.fritz.box> <821be01f-b355-9b20-04a8-3281a13975d4@ozlabs.ru> <20171129051706.GJ3023@umbus.fritz.box> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="XjbSsFHOHxvQpKib" Cc: Alex Williamson , kvm@vger.kernel.org, Yongji Xie , Eric Auger To: Alexey Kardashevskiy Return-path: Received: from ozlabs.org ([103.22.144.67]:54665 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752534AbdK2Ixk (ORCPT ); Wed, 29 Nov 2017 03:53:40 -0500 Content-Disposition: inline In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: --XjbSsFHOHxvQpKib Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Nov 29, 2017 at 06:58:11PM +1100, Alexey Kardashevskiy wrote: > On 29/11/17 16:17, David Gibson wrote: > > On Wed, Nov 29, 2017 at 03:38:30PM +1100, Alexey Kardashevskiy wrote: > >> On 29/11/17 14:57, David Gibson wrote: > >>> On Wed, Nov 29, 2017 at 02:25:43PM +1100, Alexey Kardashevskiy wrote: > >>>> On 22/11/17 17:51, David Gibson wrote: > >>>>> On Tue, Nov 21, 2017 at 10:14:45PM -0700, Alex Williamson wrote: > >>>>>> On Wed, 22 Nov 2017 15:44:55 +1100 > >>>>>> David Gibson wrote: > >>>>>> > >>>>>>> On Tue, Nov 21, 2017 at 09:28:46PM -0700, Alex Williamson wrote: > >>>>>>>> On Wed, 22 Nov 2017 15:09:32 +1100 > >>>>>>>> Alexey Kardashevskiy wrote: > >>>>>>>> =20 > >>>>>>>>> By default VFIO disables mapping of MSIX BAR to the userspace as > >>>>>>>>> the userspace may program it in a way allowing spurious interru= pts; > >>>>>>>>> instead the userspace uses the VFIO_DEVICE_SET_IRQS ioctl. > >>>>>>>>> > >>>>>>>>> This works fine as long as the system page size equals to the M= SIX > >>>>>>>>> alignment requirement which is 4KB. However with a bigger page = size > >>>>>>>>> the existing code prohibits mapping non-MSIX parts of a page wi= th MSIX > >>>>>>>>> structures so these parts have to be emulated via slow reads/wr= ites on > >>>>>>>>> a VFIO device fd. If these emulated bits are accessed often, th= is has > >>>>>>>>> serious impact on performance. > >>>>>>>>> > >>>>>>>>> This adds an ioctl to the vfio-pci device which hides the sparse > >>>>>>>>> capability and allows the userspace to map a BAR with MSIX stru= ctures. =20 > >>>>>>>> > >>>>>>>> So the user is in control of telling the kernel whether they're = allowed > >>>>>>>> to mmap the msi-x vector table. That makes absolutely no sense.= If > >>>>>>>> you're trying to figure out how userspace knows whether to impli= citly > >>>>>>>> avoid mmap'ing the msix region, I think there are far better way= s in > >>>>>>>> the existing region info ioctl. We could use a flag, or maybe t= he > >>>>>>>> existence of a capability chain pointer, or a new capability. B= ut > >>>>>>>> absolutely not this. The kernel needs to decide whether it's go= ing to > >>>>>>>> let the user do this, not the user. Thanks, =20 > >>>>>>> > >>>>>>> No, it doesn't. This is actually the approach we discussed in Pr= ague. > >>>>>>> > >>>>>>> Remember that intercepting access to the MSI-X table is not a host > >>>>>>> safety / security issue. It's just that without that we won't wi= re up > >>>>>>> the device's MSI-X vectors properly so they won't work. > >>>>>>> > >>>>>>> Basically the decision here is between > >>>>>>> > >>>>>>> A) Allow MSI-X configuration via standard PCI mechanisms, at t= he > >>>>>>> cost of making access slow for any registers sharing a page= with > >>>>>>> the MSI-X table. > >>>>>>> > >>>>>>> or > >>>>>>> > >>>>>>> B) Make access to BAR registers sharing a page with the MSI-X = table > >>>>>>> fast, at the cost of requiring some alternative mechanism to > >>>>>>> configure MSI-X vectors. > >>>>>>> > >>>>>>> And that is a tradeoff that it is reasonable for userspace to mak= e. > >>>>>>> > >>>>>>> In the case of KVM guests, the decision depends entirely on the > >>>>>>> *guest* platform. Usually we need (A) because the guest expects = to be > >>>>>>> able to poke the MSI-X table in the usual way. However for PAPR > >>>>>>> guests, there's an alternative mechanism via an RTAS call, which = means > >>>>>>> we can use (B). > >>>>>>> > >>>>>>> The host kernel can't make this decision, because it doesn't know= the > >>>>>>> guest platform (well, KVM might, but VFIO doesn't). > >>>>>>> > >>>>>>> A userspace VFIO program could also elect for (B) if it does care > >>>>>>> about performance of access to registers in the same BAR as the M= SI-X > >>>>>>> table, but doesn't need MSI-X for example. > >>>>>> > >>>>>> You're asking for an ioctl to allow the kernel to allow the user to > >>>>>> mmap the page, when instead we could just allow the user to mmap t= he > >>>>>> page and whether the user does that and how they make use of it is= up > >>>>>> to them... > >>>>> > >>>>> Duh. Sorry. For some reason I was thinking the magic MSI-X > >>>>> interception was happening in the host kernel rather than in qemu. > >>>>> > >>>>>> I understand that there are different virtualization techniques at= play > >>>>>> here, it just doesn't seem relevant. In the case of (A), the user= can > >>>>>> choose not to mmap the page overlapping the vector table even if t= he > >>>>>> kernel allows it. The user can also choose to mmap that page, but= not > >>>>>> use the portion overlapping the vector table. QEMU already does t= his > >>>>>> by overlaying a MemoryRegion for vector table emulation. We might= even > >>>>>> be able to get away with mmaping that page and emulating the vector > >>>>>> table elsewhere, which seems like the only option for a 64k page A= RM > >>>>>> system. For (B), clearly it's just a nuisance that we can't curre= ntly > >>>>>> mmap this page, but I still don't see how the user allowing the ke= rnel > >>>>>> to allow the user to mmap that page makes any sense. I can't even > >>>>>> describe it without it sounding ridiculous. Thanks, > >>>>> > >>>>> Right. Rethinking.. it seems to me we should just completely remo= ve > >>>>> the logic from the kernel banning mmap()s overlapping the MSI-X > >>>>> table. All it does is poorly attempt to stop the user shooting > >>>>> themselves in the foot. > >>>>> > >>>>> Then we just need logic in qemu to avoid doing the overlapping memo= ry > >>>>> region nonsense on a per-machine basis > >>>> > >>>> > >>>> So is there still any plan or we just ditch the feature? I am confus= ed now. > >>> > >>> The plan is what I said above. Remove the bogus check logic from the > >>> kernel, then solve within qemu, by not creating the MSI-X intercept > >>> region for pseries guests. > >> > >> > >> There were 2 proposals how to do that. Both included platform code to > >> decide whether to allow mapping or not and some transport to pass th= at > >> enablement flag from the plafform code to the VFIO-PCI driver, one was= via > >> an IOMMU group flag, the other via a PCI bus flag. Neither was accepte= d so > >> reposting those won't make any progress, what do I miss here? Was ther= e any > >> agreement on how to do this? > >=20 > > As we've discussed the filtering of mmap()able regions in the kernel > > is not a security concern. All it accomplishes is poorly trying to > > stop userspace from shooting itself in the foot by directly mapping > > and accessing the MSI-X table instead of using the VFIO interfaces to > > set up MSI vectors. > >=20 > > We should simply remove - unconditionally - the checks in the kernel > > which prevent parts of the BARs from being mapped. >=20 >=20 > Just like this? And Alex is ok with that? Yes, just like this. I was kind of hoping Alex would express an opinion when I mentioned it above. But at any rate I will advocate for this approach with him. At least until someone points out another error in my thinking and I try again. >=20 >=20 > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > index 9ed1ecf..106b825 100644 > --- a/drivers/vfio/pci/vfio_pci.c > +++ b/drivers/vfio/pci/vfio_pci.c > @@ -1146,22 +1146,6 @@ static int vfio_pci_mmap(void *device_data, struct > vm_area_struct *vma) > if (req_start + req_len > phys_len) > return -EINVAL; >=20 > - if (!vdev->msix_mmap && index =3D=3D vdev->msix_bar) { > - /* > - * Disallow mmaps overlapping the MSI-X table; users don't > - * get to touch this directly. We could find somewhere > - * else to map the overlap, but page granularity is only > - * a recommendation, not a requirement, so the user needs > - * to know which bits are real. Requiring them to mmap > - * around the table makes that clear. > - */ > - > - /* If neither entirely above nor below, then it overlaps = */ > - if (!(req_start >=3D vdev->msix_offset + vdev->msix_size = || > - req_start + req_len <=3D vdev->msix_offset)) > - return -EINVAL; > - } >=20 >=20 >=20 >=20 --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --XjbSsFHOHxvQpKib Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEdfRlhq5hpmzETofcbDjKyiDZs5IFAloedS0ACgkQbDjKyiDZ s5LlSg/9EcNjZw166G/Wx3y5Sz6igJMSAV3wN87jj5B6tjGP98YsHmfUXuDvTexa if2CwURR8SXegkpWgv1t8Wb7SMXspWSd7JMVKxqgXBXDVTmOgEM3//Jj67h5q1AM 3/ZSD4TpeeuEb0wDX02gdMvHwUUaKRXg4/LzB97KQhCoXcTW3gmrepGjAVA42lNu Mtkep1mOaz/Tl0G0wLp8OVsrmDMSvD9MW5I8hQjz4deU9cQ2upAUNqWPWxExHfgr +xIsNjJtp/y+AIw1Myj1XomQZQkeiawR227rpao1e/50S/agPSxoxtUKqAQsIuB6 qSMPj1/dKelbkKtKK1sO0uJWE1iODlKbcePTD9Rx2ey69oEnJ2A48S25UfKLroOQ YjoIBoyl7tSug17RtTwY3/XKGTiaCNe9w7BR9/Nypz/nAHvid1OroPQLO5W/myS4 nJkg8Hz5bAplTi/NdoqgggVGHRidJvsOYrdfjahQnXfPghGUfaUlhnW6MNdP+RZs fwVj2v6e1Rn1Kogit/PjF5A+vtkUbrXgoAOfukC4AfrIMCT9stjVRCEpZ+L2yN6Z 3mM5IE+jWlJj7yXjVU9kqJ4KDz+ENWtVf09QkClWOmySqxTl0WcKEySCtpA6uqpk HTr/bNkXcNembobz3H3JMaAUTfuJmmiHZYisQ3HbYuxJbiCZmBc= =k9yv -----END PGP SIGNATURE----- --XjbSsFHOHxvQpKib--