hw/block/nvme-ns.c | 2 +- hw/block/nvme.c | 46 +++++++++++++++++++++++++------------------- include/block/nvme.h | 10 +++++----- 3 files changed, 32 insertions(+), 26 deletions(-)
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
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>
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.
© 2016 - 2024 Red Hat, Inc.