QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3] hw/block/nvme: align with existing style
       [not found] <CGME20210416035611epcas5p10b843f74cc93c015b5e74da149a8bd18@epcas5p1.samsung.com>
@ 2021-04-16  3:52 ` Gollu Appalanaidu
  2021-04-16  5:28   ` Klaus Jensen
  0 siblings, 1 reply; 3+ messages in thread
From: Gollu Appalanaidu @ 2021-04-16  3:52 UTC (permalink / raw)
  To: qemu-devel
  Cc: fam, kwolf, qemu-block, Gollu Appalanaidu, mreitz, its, stefanha,
	kbusch, philmd

Use lower case hexadecimal format for the constants and in
comments use the same format as used in Spec. ("FFFFFFFFh")

Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
---
-v3: Add Suggestions (Philippe)
 Describe the NVMe subsystem style in nvme.c header

-v2: Address review comments (Klaus)
 use lower case hexa format for the code and in comments
 use the same format as used in Spec. ("FFFFFFFFh")

 hw/block/nvme-ns.c   |  2 +-
 hw/block/nvme.c      | 46 +++++++++++++++++++++++++-------------------
 include/block/nvme.h | 10 +++++-----
 3 files changed, 32 insertions(+), 26 deletions(-)

diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
index 7bb618f182..a0895614d9 100644
--- a/hw/block/nvme-ns.c
+++ b/hw/block/nvme-ns.c
@@ -303,7 +303,7 @@ static void nvme_ns_init_zoned(NvmeNamespace *ns)
 
     id_ns_z = g_malloc0(sizeof(NvmeIdNsZoned));
 
-    /* MAR/MOR are zeroes-based, 0xffffffff means no limit */
+    /* MAR/MOR are zeroes-based, FFFFFFFFFh means no limit */
     id_ns_z->mar = cpu_to_le32(ns->params.max_active_zones - 1);
     id_ns_z->mor = cpu_to_le32(ns->params.max_open_zones - 1);
     id_ns_z->zoc = 0;
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 624a1431d0..cbe7f33daa 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -134,6 +134,12 @@
  *         Setting this property to true enables Read Across Zone Boundaries.
  */
 
+/**
+ * While QEMU coding style prefers lowercase hexadecimal in constants,
+ * the NVMe subsystem use the format from the NVMe specifications in
+ * the comments: no '0x' prefix, uppercase, 'h' hexadecimal suffix
+ */
+
 #include "qemu/osdep.h"
 #include "qemu/units.h"
 #include "qemu/error-report.h"
@@ -3607,18 +3613,18 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeRequest *req)
 
     /*
      * In the base NVM command set, Flush may apply to all namespaces
-     * (indicated by NSID being set to 0xFFFFFFFF). But if that feature is used
+     * (indicated by NSID being set to FFFFFFFFh). But if that feature is used
      * along with TP 4056 (Namespace Types), it may be pretty screwed up.
      *
-     * If NSID is indeed set to 0xFFFFFFFF, we simply cannot associate the
+     * If NSID is indeed set to FFFFFFFFh, we simply cannot associate the
      * opcode with a specific command since we cannot determine a unique I/O
      * command set. Opcode 0x0 could have any other meaning than something
      * equivalent to flushing and say it DOES have completely different
-     * semantics in some other command set - does an NSID of 0xFFFFFFFF then
+     * semantics in some other command set - does an NSID of FFFFFFFFh then
      * mean "for all namespaces, apply whatever command set specific command
      * that uses the 0x0 opcode?" Or does it mean "for all namespaces, apply
      * whatever command that uses the 0x0 opcode if, and only if, it allows
-     * NSID to be 0xFFFFFFFF"?
+     * NSID to be FFFFFFFFh"?
      *
      * Anyway (and luckily), for now, we do not care about this since the
      * device only supports namespace types that includes the NVM Flush command
@@ -3934,7 +3940,7 @@ static uint16_t nvme_changed_nslist(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
             NVME_CHANGED_NSID_SIZE) {
         /*
          * If more than 1024 namespaces, the first entry in the log page should
-         * be set to 0xffffffff and the others to 0 as spec.
+         * be set to FFFFFFFFh and the others to 0 as spec.
          */
         if (i == ARRAY_SIZE(nslist)) {
             memset(nslist, 0x0, sizeof(nslist));
@@ -4332,7 +4338,7 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeRequest *req,
     trace_pci_nvme_identify_nslist(min_nsid);
 
     /*
-     * Both 0xffffffff (NVME_NSID_BROADCAST) and 0xfffffffe are invalid values
+     * Both FFFFFFFFh (NVME_NSID_BROADCAST) and FFFFFFFFEh are invalid values
      * since the Active Namespace ID List should return namespaces with ids
      * *higher* than the NSID specified in the command. This is also specified
      * in the spec (NVM Express v1.3d, Section 5.15.4).
@@ -4379,7 +4385,7 @@ static uint16_t nvme_identify_nslist_csi(NvmeCtrl *n, NvmeRequest *req,
     trace_pci_nvme_identify_nslist_csi(min_nsid, c->csi);
 
     /*
-     * Same as in nvme_identify_nslist(), 0xffffffff/0xfffffffe are invalid.
+     * Same as in nvme_identify_nslist(), FFFFFFFFh/FFFFFFFFEh are invalid.
      */
     if (min_nsid >= NVME_NSID_BROADCAST - 1) {
         return NVME_INVALID_NSID | NVME_DNR;
@@ -4595,7 +4601,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
             /*
              * The Reservation Notification Mask and Reservation Persistence
              * features require a status code of Invalid Field in Command when
-             * NSID is 0xFFFFFFFF. Since the device does not support those
+             * NSID is FFFFFFFFh. Since the device does not support those
              * features we can always return Invalid Namespace or Format as we
              * should do for all other features.
              */
@@ -4854,8 +4860,8 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeRequest *req)
             return NVME_INVALID_FIELD | NVME_DNR;
         }
 
-        trace_pci_nvme_setfeat_numq((dw11 & 0xFFFF) + 1,
-                                    ((dw11 >> 16) & 0xFFFF) + 1,
+        trace_pci_nvme_setfeat_numq((dw11 & 0xffff) + 1,
+                                    ((dw11 >> 16) & 0xffff) + 1,
                                     n->params.max_ioqpairs,
                                     n->params.max_ioqpairs);
         req->cqe.result = cpu_to_le32((n->params.max_ioqpairs - 1) |
@@ -5493,7 +5499,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
             n->bar.cc = data;
         }
         break;
-    case 0x1C:  /* CSTS */
+    case 0x1c:  /* CSTS */
         if (data & (1 << 4)) {
             NVME_GUEST_ERR(pci_nvme_ub_mmiowr_ssreset_w1c_unsupported,
                            "attempted to W1C CSTS.NSSRO"
@@ -5505,7 +5511,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
         }
         break;
     case 0x20:  /* NSSR */
-        if (data == 0x4E564D65) {
+        if (data == 0x4e564d65) {
             trace_pci_nvme_ub_mmiowr_ssreset_unsupported();
         } else {
             /* The spec says that writes of other values have no effect */
@@ -5575,11 +5581,11 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
         n->bar.cmbmsc = (n->bar.cmbmsc & 0xffffffff) | (data << 32);
         return;
 
-    case 0xE00: /* PMRCAP */
+    case 0xe00: /* PMRCAP */
         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrcap_readonly,
                        "invalid write to PMRCAP register, ignored");
         return;
-    case 0xE04: /* PMRCTL */
+    case 0xe04: /* PMRCTL */
         n->bar.pmrctl = data;
         if (NVME_PMRCTL_EN(data)) {
             memory_region_set_enabled(&n->pmr.dev->mr, true);
@@ -5590,19 +5596,19 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
             n->pmr.cmse = false;
         }
         return;
-    case 0xE08: /* PMRSTS */
+    case 0xe08: /* PMRSTS */
         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrsts_readonly,
                        "invalid write to PMRSTS register, ignored");
         return;
-    case 0xE0C: /* PMREBS */
+    case 0xe0C: /* PMREBS */
         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrebs_readonly,
                        "invalid write to PMREBS register, ignored");
         return;
-    case 0xE10: /* PMRSWTP */
+    case 0xe10: /* PMRSWTP */
         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrswtp_readonly,
                        "invalid write to PMRSWTP register, ignored");
         return;
-    case 0xE14: /* PMRMSCL */
+    case 0xe14: /* PMRMSCL */
         if (!NVME_CAP_PMRS(n->bar.cap)) {
             return;
         }
@@ -5622,7 +5628,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
         }
 
         return;
-    case 0xE18: /* PMRMSCU */
+    case 0xe18: /* PMRMSCU */
         if (!NVME_CAP_PMRS(n->bar.cap)) {
             return;
         }
@@ -5664,7 +5670,7 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
          * from PMRSTS should ensure prior writes
          * made it to persistent media
          */
-        if (addr == 0xE08 &&
+        if (addr == 0xe08 &&
             (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
             memory_region_msync(&n->pmr.dev->mr, 0, n->pmr.dev->size);
         }
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 4ac926fbc6..0739e0d665 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -848,8 +848,8 @@ enum NvmeStatusCodes {
     NVME_FW_REQ_SUSYSTEM_RESET  = 0x0110,
     NVME_NS_ALREADY_ATTACHED    = 0x0118,
     NVME_NS_PRIVATE             = 0x0119,
-    NVME_NS_NOT_ATTACHED        = 0x011A,
-    NVME_NS_CTRL_LIST_INVALID   = 0x011C,
+    NVME_NS_NOT_ATTACHED        = 0x011a,
+    NVME_NS_CTRL_LIST_INVALID   = 0x011c,
     NVME_CONFLICTING_ATTRS      = 0x0180,
     NVME_INVALID_PROT_INFO      = 0x0181,
     NVME_WRITE_TO_RO            = 0x0182,
@@ -1409,9 +1409,9 @@ typedef enum NvmeZoneState {
     NVME_ZONE_STATE_IMPLICITLY_OPEN  = 0x02,
     NVME_ZONE_STATE_EXPLICITLY_OPEN  = 0x03,
     NVME_ZONE_STATE_CLOSED           = 0x04,
-    NVME_ZONE_STATE_READ_ONLY        = 0x0D,
-    NVME_ZONE_STATE_FULL             = 0x0E,
-    NVME_ZONE_STATE_OFFLINE          = 0x0F,
+    NVME_ZONE_STATE_READ_ONLY        = 0x0d,
+    NVME_ZONE_STATE_FULL             = 0x0e,
+    NVME_ZONE_STATE_OFFLINE          = 0x0f,
 } NvmeZoneState;
 
 static inline void _nvme_check_size(void)
-- 
2.17.1



^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v3] hw/block/nvme: align with existing style
  2021-04-16  3:52 ` [PATCH v3] hw/block/nvme: align with existing style Gollu Appalanaidu
@ 2021-04-16  5:28   ` Klaus Jensen
  2021-04-21  5:38     ` Klaus Jensen
  0 siblings, 1 reply; 3+ messages in thread
From: Klaus Jensen @ 2021-04-16  5:28 UTC (permalink / raw)
  To: Gollu Appalanaidu
  Cc: fam, kwolf, qemu-block, qemu-devel, mreitz, stefanha, kbusch, philmd


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

On Apr 16 09:22, Gollu Appalanaidu wrote:
>Use lower case hexadecimal format for the constants and in
>comments use the same format as used in Spec. ("FFFFFFFFh")
>
>Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>---
>-v3: Add Suggestions (Philippe)
> Describe the NVMe subsystem style in nvme.c header
>
>-v2: Address review comments (Klaus)
> use lower case hexa format for the code and in comments
> use the same format as used in Spec. ("FFFFFFFFh")
>
> hw/block/nvme-ns.c   |  2 +-
> hw/block/nvme.c      | 46 +++++++++++++++++++++++++-------------------
> include/block/nvme.h | 10 +++++-----
> 3 files changed, 32 insertions(+), 26 deletions(-)
>
>diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
>index 7bb618f182..a0895614d9 100644
>--- a/hw/block/nvme-ns.c
>+++ b/hw/block/nvme-ns.c
>@@ -303,7 +303,7 @@ static void nvme_ns_init_zoned(NvmeNamespace *ns)
>
>     id_ns_z = g_malloc0(sizeof(NvmeIdNsZoned));
>
>-    /* MAR/MOR are zeroes-based, 0xffffffff means no limit */
>+    /* MAR/MOR are zeroes-based, FFFFFFFFFh means no limit */
>     id_ns_z->mar = cpu_to_le32(ns->params.max_active_zones - 1);
>     id_ns_z->mor = cpu_to_le32(ns->params.max_open_zones - 1);
>     id_ns_z->zoc = 0;
>diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>index 624a1431d0..cbe7f33daa 100644
>--- a/hw/block/nvme.c
>+++ b/hw/block/nvme.c
>@@ -134,6 +134,12 @@
>  *         Setting this property to true enables Read Across Zone Boundaries.
>  */
>
>+/**
>+ * While QEMU coding style prefers lowercase hexadecimal in constants,
>+ * the NVMe subsystem use the format from the NVMe specifications in
>+ * the comments: no '0x' prefix, uppercase, 'h' hexadecimal suffix
>+ */
>+
> #include "qemu/osdep.h"
> #include "qemu/units.h"
> #include "qemu/error-report.h"
>@@ -3607,18 +3613,18 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeRequest *req)
>
>     /*
>      * In the base NVM command set, Flush may apply to all namespaces
>-     * (indicated by NSID being set to 0xFFFFFFFF). But if that feature is used
>+     * (indicated by NSID being set to FFFFFFFFh). But if that feature is used
>      * along with TP 4056 (Namespace Types), it may be pretty screwed up.
>      *
>-     * If NSID is indeed set to 0xFFFFFFFF, we simply cannot associate the
>+     * If NSID is indeed set to FFFFFFFFh, we simply cannot associate the
>      * opcode with a specific command since we cannot determine a unique I/O
>      * command set. Opcode 0x0 could have any other meaning than something
>      * equivalent to flushing and say it DOES have completely different
>-     * semantics in some other command set - does an NSID of 0xFFFFFFFF then
>+     * semantics in some other command set - does an NSID of FFFFFFFFh then
>      * mean "for all namespaces, apply whatever command set specific command
>      * that uses the 0x0 opcode?" Or does it mean "for all namespaces, apply
>      * whatever command that uses the 0x0 opcode if, and only if, it allows
>-     * NSID to be 0xFFFFFFFF"?
>+     * NSID to be FFFFFFFFh"?
>      *
>      * Anyway (and luckily), for now, we do not care about this since the
>      * device only supports namespace types that includes the NVM Flush command
>@@ -3934,7 +3940,7 @@ static uint16_t nvme_changed_nslist(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
>             NVME_CHANGED_NSID_SIZE) {
>         /*
>          * If more than 1024 namespaces, the first entry in the log page should
>-         * be set to 0xffffffff and the others to 0 as spec.
>+         * be set to FFFFFFFFh and the others to 0 as spec.
>          */
>         if (i == ARRAY_SIZE(nslist)) {
>             memset(nslist, 0x0, sizeof(nslist));
>@@ -4332,7 +4338,7 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeRequest *req,
>     trace_pci_nvme_identify_nslist(min_nsid);
>
>     /*
>-     * Both 0xffffffff (NVME_NSID_BROADCAST) and 0xfffffffe are invalid values
>+     * Both FFFFFFFFh (NVME_NSID_BROADCAST) and FFFFFFFFEh are invalid values
>      * since the Active Namespace ID List should return namespaces with ids
>      * *higher* than the NSID specified in the command. This is also specified
>      * in the spec (NVM Express v1.3d, Section 5.15.4).
>@@ -4379,7 +4385,7 @@ static uint16_t nvme_identify_nslist_csi(NvmeCtrl *n, NvmeRequest *req,
>     trace_pci_nvme_identify_nslist_csi(min_nsid, c->csi);
>
>     /*
>-     * Same as in nvme_identify_nslist(), 0xffffffff/0xfffffffe are invalid.
>+     * Same as in nvme_identify_nslist(), FFFFFFFFh/FFFFFFFFEh are invalid.
>      */
>     if (min_nsid >= NVME_NSID_BROADCAST - 1) {
>         return NVME_INVALID_NSID | NVME_DNR;
>@@ -4595,7 +4601,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
>             /*
>              * The Reservation Notification Mask and Reservation Persistence
>              * features require a status code of Invalid Field in Command when
>-             * NSID is 0xFFFFFFFF. Since the device does not support those
>+             * NSID is FFFFFFFFh. Since the device does not support those
>              * features we can always return Invalid Namespace or Format as we
>              * should do for all other features.
>              */
>@@ -4854,8 +4860,8 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeRequest *req)
>             return NVME_INVALID_FIELD | NVME_DNR;
>         }
>
>-        trace_pci_nvme_setfeat_numq((dw11 & 0xFFFF) + 1,
>-                                    ((dw11 >> 16) & 0xFFFF) + 1,
>+        trace_pci_nvme_setfeat_numq((dw11 & 0xffff) + 1,
>+                                    ((dw11 >> 16) & 0xffff) + 1,
>                                     n->params.max_ioqpairs,
>                                     n->params.max_ioqpairs);
>         req->cqe.result = cpu_to_le32((n->params.max_ioqpairs - 1) |
>@@ -5493,7 +5499,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>             n->bar.cc = data;
>         }
>         break;
>-    case 0x1C:  /* CSTS */
>+    case 0x1c:  /* CSTS */
>         if (data & (1 << 4)) {
>             NVME_GUEST_ERR(pci_nvme_ub_mmiowr_ssreset_w1c_unsupported,
>                            "attempted to W1C CSTS.NSSRO"
>@@ -5505,7 +5511,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>         }
>         break;
>     case 0x20:  /* NSSR */
>-        if (data == 0x4E564D65) {
>+        if (data == 0x4e564d65) {
>             trace_pci_nvme_ub_mmiowr_ssreset_unsupported();
>         } else {
>             /* The spec says that writes of other values have no effect */
>@@ -5575,11 +5581,11 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>         n->bar.cmbmsc = (n->bar.cmbmsc & 0xffffffff) | (data << 32);
>         return;
>
>-    case 0xE00: /* PMRCAP */
>+    case 0xe00: /* PMRCAP */
>         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrcap_readonly,
>                        "invalid write to PMRCAP register, ignored");
>         return;
>-    case 0xE04: /* PMRCTL */
>+    case 0xe04: /* PMRCTL */
>         n->bar.pmrctl = data;
>         if (NVME_PMRCTL_EN(data)) {
>             memory_region_set_enabled(&n->pmr.dev->mr, true);
>@@ -5590,19 +5596,19 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>             n->pmr.cmse = false;
>         }
>         return;
>-    case 0xE08: /* PMRSTS */
>+    case 0xe08: /* PMRSTS */
>         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrsts_readonly,
>                        "invalid write to PMRSTS register, ignored");
>         return;
>-    case 0xE0C: /* PMREBS */
>+    case 0xe0C: /* PMREBS */
>         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrebs_readonly,
>                        "invalid write to PMREBS register, ignored");
>         return;
>-    case 0xE10: /* PMRSWTP */
>+    case 0xe10: /* PMRSWTP */
>         NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrswtp_readonly,
>                        "invalid write to PMRSWTP register, ignored");
>         return;
>-    case 0xE14: /* PMRMSCL */
>+    case 0xe14: /* PMRMSCL */
>         if (!NVME_CAP_PMRS(n->bar.cap)) {
>             return;
>         }
>@@ -5622,7 +5628,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>         }
>
>         return;
>-    case 0xE18: /* PMRMSCU */
>+    case 0xe18: /* PMRMSCU */
>         if (!NVME_CAP_PMRS(n->bar.cap)) {
>             return;
>         }
>@@ -5664,7 +5670,7 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
>          * from PMRSTS should ensure prior writes
>          * made it to persistent media
>          */
>-        if (addr == 0xE08 &&
>+        if (addr == 0xe08 &&
>             (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
>             memory_region_msync(&n->pmr.dev->mr, 0, n->pmr.dev->size);
>         }
>diff --git a/include/block/nvme.h b/include/block/nvme.h
>index 4ac926fbc6..0739e0d665 100644
>--- a/include/block/nvme.h
>+++ b/include/block/nvme.h
>@@ -848,8 +848,8 @@ enum NvmeStatusCodes {
>     NVME_FW_REQ_SUSYSTEM_RESET  = 0x0110,
>     NVME_NS_ALREADY_ATTACHED    = 0x0118,
>     NVME_NS_PRIVATE             = 0x0119,
>-    NVME_NS_NOT_ATTACHED        = 0x011A,
>-    NVME_NS_CTRL_LIST_INVALID   = 0x011C,
>+    NVME_NS_NOT_ATTACHED        = 0x011a,
>+    NVME_NS_CTRL_LIST_INVALID   = 0x011c,
>     NVME_CONFLICTING_ATTRS      = 0x0180,
>     NVME_INVALID_PROT_INFO      = 0x0181,
>     NVME_WRITE_TO_RO            = 0x0182,
>@@ -1409,9 +1409,9 @@ typedef enum NvmeZoneState {
>     NVME_ZONE_STATE_IMPLICITLY_OPEN  = 0x02,
>     NVME_ZONE_STATE_EXPLICITLY_OPEN  = 0x03,
>     NVME_ZONE_STATE_CLOSED           = 0x04,
>-    NVME_ZONE_STATE_READ_ONLY        = 0x0D,
>-    NVME_ZONE_STATE_FULL             = 0x0E,
>-    NVME_ZONE_STATE_OFFLINE          = 0x0F,
>+    NVME_ZONE_STATE_READ_ONLY        = 0x0d,
>+    NVME_ZONE_STATE_FULL             = 0x0e,
>+    NVME_ZONE_STATE_OFFLINE          = 0x0f,
> } NvmeZoneState;
>
> static inline void _nvme_check_size(void)
>-- 
>2.17.1
>

Looks good. I'll fixup a couple of missing constants in the comments 
(e.g. 0x0 -> 0h) when merged.

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

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v3] hw/block/nvme: align with existing style
  2021-04-16  5:28   ` Klaus Jensen
@ 2021-04-21  5:38     ` Klaus Jensen
  0 siblings, 0 replies; 3+ messages in thread
From: Klaus Jensen @ 2021-04-21  5:38 UTC (permalink / raw)
  To: Gollu Appalanaidu
  Cc: fam, kwolf, qemu-block, qemu-devel, mreitz, stefanha, kbusch, philmd


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

On Apr 16 07:28, Klaus Jensen wrote:
>On Apr 16 09:22, Gollu Appalanaidu wrote:
>>Use lower case hexadecimal format for the constants and in
>>comments use the same format as used in Spec. ("FFFFFFFFh")
>>
>>Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>>---
>>-v3: Add Suggestions (Philippe)
>>Describe the NVMe subsystem style in nvme.c header
>>
>>-v2: Address review comments (Klaus)
>>use lower case hexa format for the code and in comments
>>use the same format as used in Spec. ("FFFFFFFFh")
>>
>>hw/block/nvme-ns.c   |  2 +-
>>hw/block/nvme.c      | 46 +++++++++++++++++++++++++-------------------
>>include/block/nvme.h | 10 +++++-----
>>3 files changed, 32 insertions(+), 26 deletions(-)
>>
>>diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
>>index 7bb618f182..a0895614d9 100644
>>--- a/hw/block/nvme-ns.c
>>+++ b/hw/block/nvme-ns.c
>>@@ -303,7 +303,7 @@ static void nvme_ns_init_zoned(NvmeNamespace *ns)
>>
>>    id_ns_z = g_malloc0(sizeof(NvmeIdNsZoned));
>>
>>-    /* MAR/MOR are zeroes-based, 0xffffffff means no limit */
>>+    /* MAR/MOR are zeroes-based, FFFFFFFFFh means no limit */
>>    id_ns_z->mar = cpu_to_le32(ns->params.max_active_zones - 1);
>>    id_ns_z->mor = cpu_to_le32(ns->params.max_open_zones - 1);
>>    id_ns_z->zoc = 0;
>>diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>>index 624a1431d0..cbe7f33daa 100644
>>--- a/hw/block/nvme.c
>>+++ b/hw/block/nvme.c
>>@@ -134,6 +134,12 @@
>> *         Setting this property to true enables Read Across Zone Boundaries.
>> */
>>
>>+/**
>>+ * While QEMU coding style prefers lowercase hexadecimal in constants,
>>+ * the NVMe subsystem use the format from the NVMe specifications in
>>+ * the comments: no '0x' prefix, uppercase, 'h' hexadecimal suffix
>>+ */
>>+
>>#include "qemu/osdep.h"
>>#include "qemu/units.h"
>>#include "qemu/error-report.h"
>>@@ -3607,18 +3613,18 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeRequest *req)
>>
>>    /*
>>     * In the base NVM command set, Flush may apply to all namespaces
>>-     * (indicated by NSID being set to 0xFFFFFFFF). But if that feature is used
>>+     * (indicated by NSID being set to FFFFFFFFh). But if that feature is used
>>     * along with TP 4056 (Namespace Types), it may be pretty screwed up.
>>     *
>>-     * If NSID is indeed set to 0xFFFFFFFF, we simply cannot associate the
>>+     * If NSID is indeed set to FFFFFFFFh, we simply cannot associate the
>>     * opcode with a specific command since we cannot determine a unique I/O
>>     * command set. Opcode 0x0 could have any other meaning than something
>>     * equivalent to flushing and say it DOES have completely different
>>-     * semantics in some other command set - does an NSID of 0xFFFFFFFF then
>>+     * semantics in some other command set - does an NSID of FFFFFFFFh then
>>     * mean "for all namespaces, apply whatever command set specific command
>>     * that uses the 0x0 opcode?" Or does it mean "for all namespaces, apply
>>     * whatever command that uses the 0x0 opcode if, and only if, it allows
>>-     * NSID to be 0xFFFFFFFF"?
>>+     * NSID to be FFFFFFFFh"?
>>     *
>>     * Anyway (and luckily), for now, we do not care about this since the
>>     * device only supports namespace types that includes the NVM Flush command
>>@@ -3934,7 +3940,7 @@ static uint16_t nvme_changed_nslist(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
>>            NVME_CHANGED_NSID_SIZE) {
>>        /*
>>         * If more than 1024 namespaces, the first entry in the log page should
>>-         * be set to 0xffffffff and the others to 0 as spec.
>>+         * be set to FFFFFFFFh and the others to 0 as spec.
>>         */
>>        if (i == ARRAY_SIZE(nslist)) {
>>            memset(nslist, 0x0, sizeof(nslist));
>>@@ -4332,7 +4338,7 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeRequest *req,
>>    trace_pci_nvme_identify_nslist(min_nsid);
>>
>>    /*
>>-     * Both 0xffffffff (NVME_NSID_BROADCAST) and 0xfffffffe are invalid values
>>+     * Both FFFFFFFFh (NVME_NSID_BROADCAST) and FFFFFFFFEh are invalid values
>>     * since the Active Namespace ID List should return namespaces with ids
>>     * *higher* than the NSID specified in the command. This is also specified
>>     * in the spec (NVM Express v1.3d, Section 5.15.4).
>>@@ -4379,7 +4385,7 @@ static uint16_t nvme_identify_nslist_csi(NvmeCtrl *n, NvmeRequest *req,
>>    trace_pci_nvme_identify_nslist_csi(min_nsid, c->csi);
>>
>>    /*
>>-     * Same as in nvme_identify_nslist(), 0xffffffff/0xfffffffe are invalid.
>>+     * Same as in nvme_identify_nslist(), FFFFFFFFh/FFFFFFFFEh are invalid.
>>     */
>>    if (min_nsid >= NVME_NSID_BROADCAST - 1) {
>>        return NVME_INVALID_NSID | NVME_DNR;
>>@@ -4595,7 +4601,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
>>            /*
>>             * The Reservation Notification Mask and Reservation Persistence
>>             * features require a status code of Invalid Field in Command when
>>-             * NSID is 0xFFFFFFFF. Since the device does not support those
>>+             * NSID is FFFFFFFFh. Since the device does not support those
>>             * features we can always return Invalid Namespace or Format as we
>>             * should do for all other features.
>>             */
>>@@ -4854,8 +4860,8 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeRequest *req)
>>            return NVME_INVALID_FIELD | NVME_DNR;
>>        }
>>
>>-        trace_pci_nvme_setfeat_numq((dw11 & 0xFFFF) + 1,
>>-                                    ((dw11 >> 16) & 0xFFFF) + 1,
>>+        trace_pci_nvme_setfeat_numq((dw11 & 0xffff) + 1,
>>+                                    ((dw11 >> 16) & 0xffff) + 1,
>>                                    n->params.max_ioqpairs,
>>                                    n->params.max_ioqpairs);
>>        req->cqe.result = cpu_to_le32((n->params.max_ioqpairs - 1) |
>>@@ -5493,7 +5499,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>>            n->bar.cc = data;
>>        }
>>        break;
>>-    case 0x1C:  /* CSTS */
>>+    case 0x1c:  /* CSTS */
>>        if (data & (1 << 4)) {
>>            NVME_GUEST_ERR(pci_nvme_ub_mmiowr_ssreset_w1c_unsupported,
>>                           "attempted to W1C CSTS.NSSRO"
>>@@ -5505,7 +5511,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>>        }
>>        break;
>>    case 0x20:  /* NSSR */
>>-        if (data == 0x4E564D65) {
>>+        if (data == 0x4e564d65) {
>>            trace_pci_nvme_ub_mmiowr_ssreset_unsupported();
>>        } else {
>>            /* The spec says that writes of other values have no effect */
>>@@ -5575,11 +5581,11 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>>        n->bar.cmbmsc = (n->bar.cmbmsc & 0xffffffff) | (data << 32);
>>        return;
>>
>>-    case 0xE00: /* PMRCAP */
>>+    case 0xe00: /* PMRCAP */
>>        NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrcap_readonly,
>>                       "invalid write to PMRCAP register, ignored");
>>        return;
>>-    case 0xE04: /* PMRCTL */
>>+    case 0xe04: /* PMRCTL */
>>        n->bar.pmrctl = data;
>>        if (NVME_PMRCTL_EN(data)) {
>>            memory_region_set_enabled(&n->pmr.dev->mr, true);
>>@@ -5590,19 +5596,19 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>>            n->pmr.cmse = false;
>>        }
>>        return;
>>-    case 0xE08: /* PMRSTS */
>>+    case 0xe08: /* PMRSTS */
>>        NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrsts_readonly,
>>                       "invalid write to PMRSTS register, ignored");
>>        return;
>>-    case 0xE0C: /* PMREBS */
>>+    case 0xe0C: /* PMREBS */
>>        NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrebs_readonly,
>>                       "invalid write to PMREBS register, ignored");
>>        return;
>>-    case 0xE10: /* PMRSWTP */
>>+    case 0xe10: /* PMRSWTP */
>>        NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrswtp_readonly,
>>                       "invalid write to PMRSWTP register, ignored");
>>        return;
>>-    case 0xE14: /* PMRMSCL */
>>+    case 0xe14: /* PMRMSCL */
>>        if (!NVME_CAP_PMRS(n->bar.cap)) {
>>            return;
>>        }
>>@@ -5622,7 +5628,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>>        }
>>
>>        return;
>>-    case 0xE18: /* PMRMSCU */
>>+    case 0xe18: /* PMRMSCU */
>>        if (!NVME_CAP_PMRS(n->bar.cap)) {
>>            return;
>>        }
>>@@ -5664,7 +5670,7 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
>>         * from PMRSTS should ensure prior writes
>>         * made it to persistent media
>>         */
>>-        if (addr == 0xE08 &&
>>+        if (addr == 0xe08 &&
>>            (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
>>            memory_region_msync(&n->pmr.dev->mr, 0, n->pmr.dev->size);
>>        }
>>diff --git a/include/block/nvme.h b/include/block/nvme.h
>>index 4ac926fbc6..0739e0d665 100644
>>--- a/include/block/nvme.h
>>+++ b/include/block/nvme.h
>>@@ -848,8 +848,8 @@ enum NvmeStatusCodes {
>>    NVME_FW_REQ_SUSYSTEM_RESET  = 0x0110,
>>    NVME_NS_ALREADY_ATTACHED    = 0x0118,
>>    NVME_NS_PRIVATE             = 0x0119,
>>-    NVME_NS_NOT_ATTACHED        = 0x011A,
>>-    NVME_NS_CTRL_LIST_INVALID   = 0x011C,
>>+    NVME_NS_NOT_ATTACHED        = 0x011a,
>>+    NVME_NS_CTRL_LIST_INVALID   = 0x011c,
>>    NVME_CONFLICTING_ATTRS      = 0x0180,
>>    NVME_INVALID_PROT_INFO      = 0x0181,
>>    NVME_WRITE_TO_RO            = 0x0182,
>>@@ -1409,9 +1409,9 @@ typedef enum NvmeZoneState {
>>    NVME_ZONE_STATE_IMPLICITLY_OPEN  = 0x02,
>>    NVME_ZONE_STATE_EXPLICITLY_OPEN  = 0x03,
>>    NVME_ZONE_STATE_CLOSED           = 0x04,
>>-    NVME_ZONE_STATE_READ_ONLY        = 0x0D,
>>-    NVME_ZONE_STATE_FULL             = 0x0E,
>>-    NVME_ZONE_STATE_OFFLINE          = 0x0F,
>>+    NVME_ZONE_STATE_READ_ONLY        = 0x0d,
>>+    NVME_ZONE_STATE_FULL             = 0x0e,
>>+    NVME_ZONE_STATE_OFFLINE          = 0x0f,
>>} NvmeZoneState;
>>
>>static inline void _nvme_check_size(void)
>>-- 
>>2.17.1
>>
>
>Looks good. I'll fixup a couple of missing constants in the comments 
>(e.g. 0x0 -> 0h) when merged.
>
>Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

Applied to nvme-next.

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20210416035611epcas5p10b843f74cc93c015b5e74da149a8bd18@epcas5p1.samsung.com>
2021-04-16  3:52 ` [PATCH v3] hw/block/nvme: align with existing style Gollu Appalanaidu
2021-04-16  5:28   ` Klaus Jensen
2021-04-21  5:38     ` Klaus Jensen

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git
	git clone --mirror https://lore.kernel.org/qemu-devel/2 qemu-devel/git/2.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git