Re: [Qemu-devel] [RfC PATCH 5/6] vfio/display: adding region support

[Alex Williamson <alex.williamson@redhat.com>]

On Wed, 11 Oct 2017 12:47:20 +0200
Gerd Hoffmann <kraxel@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,
> > > +                                   &region);
> > > +        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