[PATCH 0/6] block/nvme: Align block pages queue to host page size

[PATCH 0/6] block/nvme: Align block pages queue to host page size

[Philippe Mathieu-Daudé]

The most important patch of this series is 'Align block pages
queue to host page size', it start fixing an issue on PowerPC
hosts. It is not sufficient, but the rest is VFIO specific,
so I prefer to send it separately.

The other patches are documentation I was writing down while
looking at the code.

The last patch is about the emulated device and writeback.
Marked as RFC as I don't understand writeback/dirty_log_mask yet.

Philippe Mathieu-Daudé (6):
  qemu/osdep: Document qemu_memalign() and friends
  qemu/bitmap: Document bitmap_new() returned pointer
  sysemu/block-backend: Document blk_read()/blk_pwrite()
  block/block: Document BlockSizes fields
  block/nvme: Align block pages queue to host page size
  hw/block/nvme: Make device target agnostic

 include/block/block.h          |  8 ++++++--
 include/qemu/bitmap.h          |  2 ++
 include/qemu/osdep.h           |  3 +++
 include/sysemu/block-backend.h | 26 ++++++++++++++++++++++++++
 block/nvme.c                   |  2 +-
 hw/block/nvme.c                |  4 +---
 hw/block/Makefile.objs         |  2 +-
 7 files changed, 40 insertions(+), 7 deletions(-)

-- 
2.21.3

[PATCH 1/6] qemu/osdep: Document qemu_memalign() and friends

[Philippe Mathieu-Daudé]

Document allocator functions that require a specific
de-allocator call.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/block/block.h          | 4 ++++
 include/qemu/osdep.h           | 3 +++
 include/sysemu/block-backend.h | 2 ++
 3 files changed, 9 insertions(+)

diff --git a/include/block/block.h b/include/block/block.h
index 8b62429aa4..c57fdecf9a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -526,9 +526,13 @@ void bdrv_img_create(const char *filename, const char *fmt,
 size_t bdrv_min_mem_align(BlockDriverState *bs);
 /* Returns optimal alignment in bytes for bounce buffer */
 size_t bdrv_opt_mem_align(BlockDriverState *bs);
+/* callers must free the returned pointer with qemu_vfree() */
 void *qemu_blockalign(BlockDriverState *bs, size_t size);
+/* callers must free the returned pointer with qemu_vfree() */
 void *qemu_blockalign0(BlockDriverState *bs, size_t size);
+/* callers must free the returned pointer with qemu_vfree() */
 void *qemu_try_blockalign(BlockDriverState *bs, size_t size);
+/* callers must free the returned pointer with qemu_vfree() */
 void *qemu_try_blockalign0(BlockDriverState *bs, size_t size);
 bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov);
 
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 20f5c5f197..778c459c22 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -294,8 +294,11 @@ extern int daemon(int, int);
 #endif
 
 int qemu_daemon(int nochdir, int noclose);
+/* callers must free the returned pointer with qemu_vfree() */
 void *qemu_try_memalign(size_t alignment, size_t size);
+/* callers must free the returned pointer with qemu_vfree() */
 void *qemu_memalign(size_t alignment, size_t size);
+/* callers must free the returned pointer with qemu_anon_ram_free() */
 void *qemu_anon_ram_alloc(size_t size, uint64_t *align, bool shared);
 void qemu_vfree(void *ptr);
 void qemu_anon_ram_free(void *ptr, size_t size);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 34de7faa81..f2dcf63ae3 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -203,7 +203,9 @@ uint32_t blk_get_request_alignment(BlockBackend *blk);
 uint32_t blk_get_max_transfer(BlockBackend *blk);
 int blk_get_max_iov(BlockBackend *blk);
 void blk_set_guest_block_size(BlockBackend *blk, int align);
+/* callers must free the returned pointer with qemu_vfree() */
 void *blk_try_blockalign(BlockBackend *blk, size_t size);
+/* callers must free the returned pointer with qemu_vfree() */
 void *blk_blockalign(BlockBackend *blk, size_t size);
 bool blk_op_is_blocked(BlockBackend *blk, BlockOpType op, Error **errp);
 void blk_op_unblock(BlockBackend *blk, BlockOpType op, Error *reason);
-- 
2.21.3

[PATCH 2/6] qemu/bitmap: Document bitmap_new() returned pointer

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/bitmap.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
index 82a1d2f41f..0b390ff576 100644
--- a/include/qemu/bitmap.h
+++ b/include/qemu/bitmap.h
@@ -90,12 +90,14 @@ int slow_bitmap_intersects(const unsigned long *bitmap1,
                            const unsigned long *bitmap2, long bits);
 long slow_bitmap_count_one(const unsigned long *bitmap, long nbits);
 
+/* callers must free the returned pointer with g_free() */
 static inline unsigned long *bitmap_try_new(long nbits)
 {
     long len = BITS_TO_LONGS(nbits) * sizeof(unsigned long);
     return g_try_malloc0(len);
 }
 
+/* callers must free the returned pointer with g_free() */
 static inline unsigned long *bitmap_new(long nbits)
 {
     unsigned long *ptr = bitmap_try_new(nbits);
-- 
2.21.3

[PATCH 3/6] sysemu/block-backend: Document blk_read()/blk_pwrite()

[Philippe Mathieu-Daudé]

The blk_read()/blk_pwrite() return value is not obvious,
document it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/sysemu/block-backend.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index f2dcf63ae3..823b8e94a7 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -153,7 +153,31 @@ BlockAIOCB *blk_aio_pwrite_zeroes(BlockBackend *blk, int64_t offset,
                                   int bytes, BdrvRequestFlags flags,
                                   BlockCompletionFunc *cb, void *opaque);
 int blk_make_zero(BlockBackend *blk, BdrvRequestFlags flags);
+
+/**
+ * blk_pread:
+ *
+ * @blk - the block backend where the buffer content is going to be read from
+ * @offset: position in bytes to read at
+ * @buf: the data buffer
+ * @bytes: number of bytes to read
+ *
+ * Returns: the number of bytes read on success, or a negative errno otherwise.
+ */
 int blk_pread(BlockBackend *blk, int64_t offset, void *buf, int bytes);
+
+/**
+ * blk_pwrite:
+ *
+ * @blk - the block backend where the buffer content is going to be written to
+ * @offset: position in bytes to write at
+ * @buf: the data buffer
+ * @bytes: number of bytes of @buf to write
+ * @flags: request flags
+ *
+ * Returns: the number of bytes consumed on success,
+ *          or a negative errno otherwise.
+ */
 int blk_pwrite(BlockBackend *blk, int64_t offset, const void *buf, int bytes,
                BdrvRequestFlags flags);
 int64_t blk_getlength(BlockBackend *blk);
-- 
2.21.3

[PATCH 4/6] block/block: Document BlockSizes fields

[Philippe Mathieu-Daudé]

As it is not obvious for a block neophyte what means
the 'log' value, document it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/block/block.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index c57fdecf9a..94517c92b6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -86,8 +86,8 @@ typedef enum {
 } BdrvRequestFlags;
 
 typedef struct BlockSizes {
-    uint32_t phys;
-    uint32_t log;
+    uint32_t phys;  /* physical block size */
+    uint32_t log;   /* logical block size */
 } BlockSizes;
 
 typedef struct HDGeometry {
-- 
2.21.3

[PATCH 5/6] block/nvme: Align block pages queue to host page size

[Philippe Mathieu-Daudé]

In nvme_create_queue_pair() we create a page list using
qemu_blockalign(), then map it with qemu_vfio_dma_map():

  q->prp_list_pages = qemu_blockalign0(bs, s->page_size * NVME_QUEUE_SIZE);
  r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages,
                        s->page_size * NVME_QUEUE_SIZE, ...);

With:

  s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF)));

The qemu_vfio_dma_map() documentation says "The caller need
to make sure the area is aligned to page size". While we use
multiple s->page_size as alignment, it might be not sufficient
on some hosts. Use the qemu_real_host_page_size value to be
sure the host alignment is respected.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Cc: Cédric Le Goater <clg@kaod.org>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Laurent Vivier <lvivier@redhat.com>
---
 block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/nvme.c b/block/nvme.c
index 7b7c0cc5d6..bde0d28b39 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -627,7 +627,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
 
     s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF)));
     s->doorbell_scale = (4 << (((cap >> 32) & 0xF))) / sizeof(uint32_t);
-    bs->bl.opt_mem_alignment = s->page_size;
+    bs->bl.opt_mem_alignment = MAX(qemu_real_host_page_size, s->page_size);
     timeout_ms = MIN(500 * ((cap >> 24) & 0xFF), 30000);
 
     /* Reset device to get a clean state. */
-- 
2.21.3

[RFC PATCH 6/6] hw/block/nvme: Make device target agnostic

[Philippe Mathieu-Daudé]

The NVMe device should not use target specific API.
Use memory_region_do_writeback() (which was introduced
in commit 61c490e25e0, after the NVMe emulated device
was added) to replace qemu_ram_writeback().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
RFC because I have no clue how dirty_log_mask works.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Beata Michalska <beata.michalska@linaro.org>
---
 hw/block/nvme.c        | 4 +---
 hw/block/Makefile.objs | 2 +-
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 9b453423cf..9b0ac0ea2a 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -46,7 +46,6 @@
 #include "qapi/visitor.h"
 #include "sysemu/hostmem.h"
 #include "sysemu/block-backend.h"
-#include "exec/ram_addr.h"
 
 #include "qemu/log.h"
 #include "qemu/module.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_do_writeback(&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

Re: [RFC PATCH 6/6] hw/block/nvme: Make device target agnostic

[Philippe Mathieu-Daudé]

+Keith new email

On 5/4/20 11:46 AM, Philippe Mathieu-Daudé wrote:
> The NVMe device should not use target specific API.
> Use memory_region_do_writeback() (which was introduced
> in commit 61c490e25e0, after the NVMe emulated device
> was added) to replace qemu_ram_writeback().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> RFC because I have no clue how dirty_log_mask works.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Beata Michalska <beata.michalska@linaro.org>
> ---
>   hw/block/nvme.c        | 4 +---
>   hw/block/Makefile.objs | 2 +-
>   2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 9b453423cf..9b0ac0ea2a 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -46,7 +46,6 @@
>   #include "qapi/visitor.h"
>   #include "sysemu/hostmem.h"
>   #include "sysemu/block-backend.h"
> -#include "exec/ram_addr.h"
>   
>   #include "qemu/log.h"
>   #include "qemu/module.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_do_writeback(&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/
> 

Re: [PATCH 5/6] block/nvme: Align block pages queue to host page size

[David Gibson]

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

On Mon, May 04, 2020 at 11:46:40AM +0200, Philippe Mathieu-Daudé wrote:
> In nvme_create_queue_pair() we create a page list using
> qemu_blockalign(), then map it with qemu_vfio_dma_map():
> 
>   q->prp_list_pages = qemu_blockalign0(bs, s->page_size * NVME_QUEUE_SIZE);
>   r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages,
>                         s->page_size * NVME_QUEUE_SIZE, ...);
> 
> With:
> 
>   s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF)));
> 
> The qemu_vfio_dma_map() documentation says "The caller need
> to make sure the area is aligned to page size". While we use
> multiple s->page_size as alignment, it might be not sufficient
> on some hosts. Use the qemu_real_host_page_size value to be
> sure the host alignment is respected.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
> Cc: Cédric Le Goater <clg@kaod.org>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Laurent Vivier <lvivier@redhat.com>
> ---
>  block/nvme.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 7b7c0cc5d6..bde0d28b39 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -627,7 +627,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>  
>      s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF)));
>      s->doorbell_scale = (4 << (((cap >> 32) & 0xF))) / sizeof(uint32_t);
> -    bs->bl.opt_mem_alignment = s->page_size;
> +    bs->bl.opt_mem_alignment = MAX(qemu_real_host_page_size, s->page_size);
>      timeout_ms = MIN(500 * ((cap >> 24) & 0xFF), 30000);
>  
>      /* Reset device to get a clean state. */

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

Re: [PATCH 5/6] block/nvme: Align block pages queue to host page size

[Laurent Vivier]

On 04/05/2020 11:46, Philippe Mathieu-Daudé wrote:
> In nvme_create_queue_pair() we create a page list using
> qemu_blockalign(), then map it with qemu_vfio_dma_map():
> 
>   q->prp_list_pages = qemu_blockalign0(bs, s->page_size * NVME_QUEUE_SIZE);
>   r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages,
>                         s->page_size * NVME_QUEUE_SIZE, ...);
> 
> With:
> 
>   s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF)));
> 
> The qemu_vfio_dma_map() documentation says "The caller need
> to make sure the area is aligned to page size". While we use
> multiple s->page_size as alignment, it might be not sufficient
> on some hosts. Use the qemu_real_host_page_size value to be
> sure the host alignment is respected.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Cc: Cédric Le Goater <clg@kaod.org>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Laurent Vivier <lvivier@redhat.com>
> ---
>  block/nvme.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 7b7c0cc5d6..bde0d28b39 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -627,7 +627,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>  
>      s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF)));
>      s->doorbell_scale = (4 << (((cap >> 32) & 0xF))) / sizeof(uint32_t);
> -    bs->bl.opt_mem_alignment = s->page_size;
> +    bs->bl.opt_mem_alignment = MAX(qemu_real_host_page_size, s->page_size);

Why don't you replace directly the "4096" in s->page_size by
qemu_real_host_page_size?

I think this was the purpose of MAX(4096, ...) to align to the host page
size.

Thanks,
Laurnt

Re: [PATCH 5/6] block/nvme: Align block pages queue to host page size

[Laurent Vivier]

On 05/05/2020 10:00, Laurent Vivier wrote:
> On 04/05/2020 11:46, Philippe Mathieu-Daudé wrote:
>> In nvme_create_queue_pair() we create a page list using
>> qemu_blockalign(), then map it with qemu_vfio_dma_map():
>>
>>   q->prp_list_pages = qemu_blockalign0(bs, s->page_size * NVME_QUEUE_SIZE);
>>   r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages,
>>                         s->page_size * NVME_QUEUE_SIZE, ...);
>>
>> With:
>>
>>   s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF)));
>>
>> The qemu_vfio_dma_map() documentation says "The caller need
>> to make sure the area is aligned to page size". While we use
>> multiple s->page_size as alignment, it might be not sufficient
>> on some hosts. Use the qemu_real_host_page_size value to be
>> sure the host alignment is respected.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> Cc: Cédric Le Goater <clg@kaod.org>
>> Cc: David Gibson <david@gibson.dropbear.id.au>
>> Cc: Laurent Vivier <lvivier@redhat.com>
>> ---
>>  block/nvme.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/nvme.c b/block/nvme.c
>> index 7b7c0cc5d6..bde0d28b39 100644
>> --- a/block/nvme.c
>> +++ b/block/nvme.c
>> @@ -627,7 +627,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>>  
>>      s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF)));
>>      s->doorbell_scale = (4 << (((cap >> 32) & 0xF))) / sizeof(uint32_t);
>> -    bs->bl.opt_mem_alignment = s->page_size;
>> +    bs->bl.opt_mem_alignment = MAX(qemu_real_host_page_size, s->page_size);
> 
> Why don't you replace directly the "4096" in s->page_size by
> qemu_real_host_page_size?
> 
> I think this was the purpose of MAX(4096, ...) to align to the host page
> size.

oh, I see, page_size is used for a lot of things than can be broken, and
this is what is done in bdrv_opt_mem_align() for instance, 4096 is the
sector size not the host page size.

BTW, you need the same change as in nvme_init() in
nvme_refresh_limits(), I think.

Thanks,
Laurent

Re: [PATCH 5/6] block/nvme: Align block pages queue to host page size

[Philippe Mathieu-Daudé]

On 5/5/20 10:23 AM, Laurent Vivier wrote:
> On 05/05/2020 10:00, Laurent Vivier wrote:
>> On 04/05/2020 11:46, Philippe Mathieu-Daudé wrote:
>>> In nvme_create_queue_pair() we create a page list using
>>> qemu_blockalign(), then map it with qemu_vfio_dma_map():
>>>
>>>    q->prp_list_pages = qemu_blockalign0(bs, s->page_size * NVME_QUEUE_SIZE);
>>>    r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages,
>>>                          s->page_size * NVME_QUEUE_SIZE, ...);
>>>
>>> With:
>>>
>>>    s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF)));
>>>
>>> The qemu_vfio_dma_map() documentation says "The caller need
>>> to make sure the area is aligned to page size". While we use
>>> multiple s->page_size as alignment, it might be not sufficient
>>> on some hosts. Use the qemu_real_host_page_size value to be
>>> sure the host alignment is respected.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>> Cc: Cédric Le Goater <clg@kaod.org>
>>> Cc: David Gibson <david@gibson.dropbear.id.au>
>>> Cc: Laurent Vivier <lvivier@redhat.com>
>>> ---
>>>   block/nvme.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/block/nvme.c b/block/nvme.c
>>> index 7b7c0cc5d6..bde0d28b39 100644
>>> --- a/block/nvme.c
>>> +++ b/block/nvme.c
>>> @@ -627,7 +627,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>>>   
>>>       s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF)));
>>>       s->doorbell_scale = (4 << (((cap >> 32) & 0xF))) / sizeof(uint32_t);
>>> -    bs->bl.opt_mem_alignment = s->page_size;
>>> +    bs->bl.opt_mem_alignment = MAX(qemu_real_host_page_size, s->page_size);
>>
>> Why don't you replace directly the "4096" in s->page_size by
>> qemu_real_host_page_size?
>>
>> I think this was the purpose of MAX(4096, ...) to align to the host page
>> size.
> 
> oh, I see, page_size is used for a lot of things than can be broken, and
> this is what is done in bdrv_opt_mem_align() for instance, 4096 is the
> sector size not the host page size.
> 
> BTW, you need the same change as in nvme_init() in
> nvme_refresh_limits(), I think.

nvme_init() seems OK, but you are right, I missed 
nvme_refresh_limits()... Thanks!

> 
> Thanks,
> Laurent
> 
> 

Re: [PATCH 5/6] block/nvme: Align block pages queue to host page size

[Philippe Mathieu-Daudé]

On 5/4/20 11:46 AM, Philippe Mathieu-Daudé wrote:
> In nvme_create_queue_pair() we create a page list using
> qemu_blockalign(), then map it with qemu_vfio_dma_map():
> 
>    q->prp_list_pages = qemu_blockalign0(bs, s->page_size * NVME_QUEUE_SIZE);
>    r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages,
>                          s->page_size * NVME_QUEUE_SIZE, ...);
> 
> With:
> 
>    s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF)));
> 
> The qemu_vfio_dma_map() documentation says "The caller need
> to make sure the area is aligned to page size". While we use
> multiple s->page_size as alignment, it might be not sufficient
> on some hosts. Use the qemu_real_host_page_size value to be
> sure the host alignment is respected.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Cc: Cédric Le Goater <clg@kaod.org>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Laurent Vivier <lvivier@redhat.com>
> ---
>   block/nvme.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 7b7c0cc5d6..bde0d28b39 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -627,7 +627,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>   
>       s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF)));
>       s->doorbell_scale = (4 << (((cap >> 32) & 0xF))) / sizeof(uint32_t);
> -    bs->bl.opt_mem_alignment = s->page_size;
> +    bs->bl.opt_mem_alignment = MAX(qemu_real_host_page_size, s->page_size);
>       timeout_ms = MIN(500 * ((cap >> 24) & 0xFF), 30000);
>   
>       /* Reset device to get a clean state. */
> 

Please disregard this patch as it is incomplete.

Re: [PATCH 3/6] sysemu/block-backend: Document blk_read()/blk_pwrite()

[Stefan Hajnoczi]

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

On Mon, May 04, 2020 at 11:46:38AM +0200, Philippe Mathieu-Daudé wrote:
> +
> +/**
> + * blk_pread:
> + *
> + * @blk - the block backend where the buffer content is going to be read from

s/ - /: /

> +
> +/**
> + * blk_pwrite:
> + *
> + * @blk - the block backend where the buffer content is going to be written to

s/ - /: /

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

Re: [PATCH 1/6] qemu/osdep: Document qemu_memalign() and friends

[Stefan Hajnoczi]

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

On Mon, May 04, 2020 at 11:46:36AM +0200, Philippe Mathieu-Daudé wrote:
> Document allocator functions that require a specific
> de-allocator call.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/block/block.h          | 4 ++++
>  include/qemu/osdep.h           | 3 +++
>  include/sysemu/block-backend.h | 2 ++
>  3 files changed, 9 insertions(+)

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

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

Re: [PATCH 2/6] qemu/bitmap: Document bitmap_new() returned pointer

[Stefan Hajnoczi]

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

On Mon, May 04, 2020 at 11:46:37AM +0200, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/qemu/bitmap.h | 2 ++
>  1 file changed, 2 insertions(+)

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

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

Re: [PATCH 4/6] block/block: Document BlockSizes fields

[Stefan Hajnoczi]

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

On Mon, May 04, 2020 at 11:46:39AM +0200, Philippe Mathieu-Daudé wrote:
> As it is not obvious for a block neophyte what means
> the 'log' value, document it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/block/block.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

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

Re: [RFC PATCH 6/6] hw/block/nvme: Make device target agnostic

[Stefan Hajnoczi]

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

On Mon, May 04, 2020 at 05:36:22PM +0200, Philippe Mathieu-Daudé wrote:
> +Keith new email
> 
> On 5/4/20 11:46 AM, Philippe Mathieu-Daudé wrote:
> > The NVMe device should not use target specific API.
> > Use memory_region_do_writeback() (which was introduced
> > in commit 61c490e25e0, after the NVMe emulated device
> > was added) to replace qemu_ram_writeback().
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> > RFC because I have no clue how dirty_log_mask works.
> > 
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Beata Michalska <beata.michalska@linaro.org>
> > ---
> >   hw/block/nvme.c        | 4 +---
> >   hw/block/Makefile.objs | 2 +-
> >   2 files changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 9b453423cf..9b0ac0ea2a 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -46,7 +46,6 @@
> >   #include "qapi/visitor.h"
> >   #include "sysemu/hostmem.h"
> >   #include "sysemu/block-backend.h"
> > -#include "exec/ram_addr.h"
> >   #include "qemu/log.h"
> >   #include "qemu/module.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_do_writeback(&n->pmrdev->mr, 0, n->pmrdev->size);

qemu_ram_write() is being called because we need to msync or persist
pmem here.

The memory_region_do_writeback() API is not equivalent, its purpose is
for dirty write logging (which we don't care about here because the
writes themselves will already have been logged when the guest performed
them).

I think qemu_ram_writeback() should just be made common so that this
code isn't target-specific. Maybe it should be renamed to
qemu_ram_msync() to avoid confusion with dirty write APIs.

Stefan

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

Re: [RFC PATCH 6/6] hw/block/nvme: Make device target agnostic

[Paolo Bonzini]

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

On 07/05/20 12:04, Stefan Hajnoczi wrote:
>>>               (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
>>> -            qemu_ram_writeback(n->pmrdev->mr.ram_block,
>>> -                               0, n->pmrdev->size);
>>> +            memory_region_do_writeback(&n->pmrdev->mr, 0, n->pmrdev->size);
> qemu_ram_write() is being called because we need to msync or persist
> pmem here.
> 
> The memory_region_do_writeback() API is not equivalent, its purpose is
> for dirty write logging (which we don't care about here because the
> writes themselves will already have been logged when the guest performed
> them).
> 
> I think qemu_ram_writeback() should just be made common so that this
> code isn't target-specific. Maybe it should be renamed to
> qemu_ram_msync() to avoid confusion with dirty write APIs.

Yes, we can add qemu_ram_msync and memory_region_msync.

Paolo


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