On Wed, 11 Oct 2017 12:47:20 +0200
Gerd Hoffmann <kra...@redhat.com> wrote:
> Hi,
>
> > > + ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_QUERY_GFX_PLANE,
> > > &plane);
> > > + if (ret < 0) {
> > > + fprintf(stderr, "ioctl VFIO_DEVICE_QUERY_GFX_PLANE: %s\n",
> > > + strerror(errno));
> >
> > %m?
>
> Oh, neat, didn't know this even exists.
>
> man page says it is a glibc extension though. So do we really want use
> it in a portable code base? In this specific case it would probably
> not cause any trouble though as vfio is linux-only anyway.
Right, vfio is linux only, so I haven't really hesitated to use it.
> > > + if (vdev->region_mmap == NULL) {
> > > + /* mmap region */
> > > + ret = vfio_get_region_info(&vdev->vbasedev,
> > > plane.region_index,
> > > + ®ion);
> > > + if (ret != 0) {
> > > + fprintf(stderr, "%s: vfio_get_region_info(%d): %s\n",
> > > + __func__, plane.region_index, strerror(-ret));
> > > + return;
> > > + }
> > > + vdev->region_size = region->size;
> > > + vdev->region_mmap = mmap(NULL, region->size,
> > > + PROT_READ, MAP_SHARED,
> > > + vdev->vbasedev.fd,
> > > + region->offset);
> > > + if (vdev->region_mmap == MAP_FAILED) {
> > > + fprintf(stderr, "%s: mmap region %d: %s\n", __func__,
> > > + plane.region_index, strerror(errno));
> > > + vdev->region_mmap = NULL;
> > > + g_free(region);
> > > + return;
> > > + }
> >
> > Seems like we should really try to use a VFIORegion for this.
>
> IIRC I checked this, but it didn't look straight forward as VFIORegion
> is designed for guest access not host access.
The overhead of a VFIORegion seems to be that we setup a MemoryRegion
for r/w access to the vfio region and overlap that with one or more
MemoryRegions for the mmap(s). That's a bit of structural overhead,
but we'd simply never map those into a guest visible address space.
OTOH, it saves you from dealing with the region info, and potentially
sparse mmap (you could call vfio_region_setup() and check nr_mmaps = 1,
mmaps[0].offset/size, then call vfio_region_mmap() if it checks out,
otherwise error). Just seems like duplicate code here even if the
VFIORegion includes some things we don't need. Thanks,
Alex
