On 9/28/2018 11:10 AM, Gerd Hoffmann wrote: >>> + 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? > > There is no page backing this. Also it is not performance-critical, > edid updates should be rare, so the extra code for mmap support doesn't > look like it is worth it. > > Also for the virtual registers (especially link_state) it is probably > useful to have the write callback of the mdev driver called to get > notified about the change. > >> How would QEMU side code read this region? will it be always trapped? > > qemu uses read & write syscalls (well, pread & pwrite actually). > >> If vendor driver sets _MMAP flag, will QEMU side handle that case as well? > > The current test branch doesn't, it expects read+write to work. > https://git.kraxel.org/cgit/qemu/log/?h=sirius/edid-vfio > Ok. Can you add a comment in vfio.h that this region is non-mmappable? >> I think since its blob, edid could be read by QEMU using one memcpy >> rather than adding multiple memcpy of 4 or 8 bytes. > > From qemu it's a single pwrite syscall actually. mbochs_write() splits > it into 4 byte writes and calls mbochs_access() for each of them. One > could probably add a special case for the EDID blob to mbochs_write(). > But again: doesn't seem worth the effort given that edid updates should > be a rare event. > Ok. Thanks, Kirti > cheers, > Gerd >
