Re: [PATCH v3 2/2] vfio: add edid support to mbochs sample driver

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

On Fri, 28 Sep 2018 01:27:16 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 9/21/2018 2:00 PM, Gerd Hoffmann wrote:
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > @@ -964,6 +1050,20 @@ static int mbochs_get_region_info(struct mdev_device *mdev,
> >  		region_info->flags  = (VFIO_REGION_INFO_FLAG_READ  |
> >  				       VFIO_REGION_INFO_FLAG_WRITE);
> >  		break;
> > +	case MBOCHS_EDID_REGION_INDEX:
> > +		ext->base.argsz = sizeof(*ext);
> > +		ext->base.offset = MBOCHS_EDID_OFFSET;
> > +		ext->base.size = MBOCHS_EDID_SIZE;
> > +		ext->base.flags = (VFIO_REGION_INFO_FLAG_READ  |
> > +				   VFIO_REGION_INFO_FLAG_WRITE |
> > +				   VFIO_REGION_INFO_FLAG_CAPS);  
> 
> Any reason to not to use _MMAP flag?
> How would QEMU side code read this region? will it be always trapped?
> If vendor driver sets _MMAP flag, will QEMU side handle that case as well?
> I think since its blob, edid could be read by QEMU using one memcpy
> rather than adding multiple memcpy of 4 or 8 bytes.

"Trapping" would only come into play if the region were exposed to the
VM, which there's no intention to do here afaik.  Also, just because
it doesn't support mmap doesn't mean that QEMU necessarily needs to
break down accesses into smaller words, QEMU could do:

pwrite(fd, buf, edid_max_size, region_offset + edid_offset)

ie. write the entire edid area with one operation.  I don't think
there's anything in the specification that prevents mmap now,
edid_offset could be at a page alignment, edid_max_size could be
PAGE_SIZE, and a sparse mmap capability could indicate that only the
EDID area is mmap'able, but is it worth the code to support that?
Thanks,

Alex