Re: [PATCH] hw/vfio: let readonly flag take effect for mmaped regions

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

On Fri, 27 Mar 2020 11:19:34 +0000
yan.y.zhao@intel.com wrote:

> From: Yan Zhao <yan.y.zhao@intel.com>
> 
> currently, vfio regions without VFIO_REGION_INFO_FLAG_WRITE are only
> read-only when VFIO_REGION_INFO_FLAG_MMAP is not set.
> 
> regions with flag VFIO_REGION_INFO_FLAG_READ | VFIO_REGION_INFO_FLAG_MMAP
> are only read-only in host page table for qemu.
> 
> This patch sets corresponding ept page entries read-only for regions
> with flag VFIO_REGION_INFO_FLAG_READ | VFIO_REGION_INFO_FLAG_MMAP.
> 
> accordingly, it ignores guest write when guest writes to the read-only
> regions are trapped.
> 
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Xin Zeng <xin.zeng@intel.com>
> ---

Currently we set the r/w protection on the mmap, do I understand
correctly that the change in the vfio code below results in KVM exiting
to QEMU to handle a write to a read-only region and therefore we need
the memory.c change to drop the write?  This prevents a SIGBUS or
similar?

Meanwhile vfio_region_setup() uses the same vfio_region_ops for all
regions and vfio_region_write() would still allow writes, so if the
device were using x-no-mmap=on, I think we'd still get a write to this
region and expect the vfio device to drop it.  Should we prevent that
write in QEMU as well?

Can you also identify what device and region requires this so that we
can decide whether this is QEMU 5.0 or 5.1 material?  PCI BARs are of
course always R/W and the ROM uses different ops and doesn't support
mmap, so this is a device specific region of some sort.  Thanks,

Alex


>  hw/vfio/common.c | 4 ++++
>  memory.c         | 3 +++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 0b3593b3c0..e901621ca0 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -971,6 +971,10 @@ int vfio_region_mmap(VFIORegion *region)
>                                            name, region->mmaps[i].size,
>                                            region->mmaps[i].mmap);
>          g_free(name);
> +
> +        if (!(region->flags & VFIO_REGION_INFO_FLAG_WRITE)) {
> +            memory_region_set_readonly(&region->mmaps[i].mem, true);
> +        }
>          memory_region_add_subregion(region->mem, region->mmaps[i].offset,
>                                      &region->mmaps[i].mem);
>  
> diff --git a/memory.c b/memory.c
> index 601b749906..4b1071dc74 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1313,6 +1313,9 @@ static void memory_region_ram_device_write(void *opaque, hwaddr addr,
>      MemoryRegion *mr = opaque;
>  
>      trace_memory_region_ram_device_write(get_cpu_index(), mr, addr, data, size);
> +    if (mr->readonly) {
> +        return;
> +    }
>  
>      switch (size) {
>      case 1: