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

[PATCH v3 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

Since v2:
- Added Klaus' tags with correct address

$ git backport-diff -u v2
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/4:[----] [--] 'hw/block/nvme: Update specification URL'
002/4:[----] [--] 'hw/block/nvme: Use QEMU_PACKED on hardware/packet structures'
003/4:[----] [--] 'hw/block/nvme: Fix pmrmsc register size'
004/4:[----] [--] 'hw/block/nvme: Align I/O BAR to 4 KiB'

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

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
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 v3 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.

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
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 v3 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 <k.jensen@samsung.com>
Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Cc: Andrzej Jakowski <andrzej.jakowski@linux.intel.com>
---
 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 v3 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>
---
 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 v3 3/4] hw/block/nvme: Fix pmrmsc register size

[Andrzej Jakowski]

On 6/30/20 4:04 AM, 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 <k.jensen@samsung.com>
> Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Cc: Andrzej Jakowski <andrzej.jakowski@linux.intel.com>
> ---
>  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

This is good catch, though I wanted to highlight that this will still 
need to change as this register is not aligned properly and thus not in
compliance with spec.

Reviewed-by Andrzej Jakowski <andrzej.jakowski@linux.intel.com>

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

[Philippe Mathieu-Daudé]

On 6/30/20 5:10 PM, Andrzej Jakowski wrote:
> On 6/30/20 4:04 AM, 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 <k.jensen@samsung.com>
>> Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> Cc: Andrzej Jakowski <andrzej.jakowski@linux.intel.com>
>> ---
>>  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
> 
> This is good catch, though I wanted to highlight that this will still 
> need to change as this register is not aligned properly and thus not in
> compliance with spec.

I was wondering the odd alignment too. So you are saying at some time
in the future the spec will be updated to correct the alignment?

Should we use this instead?

      uint32_t    pmrmsc;
 +    uint32_t    pmrmsc_upper32; /* the spec define this, but *
 +                                 * only low 32-bit are used  */

Or eventually an unnamed struct:

 -    uint32_t    pmrmsc;
 +    struct {
 +        uint32_t pmrmsc;
 +        uint32_t pmrmsc_upper32; /* the spec define this, but *
 +                                  * only low 32-bit are used  */
 +    };

> 
> Reviewed-by Andrzej Jakowski <andrzej.jakowski@linux.intel.com>
> 

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

[Klaus Jensen]

On Jun 30 17:16, Philippe Mathieu-Daudé wrote:
> On 6/30/20 5:10 PM, Andrzej Jakowski wrote:
> > On 6/30/20 4:04 AM, 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 <k.jensen@samsung.com>
> >> Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >> Cc: Andrzej Jakowski <andrzej.jakowski@linux.intel.com>
> >> ---
> >>  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
> > 
> > This is good catch, though I wanted to highlight that this will still 
> > need to change as this register is not aligned properly and thus not in
> > compliance with spec.
> 
> I was wondering the odd alignment too. So you are saying at some time
> in the future the spec will be updated to correct the alignment?
> 
> Should we use this instead?
> 
>       uint32_t    pmrmsc;
>  +    uint32_t    pmrmsc_upper32; /* the spec define this, but *
>  +                                 * only low 32-bit are used  */
> 
> Or eventually an unnamed struct:
> 
>  -    uint32_t    pmrmsc;
>  +    struct {
>  +        uint32_t pmrmsc;
>  +        uint32_t pmrmsc_upper32; /* the spec define this, but *
>  +                                  * only low 32-bit are used  */
>  +    };
> 
> > 
> > Reviewed-by Andrzej Jakowski <andrzej.jakowski@linux.intel.com>
> > 
> 

I'm also not sure what you mean Andrzej. The odd alignment is exactly
what the spec (v1.4) says?

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

[Andrzej Jakowski]

On 6/30/20 9:45 AM, Klaus Jensen wrote:
> On Jun 30 17:16, Philippe Mathieu-Daudé wrote:
>> On 6/30/20 5:10 PM, Andrzej Jakowski wrote:
>>> On 6/30/20 4:04 AM, 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 <k.jensen@samsung.com>
>>>> Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>> Cc: Andrzej Jakowski <andrzej.jakowski@linux.intel.com>
>>>> ---
>>>>  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
>>>
>>> This is good catch, though I wanted to highlight that this will still 
>>> need to change as this register is not aligned properly and thus not in
>>> compliance with spec.
>>
>> I was wondering the odd alignment too. So you are saying at some time
>> in the future the spec will be updated to correct the alignment?
Yep I think so.
So PMRMSC currently is 64-bit register that is defined at E14h offset.
It is in conflict with spec because spec requires 64-bit registers to 
be 64-bit aligned.
I anticipate that changes will be needed.

>>
>> Should we use this instead?
>>
>>       uint32_t    pmrmsc;
>>  +    uint32_t    pmrmsc_upper32; /* the spec define this, but *
>>  +                                 * only low 32-bit are used  */
>>
>> Or eventually an unnamed struct:
>>
>>  -    uint32_t    pmrmsc;
>>  +    struct {
>>  +        uint32_t pmrmsc;
>>  +        uint32_t pmrmsc_upper32; /* the spec define this, but *
>>  +                                  * only low 32-bit are used  */
>>  +    };
>>
>>>
>>> Reviewed-by Andrzej Jakowski <andrzej.jakowski@linux.intel.com>
>>>
>>
> 
> I'm also not sure what you mean Andrzej. The odd alignment is exactly
> what the spec (v1.4) says?
> 

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

[Dmitry Fomichev]

On Tue, 2020-06-30 at 13:04 +0200, Philippe Mathieu-Daudé wrote:
> At some point the URL changed, update it to avoid other
> developers to search for it.
> 
> Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
> 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
> 
> 

Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>

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

[Dmitry Fomichev]

On Tue, 2020-06-30 at 13:04 +0200, 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.
> 
> Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
> 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
> 
> 

Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>

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

[Dmitry Fomichev]

On Tue, 2020-06-30 at 13:04 +0200, 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 <k.jensen@samsung.com>
> Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Cc: Andrzej Jakowski <andrzej.jakowski@linux.intel.com>
> ---
>  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
> 
> 

Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>

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

[Dmitry Fomichev]

On Tue, 2020-06-30 at 13:04 +0200, Philippe Mathieu-Daudé wrote:
> 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>
> ---
>  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
> 
> 

Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>

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

[Klaus Jensen]

On Jun 30 13:04, Philippe Mathieu-Daudé wrote:
> Improvements for the I/O BAR structure:
> - correct pmrmsc register size (Klaus)
> - pack structures
> - align to 4KB
> 
> Since v2:
> - Added Klaus' tags with correct address
> 
> $ git backport-diff -u v2
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> 
> 001/4:[----] [--] 'hw/block/nvme: Update specification URL'
> 002/4:[----] [--] 'hw/block/nvme: Use QEMU_PACKED on hardware/packet structures'
> 003/4:[----] [--] 'hw/block/nvme: Fix pmrmsc register size'
> 004/4:[----] [--] 'hw/block/nvme: Align I/O BAR to 4 KiB'
> 
> 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
> 
> 

Thanks Philippe! Applied to nvme-next.