| Message ID | 20200430080946.31286-1-yan.y.zhao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drop writes to read-only ram device & vfio regions | expand |
On Thu, 30 Apr 2020 at 09:20, Yan Zhao <yan.y.zhao@intel.com> wrote: > > for ram device regions, drop guest writes if the region is read-only. > > Cc: Philippe Mathieu-Daudé <philmd@redhat.com> > Reviewed-by: 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 | 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
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.zhao@intel.com> wrote: > > > > for ram device regions, drop guest writes if the region is read-only. > > > > Cc: Philippe Mathieu-Daudé <philmd@redhat.com> > > Reviewed-by: 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 | 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
On 30/04/20 11:40, Peter Maydell wrote: >> 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 Writes to ROM are directed to mr->ops unassigned_mem_ops. Because _all_ ram-device reads and writes go through the ops, for ram-device we have to stick the check for mr->readonly in the ops. On one hand, I was quite surprised to see that unassigned_mem_write does not return MEMTX_ERROR now that I looked at it. On the other hand, we should use MEMTX_ERROR in patch 2 as well, if we decide it's the way to go. (Sorry Yan for the late response). Paolo
On Thu, May 21, 2020 at 04:38:47PM +0200, Paolo Bonzini wrote: > On 30/04/20 11:40, Peter Maydell wrote: > >> 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 > > Writes to ROM are directed to mr->ops unassigned_mem_ops. Because _all_ > ram-device reads and writes go through the ops, for ram-device we have > to stick the check for mr->readonly in the ops. > > On one hand, I was quite surprised to see that unassigned_mem_write does > not return MEMTX_ERROR now that I looked at it. > > On the other hand, we should use MEMTX_ERROR in patch 2 as well, if we > decide it's the way to go. > > (Sorry Yan for the late response). > hi Paolo, thanks for your reply and never mind :) But there's one thing I just can't figure out the reason and eagerly need your guide. why do we have to convert all .write operations to .write_with_attrs and return MEMTX_ERROR? because of the handling of writes to read-only region? however, it seems that all regions have to handle this case, so ultimately we have to convert all .write to .write_with_attrs and there would be no .write operations any more? Thanks Yan
On 25/05/20 03:18, Yan Zhao wrote: > On Thu, May 21, 2020 at 04:38:47PM +0200, Paolo Bonzini wrote: >> On 30/04/20 11:40, Peter Maydell wrote: >>>> 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 >> >> Writes to ROM are directed to mr->ops unassigned_mem_ops. Because _all_ >> ram-device reads and writes go through the ops, for ram-device we have >> to stick the check for mr->readonly in the ops. >> >> On one hand, I was quite surprised to see that unassigned_mem_write does >> not return MEMTX_ERROR now that I looked at it. >> >> On the other hand, we should use MEMTX_ERROR in patch 2 as well, if we >> decide it's the way to go. >> >> (Sorry Yan for the late response). >> > hi Paolo, > thanks for your reply and never mind :) > > But there's one thing I just can't figure out the reason and eagerly need > your guide. > > why do we have to convert all .write operations to .write_with_attrs and > return MEMTX_ERROR? because of the handling of writes to read-only region? Not all of them, only those that need to return MEMTX_ERROR. I would like some guidance from Peter as to whether (or when) reads from ROMs should return MEMTX_ERROR. This way, we can use that information to device what the read-only ram-device regions should do. Paolo
On 5/25/20 12:20 PM, Paolo Bonzini wrote: > On 25/05/20 03:18, Yan Zhao wrote: >> On Thu, May 21, 2020 at 04:38:47PM +0200, Paolo Bonzini wrote: >>> On 30/04/20 11:40, Peter Maydell wrote: >>>>> 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 >>> >>> Writes to ROM are directed to mr->ops unassigned_mem_ops. Because _all_ >>> ram-device reads and writes go through the ops, for ram-device we have >>> to stick the check for mr->readonly in the ops. >>> >>> On one hand, I was quite surprised to see that unassigned_mem_write does >>> not return MEMTX_ERROR now that I looked at it. >>> >>> On the other hand, we should use MEMTX_ERROR in patch 2 as well, if we >>> decide it's the way to go. >>> >>> (Sorry Yan for the late response). >>> >> hi Paolo, >> thanks for your reply and never mind :) >> >> But there's one thing I just can't figure out the reason and eagerly need >> your guide. >> >> why do we have to convert all .write operations to .write_with_attrs and >> return MEMTX_ERROR? because of the handling of writes to read-only region? > > Not all of them, only those that need to return MEMTX_ERROR. I would > like some guidance from Peter as to whether (or when) reads from ROMs > should return MEMTX_ERROR. This way, we can use that information to > device what the read-only ram-device regions should do. Is it only device-specific or might it be partly arch/machine-specific (depending on the bus it is mapped)? Phil.
On 25/05/20 12:54, Philippe Mathieu-Daudé wrote: >> Not all of them, only those that need to return MEMTX_ERROR. I would >> like some guidance from Peter as to whether (or when) reads from ROMs >> should return MEMTX_ERROR. This way, we can use that information to >> device what the read-only ram-device regions should do. > Is it only device-specific or might it be partly arch/machine-specific > (depending on the bus it is mapped)? Good point, I think that could be handled by propagating the error up in the memory region hierarchy (i.e. the cached AddressSpaceDispatch radix tree is used in the common case, but when you have a failure you percolate it up through the whole hierarchy since that's not a fast path). Paolo
On Mon, May 25, 2020 at 01:04:56PM +0200, Paolo Bonzini wrote: > On 25/05/20 12:54, Philippe Mathieu-Daudé wrote: > >> Not all of them, only those that need to return MEMTX_ERROR. I would > >> like some guidance from Peter as to whether (or when) reads from ROMs > >> should return MEMTX_ERROR. This way, we can use that information to > >> device what the read-only ram-device regions should do. > > Is it only device-specific or might it be partly arch/machine-specific > > (depending on the bus it is mapped)? > > Good point, I think that could be handled by propagating the error up in > the memory region hierarchy (i.e. the cached AddressSpaceDispatch radix > tree is used in the common case, but when you have a failure you > percolate it up through the whole hierarchy since that's not a fast path). > > but if we decide to propagate the error up by providing with ops->write_with_attrs, then we have to remove ops->write correspondingly. as in memory_region_dispatch_write() { ... if (mr->ops->write) { return access_with_adjusted_size(addr, &data, size, mr->ops->impl.min_access_size, mr->ops->impl.max_access_size, memory_region_write_accessor, mr, attrs); } else { return access_with_adjusted_size(addr, &data, size, mr->ops->impl.min_access_size, mr->ops->impl.max_access_size, memory_region_write_with_attrs_accessor, mr, attrs); } ... } so which regions should keep ops->write and which regions should not? Thanks Yan
On Tue, 26 May 2020 at 03:21, Yan Zhao <yan.y.zhao@intel.com> wrote:
> so which regions should keep ops->write and which regions should not?
Devices which never need to return a transaction failure
and which never care about transaction attributes can
continue to use ops->write (this is most devices).
It's only if you actually need to be able to say "that
transaction failed" or you need to look at the attributes
that you have to implement write_with_attrs -- there
are a lot fewer of these.
(You could argue for a refactoring where we drop the
old ->read and ->write methods on devices and then
rename read_with_attrs and write_with_attrs to read
and write, but unless we can manage to do it entirely
automatedly it seems like too much effort to me.)
thanks
-- PMM
On Mon, 25 May 2020 at 11:20, Paolo Bonzini <pbonzini@redhat.com> wrote: > Not all of them, only those that need to return MEMTX_ERROR. I would > like some guidance from Peter as to whether (or when) reads from ROMs > should return MEMTX_ERROR. This way, we can use that information to > device what the read-only ram-device regions should do. In general I think writes to ROMs (and indeed reads from ROMs) should not return MEMTX_ERROR. I think that in real hardware you could have a ROM that behaved either way; so our default behaviour should probably be to do what we've always done and not report a MEMTX_ERROR. (If we needed to I suppose we should implement a MEMTX_ERROR-reporting ROM, but to be honest there aren't really many real ROMs in systems these days: it's more often flash, whose response to writes is defined by the spec and is I think to ignore writes which aren't the magic "shift to program-the-flash-mode" sequence.) thanks -- PMM
On Tue, May 26, 2020 at 10:26:35AM +0100, Peter Maydell wrote: > On Mon, 25 May 2020 at 11:20, Paolo Bonzini <pbonzini@redhat.com> wrote: > > Not all of them, only those that need to return MEMTX_ERROR. I would > > like some guidance from Peter as to whether (or when) reads from ROMs > > should return MEMTX_ERROR. This way, we can use that information to > > device what the read-only ram-device regions should do. > > In general I think writes to ROMs (and indeed reads from ROMs) should > not return MEMTX_ERROR. I think that in real hardware you could have > a ROM that behaved either way; so our default behaviour should probably > be to do what we've always done and not report a MEMTX_ERROR. (If we > needed to I suppose we should implement a MEMTX_ERROR-reporting ROM, > but to be honest there aren't really many real ROMs in systems these > days: it's more often flash, whose response to writes is defined > by the spec and is I think to ignore writes which aren't the > magic "shift to program-the-flash-mode" sequence.) > then should I just drop the writes to read-only ram-device regions and vfio regions without returning MEMTX_ERROR? do you think it's good? Thanks Yan
On 28/05/20 06:35, Yan Zhao wrote: > On Tue, May 26, 2020 at 10:26:35AM +0100, Peter Maydell wrote: >> On Mon, 25 May 2020 at 11:20, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> Not all of them, only those that need to return MEMTX_ERROR. I would >>> like some guidance from Peter as to whether (or when) reads from ROMs >>> should return MEMTX_ERROR. This way, we can use that information to >>> device what the read-only ram-device regions should do. >> >> In general I think writes to ROMs (and indeed reads from ROMs) should >> not return MEMTX_ERROR. I think that in real hardware you could have >> a ROM that behaved either way; so our default behaviour should probably >> be to do what we've always done and not report a MEMTX_ERROR. (If we >> needed to I suppose we should implement a MEMTX_ERROR-reporting ROM, >> but to be honest there aren't really many real ROMs in systems these >> days: it's more often flash, whose response to writes is defined >> by the spec and is I think to ignore writes which aren't the >> magic "shift to program-the-flash-mode" sequence.) >> > then should I just drop the writes to read-only ram-device regions and > vfio regions without returning MEMTX_ERROR? > do you think it's good? I am not really sure, I have to think more about it. I think read-only RAMD regions are slightly different because the guest can expect "magic" behavior from RAMD regions (e.g. registers that trigger I/O on writes) that are simply not there for ROM. So I'm still inclined to queue your v6 patch series. Thanks, Paolo
On Thu, May 28, 2020 at 07:10:46AM +0200, Paolo Bonzini wrote: > On 28/05/20 06:35, Yan Zhao wrote: > > On Tue, May 26, 2020 at 10:26:35AM +0100, Peter Maydell wrote: > >> On Mon, 25 May 2020 at 11:20, Paolo Bonzini <pbonzini@redhat.com> wrote: > >>> Not all of them, only those that need to return MEMTX_ERROR. I would > >>> like some guidance from Peter as to whether (or when) reads from ROMs > >>> should return MEMTX_ERROR. This way, we can use that information to > >>> device what the read-only ram-device regions should do. > >> > >> In general I think writes to ROMs (and indeed reads from ROMs) should > >> not return MEMTX_ERROR. I think that in real hardware you could have > >> a ROM that behaved either way; so our default behaviour should probably > >> be to do what we've always done and not report a MEMTX_ERROR. (If we > >> needed to I suppose we should implement a MEMTX_ERROR-reporting ROM, > >> but to be honest there aren't really many real ROMs in systems these > >> days: it's more often flash, whose response to writes is defined > >> by the spec and is I think to ignore writes which aren't the > >> magic "shift to program-the-flash-mode" sequence.) > >> > > then should I just drop the writes to read-only ram-device regions and > > vfio regions without returning MEMTX_ERROR? > > do you think it's good? > > I am not really sure, I have to think more about it. I think read-only > RAMD regions are slightly different because the guest can expect "magic" > behavior from RAMD regions (e.g. registers that trigger I/O on writes) > that are simply not there for ROM. So I'm still inclined to queue your > v6 patch series. > ok. thank you Paolo. :)
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; + } switch (size) { case 1: @@ -1328,11 +1336,12 @@ static void memory_region_ram_device_write(void *opaque, hwaddr addr, *(uint64_t *)(mr->ram_block->host + addr) = data; break; } + return MEMTX_OK; } static const MemoryRegionOps ram_device_mem_ops = { .read = memory_region_ram_device_read, - .write = memory_region_ram_device_write, + .write_with_attrs = memory_region_ram_device_write, .endianness = DEVICE_HOST_ENDIAN, .valid = { .min_access_size = 1,