Re: [PATCH v3] hw/block/nvme: align with existing style
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. ("h")
Signed-off-by: Gollu Appalanaidu
---
-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. ("h")
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, 0x means no limit */
+/* MAR/MOR are zeroes-based, Fh 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 0x). But if that feature is used
+ * (indicated by NSID being set to h). But if that feature is used
* along with TP 4056 (Namespace Types), it may be pretty screwed up.
*
- * If NSID is indeed set to 0x, we simply cannot associate the
+ * If NSID is indeed set to h, 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 0x then
+ * semantics in some other command set - does an NSID of h 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 0x"?
+ * NSID to be h"?
*
* 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 0x and the others to 0 as spec.
+ * be set to h 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 0x (NVME_NSID_BROADCAST) and 0xfffe are invalid values
+ * Both h (NVME_NSID_BROADCAST) and Eh 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(), 0x/0xfffe are invalid.
+ * Same as in nvme_identify_nslist(), h/Eh 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 0x. Since the device does not support those
+ * NSID is h. Since the device does not support those
* features we can always return Invalid Namespace or Format as we
* should do for all other features.
Re: [PATCH v3] hw/block/nvme: align with existing style
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. ("h")
Signed-off-by: Gollu Appalanaidu
---
-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. ("h")
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, 0x means no limit */
+/* MAR/MOR are zeroes-based, Fh 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 0x). But if that feature is used
+ * (indicated by NSID being set to h). But if that feature is used
* along with TP 4056 (Namespace Types), it may be pretty screwed up.
*
- * If NSID is indeed set to 0x, we simply cannot associate the
+ * If NSID is indeed set to h, 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 0x then
+ * semantics in some other command set - does an NSID of h 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 0x"?
+ * NSID to be h"?
*
* 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 0x and the others to 0 as spec.
+ * be set to h 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 0x (NVME_NSID_BROADCAST) and 0xfffe are invalid values
+ * Both h (NVME_NSID_BROADCAST) and Eh 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(), 0x/0xfffe are invalid.
+ * Same as in nvme_identify_nslist(), h/Eh 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 0x. Since the device does not support those
+ * NSID is h. Since the device does not support those
* features we can always return Invalid Namespace or Format as we
* should do for all other features.
[PATCH v3] hw/block/nvme: align with existing style
Use lower case hexadecimal format for the constants and in
comments use the same format as used in Spec. ("h")
Signed-off-by: Gollu Appalanaidu
---
-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. ("h")
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, 0x means no limit */
+/* MAR/MOR are zeroes-based, Fh 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 0x). But if that feature is used
+ * (indicated by NSID being set to h). But if that feature is used
* along with TP 4056 (Namespace Types), it may be pretty screwed up.
*
- * If NSID is indeed set to 0x, we simply cannot associate the
+ * If NSID is indeed set to h, 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 0x then
+ * semantics in some other command set - does an NSID of h 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 0x"?
+ * NSID to be h"?
*
* 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 0x and the others to 0 as spec.
+ * be set to h 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 0x (NVME_NSID_BROADCAST) and 0xfffe are invalid values
+ * Both h (NVME_NSID_BROADCAST) and Eh 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(), 0x/0xfffe are invalid.
+ * Same as in nvme_identify_nslist(), h/Eh 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 0x. Since the device does not support those
+ * NSID is h. Since the device does not support those
* features we can always return Invalid Namespace or Format as we
* should do for all
