Re: [PATCH v4 1/3] memory: drop guest writes to read-only ram device regions

[Yan Zhao]

On Sun, Apr 26, 2020 at 09:04:31AM +0800, Yan Zhao wrote:
> On Sat, Apr 25, 2020 at 06:55:33PM +0800, Paolo Bonzini wrote:
> > On 17/04/20 09:44, Yan Zhao wrote:
> > > for ram device regions, drop guest writes if the regions is read-only.
> > > 
> > > Cc: Philippe Mathieu-Daudé <address@hidden>
> > > Signed-off-by: Yan Zhao <address@hidden>
> > > Signed-off-by: Xin Zeng <address@hidden>
> > > ---
> > >  memory.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/memory.c b/memory.c
> > > index 601b749906..9576dd6807 100644
> > > --- a/memory.c
> > > +++ b/memory.c
> > > @@ -34,6 +34,7 @@
> > >  #include "sysemu/accel.h"
> > >  #include "hw/boards.h"
> > >  #include "migration/vmstate.h"
> > > +#include "qemu/log.h"
> > >  
> > >  //#define DEBUG_UNASSIGNED
> > >  
> > > @@ -1313,6 +1314,12 @@ 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) {
> > > +        qemu_log_mask(LOG_GUEST_ERROR,
> > > +                      "Invalid write to read only ram device region 0x%"
> > > +                       HWADDR_PRIx" size %u\n", addr, size);
> > > +        return;
> > > +    }
> > 
> > As mentioned in the review of v1, memory_region_ram_device_write should
> > be changed to a .write_with_attrs operation, so that it can return
> > MEMTX_ERROR.
> >
hi Paolo and Alex,
need I also change vfio_region_write() in patch 2 to a .write_with_attrs
operation?
vfio_region_read() is also possible to fail, so should I change it to a
.read_with_attrs, too?

Thanks
Yan

> > Otherwise this looks good.
> > 
> hi Paolo,
> thanks for pointing it out again!
> I didn't get your meaning in v1. will update the patch!
> 
> Thanks
> Yan
> > 
>