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

[Yan Zhao]

On Tue, Mar 31, 2020 at 03:59:02PM +0800, Philippe Mathieu-Daudé wrote:
> On 3/27/20 5:17 PM, Paolo Bonzini wrote:
> > On 27/03/20 11:51, Philippe Mathieu-Daudé wrote:
> >>>    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()?
> > 
> > No, in general you want memory regions to get writes, so that they
> > become for example a machine-check exception of some sorts.  However,
> > memory_region_ram_device_write should probably be changed to a
> > .write_with_attrs operation, so that it can return MEMTX_ERROR.
> > 
> >> Please split this patch in 2, this (generic) hunk as first patch, then
> >> the VFIO more specific change.
> >>
> >>>          switch (size) {
> >>>        case 1:
> >>>
> >>
> > 
> > No need, I can just add my Acked-by for Alex to pick up the patch.
> 
> Having 2 different fix in 2 different patches helps when cherry-picking 
> (bisecting, backporting...) and reverting. My 2 cents anyway.
ok. I can seperate it in patch v2.

Thanks for your input:)