From: Jason Gunthorpe <jgg@ziepe.ca>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
cohuck@redhat.com, peterx@redhat.com
Subject: Re: [PATCH 2/3] vfio-pci: Fault mmaps to enable vma tracking
Date: Mon, 4 May 2020 12:05:56 -0300 [thread overview]
Message-ID: <20200504150556.GX26002@ziepe.ca> (raw)
In-Reply-To: <20200504082055.0faeef8b@x1.home>
On Mon, May 04, 2020 at 08:20:55AM -0600, Alex Williamson wrote:
> On Fri, 1 May 2020 20:25:50 -0300
> Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> > On Fri, May 01, 2020 at 03:39:19PM -0600, Alex Williamson wrote:
> > > Rather than calling remap_pfn_range() when a region is mmap'd, setup
> > > a vm_ops handler to support dynamic faulting of the range on access.
> > > This allows us to manage a list of vmas actively mapping the area that
> > > we can later use to invalidate those mappings. The open callback
> > > invalidates the vma range so that all tracking is inserted in the
> > > fault handler and removed in the close handler.
> > >
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > drivers/vfio/pci/vfio_pci.c | 76 ++++++++++++++++++++++++++++++++++-
> > > drivers/vfio/pci/vfio_pci_private.h | 7 +++
> > > 2 files changed, 81 insertions(+), 2 deletions(-)
> >
> > > +static vm_fault_t vfio_pci_mmap_fault(struct vm_fault *vmf)
> > > +{
> > > + struct vm_area_struct *vma = vmf->vma;
> > > + struct vfio_pci_device *vdev = vma->vm_private_data;
> > > +
> > > + if (vfio_pci_add_vma(vdev, vma))
> > > + return VM_FAULT_OOM;
> > > +
> > > + if (remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
> > > + vma->vm_end - vma->vm_start, vma->vm_page_prot))
> > > + return VM_FAULT_SIGBUS;
> > > +
> > > + return VM_FAULT_NOPAGE;
> > > +}
> > > +
> > > +static const struct vm_operations_struct vfio_pci_mmap_ops = {
> > > + .open = vfio_pci_mmap_open,
> > > + .close = vfio_pci_mmap_close,
> > > + .fault = vfio_pci_mmap_fault,
> > > +};
> > > +
> > > static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
> > > {
> > > struct vfio_pci_device *vdev = device_data;
> > > @@ -1357,8 +1421,14 @@ static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
> > > vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> > > vma->vm_pgoff = (pci_resource_start(pdev, index) >> PAGE_SHIFT) + pgoff;
> > >
> > > - return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
> > > - req_len, vma->vm_page_prot);
> > > + /*
> > > + * See remap_pfn_range(), called from vfio_pci_fault() but we can't
> > > + * change vm_flags within the fault handler. Set them now.
> > > + */
> > > + vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
> > > + vma->vm_ops = &vfio_pci_mmap_ops;
> >
> > Perhaps do the vfio_pci_add_vma & remap_pfn_range combo here if the
> > BAR is activated ? That way a fully populated BAR is presented in the
> > common case and avoids taking a fault path?
> >
> > But it does seem OK as is
>
> Thanks for reviewing. There's also an argument that we defer
> remap_pfn_range() until the device is actually touched, which might
> reduce the startup latency.
But not startup to a functional VM as that will now have to take the
slower fault path.
> It's also a bit inconsistent with the vm_ops.open() path where I
> can't return error, so I can't call vfio_pci_add_vma(), I can only
> zap the vma so that the fault handler can return an error if
> necessary.
open could allocate memory so the zap isn't needed. If allocation
fails then do the zap and take the slow path.
> handler. If there's a good reason to do otherwise, I can make the
> change, but I doubt I'd have encountered the dma mapping of an
> unfaulted vma issue had I done it this way, so maybe there's a test
> coverage argument as well. Thanks,
This test is best done by having one thread disable the other bar
while another thread is trying to map it
Jason
next prev parent reply other threads:[~2020-05-04 15:06 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-01 21:38 [PATCH 0/3] vfio-pci: Block user access to disabled device MMIO Alex Williamson
2020-05-01 21:39 ` [PATCH 1/3] vfio/type1: Support faulting PFNMAP vmas Alex Williamson
2020-05-01 23:50 ` Jason Gunthorpe
2020-05-04 14:06 ` Alex Williamson
2020-05-04 15:02 ` Jason Gunthorpe
2020-05-01 21:39 ` [PATCH 2/3] vfio-pci: Fault mmaps to enable vma tracking Alex Williamson
2020-05-01 23:25 ` Jason Gunthorpe
2020-05-04 14:20 ` Alex Williamson
2020-05-04 15:05 ` Jason Gunthorpe [this message]
2020-05-04 15:53 ` Alex Williamson
2020-05-01 21:39 ` [PATCH 3/3] vfio-pci: Invalidate mmaps and block MMIO access on disabled memory Alex Williamson
2020-05-01 23:48 ` Jason Gunthorpe
2020-05-04 18:26 ` Alex Williamson
2020-05-04 18:44 ` Jason Gunthorpe
2020-05-04 19:35 ` Alex Williamson
2020-05-04 20:01 ` Jason Gunthorpe
2020-05-05 17:12 ` Alex Williamson
2020-05-05 18:36 ` Jason Gunthorpe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200504150556.GX26002@ziepe.ca \
--to=jgg@ziepe.ca \
--cc=alex.williamson@redhat.com \
--cc=cohuck@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=peterx@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).