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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180927141706.1d15a579@t450s.home>
Date:   Thu, 27 Sep 2018 14:17:06 -0600
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Kirti Wankhede <kwankhede@...dia.com>
Cc:     Gerd Hoffmann <kraxel@...hat.com>,
        <intel-gvt-dev@...ts.freedesktop.org>, <kvm@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 2/2] vfio: add edid support to mbochs sample driver

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

> On 9/21/2018 2:00 PM, Gerd Hoffmann wrote:
> > Signed-off-by: Gerd Hoffmann <kraxel@...hat.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