* [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 related [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, other threads:[~2021-04-21 5:44 UTC | newest]
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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).