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

[Peter Maydell]

On Thu, 30 Apr 2020 at 09:20, Yan Zhao <address@hidden> wrote:
>
> for ram device regions, drop guest writes if the region is read-only.
>
> Cc: Philippe Mathieu-Daudé <address@hidden>
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> Signed-off-by: Yan Zhao <address@hidden>
> Signed-off-by: Xin Zeng <address@hidden>
> ---
>  memory.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/memory.c b/memory.c
> index 601b749906..a1bba985b9 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
>
> @@ -1307,12 +1308,19 @@ static uint64_t memory_region_ram_device_read(void 
> *opaque,
>      return data;
>  }
>
> -static void memory_region_ram_device_write(void *opaque, hwaddr addr,
> -                                           uint64_t data, unsigned size)
> +static MemTxResult memory_region_ram_device_write(void *opaque, hwaddr addr,
> +                                                  uint64_t data, unsigned 
> size,
> +                                                  MemTxAttrs attrs)
>  {
>      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 addr 0x%"
> +                      HWADDR_PRIx" size %u\n", addr, size);
> +        return MEMTX_ERROR;
> +    }

This does not "drop" a write to a r/o region -- it causes it to generate
whatever the guest architecture's equivalent of a bus error is (eg data
abort on Arm).

More generally, this change seems a bit odd: currently we do not
check the mr->readonly flag here, but in general guests don't get
to write to ROM areas. Where is that check currently done, and
should the vfio case you're trying to fix do its check in whatever
the equivalent of that place is? Alternatively, if we want to make
memory_region_ram_device_write() do the check, does that mean we
now have unnecessary checks elsewhere.

My guess is that memory_region_ram_device_write() isn't the
right place to check for read-only-ness, because it only applies
to RAM-backed MRs, not to any other kind of MR which might equally
be readonly.

thanks
-- PMM