[PATCH 0/4] memory: Add memory_region_msync() & make NVMe emulated device generic

[PATCH 0/4] memory: Add memory_region_msync() & make NVMe emulated device generic

[Philippe Mathieu-Daudé]

Let the NVMe emulated device be target-agnostic.

It is not clear if dccvap_writefn() really needs
memory_region_writeback() or could use memory_region_msync().

Philippe Mathieu-Daudé (4):
  memory: Rename memory_region_do_writeback -> memory_region_writeback
  memory: Extract memory_region_msync() from memory_region_writeback()
  hw/block: Let the NVMe emulated device be target-agnostic
  exec: Rename qemu_ram_writeback() as qemu_ram_msync()

 include/exec/memory.h   | 15 +++++++++++++--
 include/exec/ram_addr.h |  4 ++--
 exec.c                  |  2 +-
 hw/block/nvme.c         |  6 ++----
 memory.c                | 12 +++++++++---
 target/arm/helper.c     |  2 +-
 hw/block/Makefile.objs  |  2 +-
 7 files changed, 29 insertions(+), 14 deletions(-)

-- 
2.21.3

[PATCH 1/4] memory: Rename memory_region_do_writeback -> memory_region_writeback

[Philippe Mathieu-Daudé]

We usually use '_do_' for internal functions. Rename
memory_region_do_writeback() as memory_region_writeback().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/exec/memory.h | 4 ++--
 memory.c              | 2 +-
 target/arm/helper.c   | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index e000bd2f97..1b7cfdd5b6 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1474,14 +1474,14 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr);
 void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize,
                               Error **errp);
 /**
- * memory_region_do_writeback: Trigger cache writeback or msync for
+ * memory_region_writeback: Trigger cache writeback or msync for
  * selected address range
  *
  * @mr: the memory region to be updated
  * @addr: the initial address of the range to be written back
  * @size: the size of the range to be written back
  */
-void memory_region_do_writeback(MemoryRegion *mr, hwaddr addr, hwaddr size);
+void memory_region_writeback(MemoryRegion *mr, hwaddr addr, hwaddr size);
 
 /**
  * memory_region_set_log: Turn dirty logging on or off for a region.
diff --git a/memory.c b/memory.c
index 601b749906..aa1f807392 100644
--- a/memory.c
+++ b/memory.c
@@ -2198,7 +2198,7 @@ void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize, Error **errp
 }
 
 
-void memory_region_do_writeback(MemoryRegion *mr, hwaddr addr, hwaddr size)
+void memory_region_writeback(MemoryRegion *mr, hwaddr addr, hwaddr size)
 {
     /*
      * Might be extended case needed to cover
diff --git a/target/arm/helper.c b/target/arm/helper.c
index a94f650795..c4710b7371 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6829,7 +6829,7 @@ static void dccvap_writefn(CPUARMState *env, const ARMCPRegInfo *opaque,
         mr = memory_region_from_host(haddr, &offset);
 
         if (mr) {
-            memory_region_do_writeback(mr, offset, dline_size);
+            memory_region_writeback(mr, offset, dline_size);
         }
     }
 }
-- 
2.21.3

[PATCH 2/4] memory: Extract memory_region_msync() from memory_region_writeback()

[Philippe Mathieu-Daudé]

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/exec/memory.h | 13 ++++++++++++-
 memory.c              | 10 ++++++++--
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 1b7cfdd5b6..3e00cdbbfa 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1473,8 +1473,19 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr);
  */
 void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize,
                               Error **errp);
+
 /**
- * memory_region_writeback: Trigger cache writeback or msync for
+ * memory_region_msync: Synchronize selected address range of
+ * a memory mapped region
+ *
+ * @mr: the memory region to be msync
+ * @addr: the initial address of the range to be sync
+ * @size: the size of the range to be sync
+ */
+void memory_region_msync(MemoryRegion *mr, hwaddr addr, hwaddr size);
+
+/**
+ * memory_region_writeback: Trigger cache writeback for
  * selected address range
  *
  * @mr: the memory region to be updated
diff --git a/memory.c b/memory.c
index aa1f807392..ed26514c89 100644
--- a/memory.c
+++ b/memory.c
@@ -2197,6 +2197,12 @@ void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize, Error **errp
     qemu_ram_resize(mr->ram_block, newsize, errp);
 }
 
+void memory_region_msync(MemoryRegion *mr, hwaddr addr, hwaddr size)
+{
+    if (mr->ram_block) {
+        qemu_ram_writeback(mr->ram_block, addr, size);
+    }
+}
 
 void memory_region_writeback(MemoryRegion *mr, hwaddr addr, hwaddr size)
 {
@@ -2204,8 +2210,8 @@ void memory_region_writeback(MemoryRegion *mr, hwaddr addr, hwaddr size)
      * Might be extended case needed to cover
      * different types of memory regions
      */
-    if (mr->ram_block && mr->dirty_log_mask) {
-        qemu_ram_writeback(mr->ram_block, addr, size);
+    if (mr->dirty_log_mask) {
+        memory_region_msync(mr, addr, size);
     }
 }
 
-- 
2.21.3

[PATCH 3/4] hw/block: Let the NVMe emulated device be target-agnostic

[Philippe Mathieu-Daudé]

Now than the non-target specific memory_region_msync() function
is available, use it to make this device target-agnostic.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/nvme.c        | 6 ++----
 hw/block/Makefile.objs | 2 +-
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 9b453423cf..8a72dce0e6 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -46,8 +46,7 @@
 #include "qapi/visitor.h"
 #include "sysemu/hostmem.h"
 #include "sysemu/block-backend.h"
-#include "exec/ram_addr.h"
-
+#include "exec/memory.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
 #include "qemu/cutils.h"
@@ -1207,8 +1206,7 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
          */
         if (addr == 0xE08 &&
             (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
-            qemu_ram_writeback(n->pmrdev->mr.ram_block,
-                               0, n->pmrdev->size);
+            memory_region_msync(&n->pmrdev->mr, 0, n->pmrdev->size);
         }
         memcpy(&val, ptr + addr, size);
     } else {
diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
index 47960b5f0d..8855c22656 100644
--- a/hw/block/Makefile.objs
+++ b/hw/block/Makefile.objs
@@ -13,6 +13,6 @@ common-obj-$(CONFIG_SH4) += tc58128.o
 
 obj-$(CONFIG_VIRTIO_BLK) += virtio-blk.o
 obj-$(CONFIG_VHOST_USER_BLK) += vhost-user-blk.o
-obj-$(CONFIG_NVME_PCI) += nvme.o
+common-obj-$(CONFIG_NVME_PCI) += nvme.o
 
 obj-y += dataplane/
-- 
2.21.3

[PATCH 4/4] exec: Rename qemu_ram_writeback() as qemu_ram_msync()

[Philippe Mathieu-Daudé]

Rename qemu_ram_writeback() as qemu_ram_msync() to better
match what it does.

Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/exec/ram_addr.h | 4 ++--
 exec.c                  | 2 +-
 memory.c                | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index a94809f89b..84817e19fa 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -136,12 +136,12 @@ void qemu_ram_free(RAMBlock *block);
 
 int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp);
 
-void qemu_ram_writeback(RAMBlock *block, ram_addr_t start, ram_addr_t length);
+void qemu_ram_msync(RAMBlock *block, ram_addr_t start, ram_addr_t length);
 
 /* Clear whole block of mem */
 static inline void qemu_ram_block_writeback(RAMBlock *block)
 {
-    qemu_ram_writeback(block, 0, block->used_length);
+    qemu_ram_msync(block, 0, block->used_length);
 }
 
 #define DIRTY_CLIENTS_ALL     ((1 << DIRTY_MEMORY_NUM) - 1)
diff --git a/exec.c b/exec.c
index 2874bb5088..f5a8cdf370 100644
--- a/exec.c
+++ b/exec.c
@@ -2127,7 +2127,7 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
  * Otherwise no-op.
  * @Note: this is supposed to be a synchronous op.
  */
-void qemu_ram_writeback(RAMBlock *block, ram_addr_t start, ram_addr_t length)
+void qemu_ram_msync(RAMBlock *block, ram_addr_t start, ram_addr_t length)
 {
     /* The requested range should fit in within the block range */
     g_assert((start + length) <= block->used_length);
diff --git a/memory.c b/memory.c
index ed26514c89..352b0a3aa8 100644
--- a/memory.c
+++ b/memory.c
@@ -2200,7 +2200,7 @@ void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize, Error **errp
 void memory_region_msync(MemoryRegion *mr, hwaddr addr, hwaddr size)
 {
     if (mr->ram_block) {
-        qemu_ram_writeback(mr->ram_block, addr, size);
+        qemu_ram_msync(mr->ram_block, addr, size);
     }
 }
 
-- 
2.21.3

Re: [PATCH 0/4] memory: Add memory_region_msync() & make NVMe emulated device generic

[Paolo Bonzini]

On 08/05/20 08:24, Philippe Mathieu-Daudé wrote:
> It is not clear if dccvap_writefn() really needs
> memory_region_writeback() or could use memory_region_msync().

Indeed, I don't understand the code and why it matters that
mr->dirty_log_mask is nonzero.

mr->dirty_log_mask tells if dirty tracking has been enabled, not if the
page is dirty.  It would always be true during live migration and when
running on TCG, but otherwise it would always be false.

Beata, can you explain what you had in mind? :)

Paolo

Re: [PATCH 0/4] memory: Add memory_region_msync() & make NVMe emulated device generic

[Beata Michalska]

On Fri, 8 May 2020 at 07:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 08/05/20 08:24, Philippe Mathieu-Daudé wrote:
> > It is not clear if dccvap_writefn() really needs
> > memory_region_writeback() or could use memory_region_msync().
>
> Indeed, I don't understand the code and why it matters that
> mr->dirty_log_mask is nonzero.
>
> mr->dirty_log_mask tells if dirty tracking has been enabled, not if the
> page is dirty.  It would always be true during live migration and when
> running on TCG, but otherwise it would always be false.
>
> Beata, can you explain what you had in mind? :)
>
It has been a while ... , but the intention there was to skip the sync
if there is nothing to be synced in the first place - so for performance
reasons. I honestly do not recall why I went for the dirty_log_mask,
as that seems not  to be the right choice .

BR
Beata
> Paolo
>

Re: [PATCH 0/4] memory: Add memory_region_msync() & make NVMe emulated device generic

[Paolo Bonzini]

On 08/05/20 17:20, Beata Michalska wrote:
>>
>> mr->dirty_log_mask tells if dirty tracking has been enabled, not if the
>> page is dirty.  It would always be true during live migration and when
>> running on TCG, but otherwise it would always be false.
>>
>> Beata, can you explain what you had in mind? :)
>>
> It has been a while ... , but the intention there was to skip the sync
> if there is nothing to be synced in the first place - so for performance
> reasons. I honestly do not recall why I went for the dirty_log_mask,
> as that seems not  to be the right choice .

You probably wanted to look at the dirty bitmap, but you would have to
define a new bitmap rather than looking at dirty_log_mask.

But that's cool, because it means that we can just remove it!  Thanks,

Paolo

Re: [PATCH 0/4] memory: Add memory_region_msync() & make NVMe emulated device generic

[Philippe Mathieu-Daudé]

On 5/8/20 5:33 PM, Paolo Bonzini wrote:
> On 08/05/20 17:20, Beata Michalska wrote:
>>>
>>> mr->dirty_log_mask tells if dirty tracking has been enabled, not if the
>>> page is dirty.  It would always be true during live migration and when
>>> running on TCG, but otherwise it would always be false.
>>>
>>> Beata, can you explain what you had in mind? :)
>>>
>> It has been a while ... , but the intention there was to skip the sync
>> if there is nothing to be synced in the first place - so for performance
>> reasons. I honestly do not recall why I went for the dirty_log_mask,
>> as that seems not  to be the right choice .
> 
> You probably wanted to look at the dirty bitmap, but you would have to
> define a new bitmap rather than looking at dirty_log_mask.
> 
> But that's cool, because it means that we can just remove it!  Thanks,

So I understand I can simply rename memory_region_writeback -> 
memory_region_msync in patch #2.

Re: [PATCH 0/4] memory: Add memory_region_msync() & make NVMe emulated device generic

[Philippe Mathieu-Daudé]

On Fri, May 8, 2020 at 5:34 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 5/8/20 5:33 PM, Paolo Bonzini wrote:
> > On 08/05/20 17:20, Beata Michalska wrote:
> >>>
> >>> mr->dirty_log_mask tells if dirty tracking has been enabled, not if the
> >>> page is dirty.  It would always be true during live migration and when
> >>> running on TCG, but otherwise it would always be false.
> >>>
> >>> Beata, can you explain what you had in mind? :)
> >>>
> >> It has been a while ... , but the intention there was to skip the sync
> >> if there is nothing to be synced in the first place - so for performance
> >> reasons. I honestly do not recall why I went for the dirty_log_mask,
> >> as that seems not  to be the right choice .
> >
> > You probably wanted to look at the dirty bitmap, but you would have to
> > define a new bitmap rather than looking at dirty_log_mask.
> >
> > But that's cool, because it means that we can just remove it!  Thanks,
>
> So I understand I can simply rename memory_region_writeback ->
> memory_region_msync in patch #2.

(removing the mr->dirty_log_mask).

Re: [PATCH 0/4] memory: Add memory_region_msync() & make NVMe emulated device generic

[Paolo Bonzini]

On 08/05/20 17:35, Philippe Mathieu-Daudé wrote:
>> So I understand I can simply rename memory_region_writeback ->
>> memory_region_msync in patch #2.
> 
> (removing the mr->dirty_log_mask).
> 

Yes.

Paolo

Re: [PATCH 1/4] memory: Rename memory_region_do_writeback -> memory_region_writeback

[Richard Henderson]

On 5/7/20 11:24 PM, Philippe Mathieu-Daudé wrote:
> We usually use '_do_' for internal functions. Rename
> memory_region_do_writeback() as memory_region_writeback().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/exec/memory.h | 4 ++--
>  memory.c              | 2 +-
>  target/arm/helper.c   | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 3/4] hw/block: Let the NVMe emulated device be target-agnostic

[Richard Henderson]

On 5/7/20 11:24 PM, Philippe Mathieu-Daudé wrote:
> Now than the non-target specific memory_region_msync() function
> is available, use it to make this device target-agnostic.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/nvme.c        | 6 ++----
>  hw/block/Makefile.objs | 2 +-
>  2 files changed, 3 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 4/4] exec: Rename qemu_ram_writeback() as qemu_ram_msync()

[Richard Henderson]

On 5/7/20 11:24 PM, Philippe Mathieu-Daudé wrote:
> Rename qemu_ram_writeback() as qemu_ram_msync() to better
> match what it does.
> 
> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/exec/ram_addr.h | 4 ++--
>  exec.c                  | 2 +-
>  memory.c                | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 0/4] memory: Add memory_region_msync() & make NVMe emulated device generic

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 974 bytes --]

On Fri, May 08, 2020 at 08:24:52AM +0200, Philippe Mathieu-Daudé wrote:
> Let the NVMe emulated device be target-agnostic.
> 
> It is not clear if dccvap_writefn() really needs
> memory_region_writeback() or could use memory_region_msync().
> 
> Philippe Mathieu-Daudé (4):
>   memory: Rename memory_region_do_writeback -> memory_region_writeback
>   memory: Extract memory_region_msync() from memory_region_writeback()
>   hw/block: Let the NVMe emulated device be target-agnostic
>   exec: Rename qemu_ram_writeback() as qemu_ram_msync()
> 
>  include/exec/memory.h   | 15 +++++++++++++--
>  include/exec/ram_addr.h |  4 ++--
>  exec.c                  |  2 +-
>  hw/block/nvme.c         |  6 ++----
>  memory.c                | 12 +++++++++---
>  target/arm/helper.c     |  2 +-
>  hw/block/Makefile.objs  |  2 +-
>  7 files changed, 29 insertions(+), 14 deletions(-)
> 
> -- 
> 2.21.3
> 
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/4] memory: Add memory_region_msync() & make NVMe emulated device generic

[Paolo Bonzini]

[-- Attachment #1.1: Type: text/plain, Size: 1096 bytes --]

On 21/05/20 14:32, Stefan Hajnoczi wrote:
> On Fri, May 08, 2020 at 08:24:52AM +0200, Philippe Mathieu-Daudé wrote:
>> Let the NVMe emulated device be target-agnostic.
>>
>> It is not clear if dccvap_writefn() really needs
>> memory_region_writeback() or could use memory_region_msync().
>>
>> Philippe Mathieu-Daudé (4):
>>   memory: Rename memory_region_do_writeback -> memory_region_writeback
>>   memory: Extract memory_region_msync() from memory_region_writeback()
>>   hw/block: Let the NVMe emulated device be target-agnostic
>>   exec: Rename qemu_ram_writeback() as qemu_ram_msync()
>>
>>  include/exec/memory.h   | 15 +++++++++++++--
>>  include/exec/ram_addr.h |  4 ++--
>>  exec.c                  |  2 +-
>>  hw/block/nvme.c         |  6 ++----
>>  memory.c                | 12 +++++++++---
>>  target/arm/helper.c     |  2 +-
>>  hw/block/Makefile.objs  |  2 +-
>>  7 files changed, 29 insertions(+), 14 deletions(-)
>>
>> -- 
>> 2.21.3
>>
>>
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/4] memory: Add memory_region_msync() & make NVMe emulated device generic

[Philippe Mathieu-Daudé]

On 5/21/20 2:46 PM, Paolo Bonzini wrote:
> On 21/05/20 14:32, Stefan Hajnoczi wrote:
>> On Fri, May 08, 2020 at 08:24:52AM +0200, Philippe Mathieu-Daudé wrote:
>>> Let the NVMe emulated device be target-agnostic.
>>>
>>> It is not clear if dccvap_writefn() really needs
>>> memory_region_writeback() or could use memory_region_msync().
>>>
>>> Philippe Mathieu-Daudé (4):
>>>    memory: Rename memory_region_do_writeback -> memory_region_writeback
>>>    memory: Extract memory_region_msync() from memory_region_writeback()
>>>    hw/block: Let the NVMe emulated device be target-agnostic
>>>    exec: Rename qemu_ram_writeback() as qemu_ram_msync()
>>>
>>>   include/exec/memory.h   | 15 +++++++++++++--
>>>   include/exec/ram_addr.h |  4 ++--
>>>   exec.c                  |  2 +-
>>>   hw/block/nvme.c         |  6 ++----
>>>   memory.c                | 12 +++++++++---
>>>   target/arm/helper.c     |  2 +-
>>>   hw/block/Makefile.objs  |  2 +-
>>>   7 files changed, 29 insertions(+), 14 deletions(-)
>>>
>>> -- 
>>> 2.21.3
>>>
>>>
>>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>
> 
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> 

Thanks both.

Stefan, do you mind taking the series via your block tree?

Thanks,

Phil.

Re: [PATCH 0/4] memory: Add memory_region_msync() & make NVMe emulated device generic

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]

On Fri, May 08, 2020 at 08:24:52AM +0200, Philippe Mathieu-Daudé wrote:
> Let the NVMe emulated device be target-agnostic.
> 
> It is not clear if dccvap_writefn() really needs
> memory_region_writeback() or could use memory_region_msync().
> 
> Philippe Mathieu-Daudé (4):
>   memory: Rename memory_region_do_writeback -> memory_region_writeback
>   memory: Extract memory_region_msync() from memory_region_writeback()
>   hw/block: Let the NVMe emulated device be target-agnostic
>   exec: Rename qemu_ram_writeback() as qemu_ram_msync()
> 
>  include/exec/memory.h   | 15 +++++++++++++--
>  include/exec/ram_addr.h |  4 ++--
>  exec.c                  |  2 +-
>  hw/block/nvme.c         |  6 ++----
>  memory.c                | 12 +++++++++---
>  target/arm/helper.c     |  2 +-
>  hw/block/Makefile.objs  |  2 +-
>  7 files changed, 29 insertions(+), 14 deletions(-)

Thanks, applied to my block tree with Paolo's Acked-by:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]