[PATCH v6 0/3] drop writes to read-only ram device & vfio regions

[PATCH v6 0/3] drop writes to read-only ram device & vfio regions

[Yan Zhao]

guest writes to read-only memory regions need to be dropped.

patch 1 modifies handler of ram device memory regions to drop guest writes
to read-only ram device memory regions

patch 2 modifies handler of non-mmap'd read-only vfio regions to drop guest
writes to those regions 

patch 3 set read-only flag to mmap'd read-only vfio regions, so that guest
writes to those regions would be trapped.
without patch 1, host qemu would then crash on guest write to those
read-only regions.
with patch 1, host qemu would drop the writes.

Changelog:
v6:
-fixed two style alignment problems in patch 1. (Philippe)

v5:
-changed write handler of ram device memory region from .write to
.write_with_attrs in patch 1 (Paolo)
(for vfio region in patch 2, I still keep the operations as .read & .write.
the reasons are:
1. vfio_region_ops are for mmio/pio regions. the top level read/write
dispatcher in kvm just ignores their return values. (the return value of
address_space_rw() is just ignored)
2. there are a lot of callers to vfio_region_read() and
vfio_region_write(), who actually do not care about the return values
)
-minor changes on text format in error logs.

v4:
-instead of modifying tracing log, added qemu_log_mask(LOG_GUEST_ERROR...)
to log guest writes to read-only regions (Philippe)

for
v3:
-refreshed and Cc Stefan for reviewing of tracing part

v2:
-split one big patches into smaller ones (Philippe)
-modify existing trace to record guest writes to read-only memory (Alex)
-modify vfio_region_write() to drop guest writes to non-mmap'd read-only
 region (Alex)



Yan Zhao (3):
  memory: drop guest writes to read-only ram device regions
  hw/vfio: drop guest writes to ro regions
  hw/vfio: let read-only flag take effect for mmap'd regions

 hw/vfio/common.c | 17 +++++++++++++++--
 memory.c         | 15 ++++++++++++---
 2 files changed, 27 insertions(+), 5 deletions(-)

-- 
2.17.1

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

[Yan Zhao]

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;
+    }
 
     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,
-- 
2.17.1

[PATCH v6 2/3] hw/vfio: drop guest writes to ro regions

[Yan Zhao]

for vfio regions that are without write permission,
drop guest writes to those regions.

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>
---
 hw/vfio/common.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 0b3593b3c0..2a4fedfeaa 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -38,6 +38,7 @@
 #include "sysemu/reset.h"
 #include "trace.h"
 #include "qapi/error.h"
+#include "qemu/log.h"
 
 VFIOGroupList vfio_group_list =
     QLIST_HEAD_INITIALIZER(vfio_group_list);
@@ -190,6 +191,16 @@ void vfio_region_write(void *opaque, hwaddr addr,
         uint64_t qword;
     } buf;
 
+    trace_vfio_region_write(vbasedev->name, region->nr, addr, data, size);
+    if (!(region->flags & VFIO_REGION_INFO_FLAG_WRITE)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "Invalid write to read only vfio region (%s:region%d"
+                      "+0x%"HWADDR_PRIx" size %d)\n", vbasedev->name,
+                      region->nr, addr, size);
+
+        return;
+    }
+
     switch (size) {
     case 1:
         buf.byte = data;
@@ -215,8 +226,6 @@ void vfio_region_write(void *opaque, hwaddr addr,
                      addr, data, size);
     }
 
-    trace_vfio_region_write(vbasedev->name, region->nr, addr, data, size);
-
     /*
      * A read or write to a BAR always signals an INTx EOI.  This will
      * do nothing if not pending (including not in INTx mode).  We assume
-- 
2.17.1

[PATCH v6 3/3] hw/vfio: let read-only flag take effect for mmap'd regions

[Yan Zhao]

along side setting host page table to be read-only, the memory regions
are also required to be read-only, so that when guest writes to the
read-only & mmap'd regions, vmexits would happen and region write handlers
are called.

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>
---
 hw/vfio/common.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 2a4fedfeaa..bf510e66c0 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -980,6 +980,10 @@ int vfio_region_mmap(VFIORegion *region)
                                           name, region->mmaps[i].size,
                                           region->mmaps[i].mmap);
         g_free(name);
+
+        if (!(region->flags & VFIO_REGION_INFO_FLAG_WRITE)) {
+            memory_region_set_readonly(&region->mmaps[i].mem, true);
+        }
         memory_region_add_subregion(region->mem, region->mmaps[i].offset,
                                     &region->mmaps[i].mem);
 
-- 
2.17.1

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

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

[Yan Zhao]

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

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

[Paolo Bonzini]

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

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

[Yan Zhao]

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

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

[Paolo Bonzini]

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

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

[Philippe Mathieu-Daudé]

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.

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

[Paolo Bonzini]

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

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

[Yan Zhao]

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

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

[Peter Maydell]

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

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

[Peter Maydell]

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

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

[Yan Zhao]

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

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

[Paolo Bonzini]

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

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

[Yan Zhao]

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. :)