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

[Yan Zhao <yan.y.zhao@intel.com>]

On Mon, Apr 27, 2020 at 05:31:48PM +0800, Philippe Mathieu-Daudé wrote:
> On 4/27/20 11:15 AM, Yan Zhao wrote:
> > 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é <philmd@redhat.com>
> >>>> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> >>>> Signed-off-by: Xin Zeng <xin.zeng@intel.com>
> >>>> ---
> >>>>   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?
> 
> Yes.
> 
> Please submit your series as a thread, with a cover letter:
> https://wiki.qemu.org/Contribute/SubmitAPatch#Include_a_meaningful_cover_letter
>
hi Philippe
thanks for pointing out this issue.
I just realized this version of patches were sent separately, though I did send
a cover letter. not sure what happened. maybe I just forgot to add an
-in-reply-to before sending out.
will pay attention to it next time.

Thanks
Yan

> > 
> > 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
> >>>
> >>
> > 
>