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

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

[Yan Zhao]

Forgot the changelog, so sent this patch 0/3 again.
sorry about that.

On Sat, Apr 04, 2020 at 01:08:23AM +0800, Zhao, Yan Y wrote:
> 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 let mmap'd read-only vfio regions be able to generate vmexit for
> guest write. so, without patch 1, host qemu would crash on guest write to
> this read-only region. with patch 1, host qemu would drop the writes.
> 
> Changelog:
> 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     | 12 +++++++++++-
>  hw/vfio/trace-events |  2 +-
>  memory.c             |  6 +++++-
>  trace-events         |  2 +-
>  4 files changed, 18 insertions(+), 4 deletions(-)
> 
> -- 
> 2.17.1
> 

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

[Peter Maydell]

On Fri, 3 Apr 2020 at 09:13, Yan Zhao <yan.y.zhao@intel.com> wrote:
>
> 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 let mmap'd read-only vfio regions be able to generate vmexit for
> guest write. so, without patch 1, host qemu would crash on guest write to
> this read-only region. with patch 1, host qemu would drop the writes.

Just FYI, there seems to be a minor clock or timezone issue with
however you're sending these emails: the Date header you
sent reads "Date: Fri, 3 Apr 2020 16:56:57 +0000" but that
time in the UTC +0000 timezone is (as I write this) still several
hours in the future. This isn't a big deal but you probably want
to look into fixing it.

(Noticed because the wrong-date makes your patches stick to
the top of the https://patchew.org/QEMU/ list even after other
patches arrive after them.)

thanks
-- PMM

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

[Yan Zhao]

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 let mmap'd read-only vfio regions be able to generate vmexit for
guest write. so, without patch 1, host qemu would crash on guest write to
this read-only region. with patch 1, host qemu would drop the writes.

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     | 12 +++++++++++-
 hw/vfio/trace-events |  2 +-
 memory.c             |  6 +++++-
 trace-events         |  2 +-
 4 files changed, 18 insertions(+), 4 deletions(-)

-- 
2.17.1

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

[Yan Zhao]

for ram device regions, drop guest writes if the regions is read-only.

Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
Signed-off-by: Xin Zeng <xin.zeng@intel.com>
---
 memory.c     | 6 +++++-
 trace-events | 2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/memory.c b/memory.c
index 601b749906..a380b59980 100644
--- a/memory.c
+++ b/memory.c
@@ -1312,7 +1312,11 @@ 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);
+    trace_memory_region_ram_device_write(get_cpu_index(), mr, addr, data,
+                                         size, mr->readonly);
+    if (mr->readonly) {
+        return;
+    }
 
     switch (size) {
     case 1:
diff --git a/trace-events b/trace-events
index 42107ebc69..e1de662973 100644
--- a/trace-events
+++ b/trace-events
@@ -61,7 +61,7 @@ memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t value,
 memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size %u"
 memory_region_subpage_write(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size %u"
 memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
-memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
+memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size, bool readonly) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u" " is_readonly_region=%d"
 flatview_new(void *view, void *root) "%p (root %p)"
 flatview_destroy(void *view, void *root) "%p (root %p)"
 flatview_destroy_rcu(void *view, void *root) "%p (root %p)"
-- 
2.17.1

[PATCH v2 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.

Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
Signed-off-by: Xin Zeng <xin.zeng@intel.com>
---
 hw/vfio/common.c     | 8 +++++++-
 hw/vfio/trace-events | 2 +-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 0b3593b3c0..fd6ee1fe3e 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -190,6 +190,11 @@ void vfio_region_write(void *opaque, hwaddr addr,
         uint64_t qword;
     } buf;
 
+    if (!(region->flags & VFIO_REGION_INFO_FLAG_WRITE)) {
+        trace_vfio_region_write(vbasedev->name, region->nr,
+                                   addr, data, size, true);
+        return;
+    }
     switch (size) {
     case 1:
         buf.byte = data;
@@ -215,7 +220,8 @@ void vfio_region_write(void *opaque, hwaddr addr,
                      addr, data, size);
     }
 
-    trace_vfio_region_write(vbasedev->name, region->nr, addr, data, size);
+    trace_vfio_region_write(vbasedev->name, region->nr, addr, data, size,
+                            false);
 
     /*
      * A read or write to a BAR always signals an INTx EOI.  This will
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index b1ef55a33f..fb9ff604e6 100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -91,7 +91,7 @@ vfio_pci_nvlink2_setup_quirk_ssatgt(const char *name, uint64_t tgt, uint64_t siz
 vfio_pci_nvlink2_setup_quirk_lnkspd(const char *name, uint32_t link_speed) "%s link_speed=0x%x"
 
 # common.c
-vfio_region_write(const char *name, int index, uint64_t addr, uint64_t data, unsigned size) " (%s:region%d+0x%"PRIx64", 0x%"PRIx64 ", %d)"
+vfio_region_write(const char *name, int index, uint64_t addr, uint64_t data, unsigned size, bool readonly) " (%s:region%d+0x%"PRIx64", 0x%"PRIx64 ", %d)" " is_readonly_region=%d."
 vfio_region_read(char *name, int index, uint64_t addr, unsigned size, uint64_t data) " (%s:region%d+0x%"PRIx64", %d) = 0x%"PRIx64
 vfio_iommu_map_notify(const char *op, uint64_t iova_start, uint64_t iova_end) "iommu %s @ 0x%"PRIx64" - 0x%"PRIx64
 vfio_listener_region_add_skip(uint64_t start, uint64_t end) "SKIPPING region_add 0x%"PRIx64" - 0x%"PRIx64
-- 
2.17.1

[PATCH v2 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.

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 fd6ee1fe3e..fc7618e041 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -977,6 +977,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

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

[Yan Zhao]

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 let mmap'd read-only vfio regions be able to generate vmexit for
guest write. so, without patch 1, host qemu would crash on guest write to
this read-only region. with patch 1, host qemu would drop the writes.

Changelog:
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     | 12 +++++++++++-
 hw/vfio/trace-events |  2 +-
 memory.c             |  6 +++++-
 trace-events         |  2 +-
 4 files changed, 18 insertions(+), 4 deletions(-)

-- 
2.17.1

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

[Yan Zhao]

On Fri, Apr 03, 2020 at 08:53:12PM +0800, Peter Maydell wrote:
> On Fri, 3 Apr 2020 at 09:13, Yan Zhao <yan.y.zhao@intel.com> wrote:
> >
> > 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 let mmap'd read-only vfio regions be able to generate vmexit for
> > guest write. so, without patch 1, host qemu would crash on guest write to
> > this read-only region. with patch 1, host qemu would drop the writes.
> 
> Just FYI, there seems to be a minor clock or timezone issue with
> however you're sending these emails: the Date header you
> sent reads "Date: Fri, 3 Apr 2020 16:56:57 +0000" but that
> time in the UTC +0000 timezone is (as I write this) still several
> hours in the future. This isn't a big deal but you probably want
> to look into fixing it.
> 
> (Noticed because the wrong-date makes your patches stick to
> the top of the https://patchew.org/QEMU/ list even after other
> patches arrive after them.)
>
thanks for informing of it! will pay attention to it.
Thanks
Yan