On Thu, Apr 30, 2020 at 05:40:25PM +0800, Peter Maydell wrote: > On Thu, 30 Apr 2020 at 09:20, Yan Zhao <yan.y.z...@intel.com> wrote: > > > > for ram device regions, drop guest writes if the region is read-only. > > > > Cc: Philippe Mathieu-Daudé <phi...@redhat.com> > > Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com> > > Signed-off-by: Yan Zhao <yan.y.z...@intel.com> > > Signed-off-by: Xin Zeng <xin.z...@intel.com> > > --- > > 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). > hmm, I'm not sure. so your expectation is silently dropping guest writes without any bus error, right?
> 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 it's not a ROM, but a ram region backed by a device. we wish this region to be read-only sometimes, in order to implement some useful features. It can be a virtual BAR region in a virtual mdev device. > 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. currently, vfio implements the BAR regions in two types: 1. non-mmap'd, meaning this region will not be added into kvm memory slots, and whenever guest accesses it, it will be trapped into a host handler. we do the read-only check in patch 2 of this series. 2. mmap'd, meaning this region will be added into kvm memory slots, and guest could access it without any hypervisor intervening. so without patch 3 in the series, there's no write protection to guest writes. after setting this mmap'd region to read-only in patch 3, the corresponding memory slot in kvm is set to read-only, so only guest writes would be trapped into host, i.e. into the memory_region_ram_device_write(). guest reads is still within the guest without hypervisor intervening. > > 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. > there might be other MRs that require checking of read-only-ness. but their handlers have the right to be called to know it has happened, and they might want to do some special handling of it. That's why I did not put the check in general dispatcher. Thanks Yan