[PATCH v2 0/4] hw/block/nvme: Fix I/O BAR structure

[PATCH v2 0/4] hw/block/nvme: Fix I/O BAR structure

[Philippe Mathieu-Daudé]

Improvements for the I/O BAR structure:
- correct pmrmsc register size (Klaus)
- pack structures
- align to 4KB

Philippe Mathieu-Daudé (4):
  hw/block/nvme: Update specification URL
  hw/block/nvme: Use QEMU_PACKED on hardware/packet structures
  hw/block/nvme: Fix pmrmsc register size
  hw/block/nvme: Align I/O BAR to 4 KiB

 include/block/nvme.h | 42 ++++++++++++++++++++++--------------------
 hw/block/nvme.c      |  7 +++----
 2 files changed, 25 insertions(+), 24 deletions(-)

-- 
2.21.3

[PATCH v2 1/4] hw/block/nvme: Update specification URL

[Philippe Mathieu-Daudé]

At some point the URL changed, update it to avoid other
developers to search for it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 1aee042d4c..6628d0a4ba 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -11,7 +11,7 @@
 /**
  * Reference Specs: http://www.nvmexpress.org, 1.2, 1.1, 1.0e
  *
- *  http://www.nvmexpress.org/resources/
+ *  https://nvmexpress.org/developers/nvme-specification/
  */
 
 /**
-- 
2.21.3

[PATCH v2 2/4] hw/block/nvme: Use QEMU_PACKED on hardware/packet structures

[Philippe Mathieu-Daudé]

These structures either describe hardware registers, or
commands ('packets') to send to the hardware. To forbid
the compiler to optimize and change fields alignment,
mark the structures as packed.

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

diff --git a/include/block/nvme.h b/include/block/nvme.h
index 1720ee1d51..71c5681912 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -1,7 +1,7 @@
 #ifndef BLOCK_NVME_H
 #define BLOCK_NVME_H
 
-typedef struct NvmeBar {
+typedef struct QEMU_PACKED NvmeBar {
     uint64_t    cap;
     uint32_t    vs;
     uint32_t    intms;
@@ -377,7 +377,7 @@ enum NvmePmrmscMask {
 #define NVME_PMRMSC_SET_CBA(pmrmsc, val)   \
     (pmrmsc |= (uint64_t)(val & PMRMSC_CBA_MASK) << PMRMSC_CBA_SHIFT)
 
-typedef struct NvmeCmd {
+typedef struct QEMU_PACKED NvmeCmd {
     uint8_t     opcode;
     uint8_t     fuse;
     uint16_t    cid;
@@ -422,7 +422,7 @@ enum NvmeIoCommands {
     NVME_CMD_DSM                = 0x09,
 };
 
-typedef struct NvmeDeleteQ {
+typedef struct QEMU_PACKED NvmeDeleteQ {
     uint8_t     opcode;
     uint8_t     flags;
     uint16_t    cid;
@@ -432,7 +432,7 @@ typedef struct NvmeDeleteQ {
     uint32_t    rsvd11[5];
 } NvmeDeleteQ;
 
-typedef struct NvmeCreateCq {
+typedef struct QEMU_PACKED NvmeCreateCq {
     uint8_t     opcode;
     uint8_t     flags;
     uint16_t    cid;
@@ -449,7 +449,7 @@ typedef struct NvmeCreateCq {
 #define NVME_CQ_FLAGS_PC(cq_flags)  (cq_flags & 0x1)
 #define NVME_CQ_FLAGS_IEN(cq_flags) ((cq_flags >> 1) & 0x1)
 
-typedef struct NvmeCreateSq {
+typedef struct QEMU_PACKED NvmeCreateSq {
     uint8_t     opcode;
     uint8_t     flags;
     uint16_t    cid;
@@ -474,7 +474,7 @@ enum NvmeQueueFlags {
     NVME_Q_PRIO_LOW     = 3,
 };
 
-typedef struct NvmeIdentify {
+typedef struct QEMU_PACKED NvmeIdentify {
     uint8_t     opcode;
     uint8_t     flags;
     uint16_t    cid;
@@ -486,7 +486,7 @@ typedef struct NvmeIdentify {
     uint32_t    rsvd11[5];
 } NvmeIdentify;
 
-typedef struct NvmeRwCmd {
+typedef struct QEMU_PACKED NvmeRwCmd {
     uint8_t     opcode;
     uint8_t     flags;
     uint16_t    cid;
@@ -528,7 +528,7 @@ enum {
     NVME_RW_PRINFO_PRCHK_REF    = 1 << 10,
 };
 
-typedef struct NvmeDsmCmd {
+typedef struct QEMU_PACKED NvmeDsmCmd {
     uint8_t     opcode;
     uint8_t     flags;
     uint16_t    cid;
@@ -547,7 +547,7 @@ enum {
     NVME_DSMGMT_AD  = 1 << 2,
 };
 
-typedef struct NvmeDsmRange {
+typedef struct QEMU_PACKED NvmeDsmRange {
     uint32_t    cattr;
     uint32_t    nlb;
     uint64_t    slba;
@@ -569,14 +569,14 @@ enum NvmeAsyncEventRequest {
     NVME_AER_INFO_SMART_SPARE_THRESH        = 2,
 };
 
-typedef struct NvmeAerResult {
+typedef struct QEMU_PACKED NvmeAerResult {
     uint8_t event_type;
     uint8_t event_info;
     uint8_t log_page;
     uint8_t resv;
 } NvmeAerResult;
 
-typedef struct NvmeCqe {
+typedef struct QEMU_PACKED NvmeCqe {
     uint32_t    result;
     uint32_t    rsvd;
     uint16_t    sq_head;
@@ -634,7 +634,7 @@ enum NvmeStatusCodes {
     NVME_NO_COMPLETE            = 0xffff,
 };
 
-typedef struct NvmeFwSlotInfoLog {
+typedef struct QEMU_PACKED NvmeFwSlotInfoLog {
     uint8_t     afi;
     uint8_t     reserved1[7];
     uint8_t     frs1[8];
@@ -647,7 +647,7 @@ typedef struct NvmeFwSlotInfoLog {
     uint8_t     reserved2[448];
 } NvmeFwSlotInfoLog;
 
-typedef struct NvmeErrorLog {
+typedef struct QEMU_PACKED NvmeErrorLog {
     uint64_t    error_count;
     uint16_t    sqid;
     uint16_t    cid;
@@ -659,7 +659,7 @@ typedef struct NvmeErrorLog {
     uint8_t     resv[35];
 } NvmeErrorLog;
 
-typedef struct NvmeSmartLog {
+typedef struct QEMU_PACKED NvmeSmartLog {
     uint8_t     critical_warning;
     uint8_t     temperature[2];
     uint8_t     available_spare;
@@ -693,7 +693,7 @@ enum LogIdentifier {
     NVME_LOG_FW_SLOT_INFO   = 0x03,
 };
 
-typedef struct NvmePSD {
+typedef struct QEMU_PACKED NvmePSD {
     uint16_t    mp;
     uint16_t    reserved;
     uint32_t    enlat;
@@ -713,7 +713,7 @@ enum {
     NVME_ID_CNS_NS_ACTIVE_LIST = 0x2,
 };
 
-typedef struct NvmeIdCtrl {
+typedef struct QEMU_PACKED NvmeIdCtrl {
     uint16_t    vid;
     uint16_t    ssvid;
     uint8_t     sn[20];
@@ -807,7 +807,7 @@ enum NvmeFeatureIds {
     NVME_SOFTWARE_PROGRESS_MARKER   = 0x80
 };
 
-typedef struct NvmeRangeType {
+typedef struct QEMU_PACKED NvmeRangeType {
     uint8_t     type;
     uint8_t     attributes;
     uint8_t     rsvd2[14];
@@ -817,13 +817,13 @@ typedef struct NvmeRangeType {
     uint8_t     rsvd48[16];
 } NvmeRangeType;
 
-typedef struct NvmeLBAF {
+typedef struct QEMU_PACKED NvmeLBAF {
     uint16_t    ms;
     uint8_t     ds;
     uint8_t     rp;
 } NvmeLBAF;
 
-typedef struct NvmeIdNs {
+typedef struct QEMU_PACKED NvmeIdNs {
     uint64_t    nsze;
     uint64_t    ncap;
     uint64_t    nuse;
-- 
2.21.3

[PATCH v2 3/4] hw/block/nvme: Fix pmrmsc register size

[Philippe Mathieu-Daudé]

The Persistent Memory Region Controller Memory Space Control
register is 64-bit wide. See 'Figure 68: Register Definition'
of the 'NVM Express Base Specification Revision 1.4'.

Fixes: 6cf9413229 ("introduce PMR support from NVMe 1.4 spec")
Reported-by: Klaus Jensen <its@irrelevant.dk>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Cc: Andrzej Jakowski <andrzej.jakowski@linux.intel.com>
Cc: Keith Busch <kbusch@kernel.org>
---
 include/block/nvme.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/block/nvme.h b/include/block/nvme.h
index 71c5681912..82c384614a 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -21,7 +21,7 @@ typedef struct QEMU_PACKED NvmeBar {
     uint32_t    pmrsts;
     uint32_t    pmrebs;
     uint32_t    pmrswtp;
-    uint32_t    pmrmsc;
+    uint64_t    pmrmsc;
 } NvmeBar;
 
 enum NvmeCapShift {
-- 
2.21.3

[PATCH v2 4/4] hw/block/nvme: Align I/O BAR to 4 KiB

[Philippe Mathieu-Daudé]

Simplify the NVMe emulated device by aligning the I/O BAR to 4 KiB.

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v2: Do not include 'cmd_set_specfic' (Klaus)
---
 include/block/nvme.h | 2 ++
 hw/block/nvme.c      | 5 ++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/block/nvme.h b/include/block/nvme.h
index 82c384614a..4e1cea576a 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -22,6 +22,7 @@ typedef struct QEMU_PACKED NvmeBar {
     uint32_t    pmrebs;
     uint32_t    pmrswtp;
     uint64_t    pmrmsc;
+    uint8_t     reserved[484];
 } NvmeBar;
 
 enum NvmeCapShift {
@@ -879,6 +880,7 @@ enum NvmeIdNsDps {
 
 static inline void _nvme_check_size(void)
 {
+    QEMU_BUILD_BUG_ON(sizeof(NvmeBar) != 4096);
     QEMU_BUILD_BUG_ON(sizeof(NvmeAerResult) != 4);
     QEMU_BUILD_BUG_ON(sizeof(NvmeCqe) != 16);
     QEMU_BUILD_BUG_ON(sizeof(NvmeDsmRange) != 16);
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 6628d0a4ba..2aa54bc20e 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -55,7 +55,6 @@
 #include "nvme.h"
 
 #define NVME_MAX_IOQPAIRS 0xffff
-#define NVME_REG_SIZE 0x1000
 #define NVME_DB_SIZE  4
 #define NVME_CMB_BIR 2
 #define NVME_PMR_BIR 2
@@ -1322,7 +1321,7 @@ static void nvme_mmio_write(void *opaque, hwaddr addr, uint64_t data,
     NvmeCtrl *n = (NvmeCtrl *)opaque;
     if (addr < sizeof(n->bar)) {
         nvme_write_bar(n, addr, data, size);
-    } else if (addr >= 0x1000) {
+    } else {
         nvme_process_db(n, addr, data);
     }
 }
@@ -1416,7 +1415,7 @@ static void nvme_init_state(NvmeCtrl *n)
 {
     n->num_namespaces = 1;
     /* add one to max_ioqpairs to account for the admin queue pair */
-    n->reg_size = pow2ceil(NVME_REG_SIZE +
+    n->reg_size = pow2ceil(sizeof(NvmeBar) +
                            2 * (n->params.max_ioqpairs + 1) * NVME_DB_SIZE);
     n->namespaces = g_new0(NvmeNamespace, n->num_namespaces);
     n->sq = g_new0(NvmeSQueue *, n->params.max_ioqpairs + 1);
-- 
2.21.3

Re: [PATCH v2 1/4] hw/block/nvme: Update specification URL

[Klaus Jensen]

On Jun 30 12:37, Philippe Mathieu-Daudé wrote:
> At some point the URL changed, update it to avoid other
> developers to search for it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

> ---
>  hw/block/nvme.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 1aee042d4c..6628d0a4ba 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -11,7 +11,7 @@
>  /**
>   * Reference Specs: http://www.nvmexpress.org, 1.2, 1.1, 1.0e
>   *
> - *  http://www.nvmexpress.org/resources/
> + *  https://nvmexpress.org/developers/nvme-specification/
>   */
>  
>  /**
> -- 
> 2.21.3
> 
> 

Re: [PATCH v2 3/4] hw/block/nvme: Fix pmrmsc register size

[Klaus Jensen]

On Jun 30 12:37, Philippe Mathieu-Daudé wrote:
> The Persistent Memory Region Controller Memory Space Control
> register is 64-bit wide. See 'Figure 68: Register Definition'
> of the 'NVM Express Base Specification Revision 1.4'.
> 
> Fixes: 6cf9413229 ("introduce PMR support from NVMe 1.4 spec")
> Reported-by: Klaus Jensen <its@irrelevant.dk>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

(if possible, please change the Reported-by to my Samsung address)

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

> ---
> Cc: Andrzej Jakowski <andrzej.jakowski@linux.intel.com>
> Cc: Keith Busch <kbusch@kernel.org>
> ---
>  include/block/nvme.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 71c5681912..82c384614a 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -21,7 +21,7 @@ typedef struct QEMU_PACKED NvmeBar {
>      uint32_t    pmrsts;
>      uint32_t    pmrebs;
>      uint32_t    pmrswtp;
> -    uint32_t    pmrmsc;
> +    uint64_t    pmrmsc;
>  } NvmeBar;
>  
>  enum NvmeCapShift {
> -- 
> 2.21.3
> 
> 

Re: [PATCH v2 2/4] hw/block/nvme: Use QEMU_PACKED on hardware/packet structures

[Klaus Jensen]

On Jun 30 12:37, Philippe Mathieu-Daudé wrote:
> These structures either describe hardware registers, or
> commands ('packets') to send to the hardware. To forbid
> the compiler to optimize and change fields alignment,
> mark the structures as packed.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

> ---
>  include/block/nvme.h | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 1720ee1d51..71c5681912 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -1,7 +1,7 @@
>  #ifndef BLOCK_NVME_H
>  #define BLOCK_NVME_H
>  
> -typedef struct NvmeBar {
> +typedef struct QEMU_PACKED NvmeBar {
>      uint64_t    cap;
>      uint32_t    vs;
>      uint32_t    intms;
> @@ -377,7 +377,7 @@ enum NvmePmrmscMask {
>  #define NVME_PMRMSC_SET_CBA(pmrmsc, val)   \
>      (pmrmsc |= (uint64_t)(val & PMRMSC_CBA_MASK) << PMRMSC_CBA_SHIFT)
>  
> -typedef struct NvmeCmd {
> +typedef struct QEMU_PACKED NvmeCmd {
>      uint8_t     opcode;
>      uint8_t     fuse;
>      uint16_t    cid;
> @@ -422,7 +422,7 @@ enum NvmeIoCommands {
>      NVME_CMD_DSM                = 0x09,
>  };
>  
> -typedef struct NvmeDeleteQ {
> +typedef struct QEMU_PACKED NvmeDeleteQ {
>      uint8_t     opcode;
>      uint8_t     flags;
>      uint16_t    cid;
> @@ -432,7 +432,7 @@ typedef struct NvmeDeleteQ {
>      uint32_t    rsvd11[5];
>  } NvmeDeleteQ;
>  
> -typedef struct NvmeCreateCq {
> +typedef struct QEMU_PACKED NvmeCreateCq {
>      uint8_t     opcode;
>      uint8_t     flags;
>      uint16_t    cid;
> @@ -449,7 +449,7 @@ typedef struct NvmeCreateCq {
>  #define NVME_CQ_FLAGS_PC(cq_flags)  (cq_flags & 0x1)
>  #define NVME_CQ_FLAGS_IEN(cq_flags) ((cq_flags >> 1) & 0x1)
>  
> -typedef struct NvmeCreateSq {
> +typedef struct QEMU_PACKED NvmeCreateSq {
>      uint8_t     opcode;
>      uint8_t     flags;
>      uint16_t    cid;
> @@ -474,7 +474,7 @@ enum NvmeQueueFlags {
>      NVME_Q_PRIO_LOW     = 3,
>  };
>  
> -typedef struct NvmeIdentify {
> +typedef struct QEMU_PACKED NvmeIdentify {
>      uint8_t     opcode;
>      uint8_t     flags;
>      uint16_t    cid;
> @@ -486,7 +486,7 @@ typedef struct NvmeIdentify {
>      uint32_t    rsvd11[5];
>  } NvmeIdentify;
>  
> -typedef struct NvmeRwCmd {
> +typedef struct QEMU_PACKED NvmeRwCmd {
>      uint8_t     opcode;
>      uint8_t     flags;
>      uint16_t    cid;
> @@ -528,7 +528,7 @@ enum {
>      NVME_RW_PRINFO_PRCHK_REF    = 1 << 10,
>  };
>  
> -typedef struct NvmeDsmCmd {
> +typedef struct QEMU_PACKED NvmeDsmCmd {
>      uint8_t     opcode;
>      uint8_t     flags;
>      uint16_t    cid;
> @@ -547,7 +547,7 @@ enum {
>      NVME_DSMGMT_AD  = 1 << 2,
>  };
>  
> -typedef struct NvmeDsmRange {
> +typedef struct QEMU_PACKED NvmeDsmRange {
>      uint32_t    cattr;
>      uint32_t    nlb;
>      uint64_t    slba;
> @@ -569,14 +569,14 @@ enum NvmeAsyncEventRequest {
>      NVME_AER_INFO_SMART_SPARE_THRESH        = 2,
>  };
>  
> -typedef struct NvmeAerResult {
> +typedef struct QEMU_PACKED NvmeAerResult {
>      uint8_t event_type;
>      uint8_t event_info;
>      uint8_t log_page;
>      uint8_t resv;
>  } NvmeAerResult;
>  
> -typedef struct NvmeCqe {
> +typedef struct QEMU_PACKED NvmeCqe {
>      uint32_t    result;
>      uint32_t    rsvd;
>      uint16_t    sq_head;
> @@ -634,7 +634,7 @@ enum NvmeStatusCodes {
>      NVME_NO_COMPLETE            = 0xffff,
>  };
>  
> -typedef struct NvmeFwSlotInfoLog {
> +typedef struct QEMU_PACKED NvmeFwSlotInfoLog {
>      uint8_t     afi;
>      uint8_t     reserved1[7];
>      uint8_t     frs1[8];
> @@ -647,7 +647,7 @@ typedef struct NvmeFwSlotInfoLog {
>      uint8_t     reserved2[448];
>  } NvmeFwSlotInfoLog;
>  
> -typedef struct NvmeErrorLog {
> +typedef struct QEMU_PACKED NvmeErrorLog {
>      uint64_t    error_count;
>      uint16_t    sqid;
>      uint16_t    cid;
> @@ -659,7 +659,7 @@ typedef struct NvmeErrorLog {
>      uint8_t     resv[35];
>  } NvmeErrorLog;
>  
> -typedef struct NvmeSmartLog {
> +typedef struct QEMU_PACKED NvmeSmartLog {
>      uint8_t     critical_warning;
>      uint8_t     temperature[2];
>      uint8_t     available_spare;
> @@ -693,7 +693,7 @@ enum LogIdentifier {
>      NVME_LOG_FW_SLOT_INFO   = 0x03,
>  };
>  
> -typedef struct NvmePSD {
> +typedef struct QEMU_PACKED NvmePSD {
>      uint16_t    mp;
>      uint16_t    reserved;
>      uint32_t    enlat;
> @@ -713,7 +713,7 @@ enum {
>      NVME_ID_CNS_NS_ACTIVE_LIST = 0x2,
>  };
>  
> -typedef struct NvmeIdCtrl {
> +typedef struct QEMU_PACKED NvmeIdCtrl {
>      uint16_t    vid;
>      uint16_t    ssvid;
>      uint8_t     sn[20];
> @@ -807,7 +807,7 @@ enum NvmeFeatureIds {
>      NVME_SOFTWARE_PROGRESS_MARKER   = 0x80
>  };
>  
> -typedef struct NvmeRangeType {
> +typedef struct QEMU_PACKED NvmeRangeType {
>      uint8_t     type;
>      uint8_t     attributes;
>      uint8_t     rsvd2[14];
> @@ -817,13 +817,13 @@ typedef struct NvmeRangeType {
>      uint8_t     rsvd48[16];
>  } NvmeRangeType;
>  
> -typedef struct NvmeLBAF {
> +typedef struct QEMU_PACKED NvmeLBAF {
>      uint16_t    ms;
>      uint8_t     ds;
>      uint8_t     rp;
>  } NvmeLBAF;
>  
> -typedef struct NvmeIdNs {
> +typedef struct QEMU_PACKED NvmeIdNs {
>      uint64_t    nsze;
>      uint64_t    ncap;
>      uint64_t    nuse;
> -- 
> 2.21.3
> 
>