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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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

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