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

[Philippe Mathieu-Daudé]

Hi Yan,

On 3/27/20 12:19 PM, address@hidden wrote:
> From: Yan Zhao <address@hidden>
>
> 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 <address@hidden>
> Signed-off-by: Xin Zeng <address@hidden>
> ---
>   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;
> +    }

Shouldn't this be in memory_region_dispatch_write()?

Please split this patch in 2, this (generic) hunk as first patch, then 
the VFIO more specific change.

>   
>       switch (size) {
>       case 1: