From: Klaus Jensen <k.jensen@samsung.com>
Add the NvmeBarRegs enum and use these instead of explicit register
offsets.
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
include/block/nvme.h | 27 +++++++++++++++++++++++++++
hw/nvme/ctrl.c | 44 ++++++++++++++++++++++----------------------
2 files changed, 49 insertions(+), 22 deletions(-)
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 84053b68b987..082d4bddbf9f 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -31,6 +31,33 @@ typedef struct QEMU_PACKED NvmeBar {
uint8_t css[484];
} NvmeBar;
+enum NvmeBarRegs {
+ NVME_REG_CAP = 0x0,
+ NVME_REG_VS = 0x8,
+ NVME_REG_INTMS = 0xc,
+ NVME_REG_INTMC = 0x10,
+ NVME_REG_CC = 0x14,
+ NVME_REG_CSTS = 0x1c,
+ NVME_REG_NSSR = 0x20,
+ NVME_REG_AQA = 0x24,
+ NVME_REG_ASQ = 0x28,
+ NVME_REG_ACQ = 0x30,
+ NVME_REG_CMBLOC = 0x38,
+ NVME_REG_CMBSZ = 0x3c,
+ NVME_REG_BPINFO = 0x40,
+ NVME_REG_BPRSEL = 0x44,
+ NVME_REG_BPMBL = 0x48,
+ NVME_REG_CMBMSC = 0x50,
+ NVME_REG_CMBSTS = 0x58,
+ NVME_REG_PMRCAP = 0xe00,
+ NVME_REG_PMRCTL = 0xe04,
+ NVME_REG_PMRSTS = 0xe08,
+ NVME_REG_PMREBS = 0xe0c,
+ NVME_REG_PMRSWTP = 0xe10,
+ NVME_REG_PMRMSCL = 0xe14,
+ NVME_REG_PMRMSCU = 0xe18,
+};
+
enum NvmeCapShift {
CAP_MQES_SHIFT = 0,
CAP_CQR_SHIFT = 16,
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 28299c6f3764..8c305315f41c 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -5740,7 +5740,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
}
switch (offset) {
- case 0xc: /* INTMS */
+ case NVME_REG_INTMS:
if (unlikely(msix_enabled(&(n->parent_obj)))) {
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_intmask_with_msix,
"undefined access to interrupt mask set"
@@ -5752,7 +5752,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
trace_pci_nvme_mmio_intm_set(data & 0xffffffff, n->bar.intmc);
nvme_irq_check(n);
break;
- case 0x10: /* INTMC */
+ case NVME_REG_INTMC:
if (unlikely(msix_enabled(&(n->parent_obj)))) {
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_intmask_with_msix,
"undefined access to interrupt mask clr"
@@ -5764,7 +5764,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
trace_pci_nvme_mmio_intm_clr(data & 0xffffffff, n->bar.intmc);
nvme_irq_check(n);
break;
- case 0x14: /* CC */
+ case NVME_REG_CC:
trace_pci_nvme_mmio_cfg(data & 0xffffffff);
/* Windows first sends data, then sends enable bit */
if (!NVME_CC_EN(data) && !NVME_CC_EN(n->bar.cc) &&
@@ -5798,7 +5798,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
n->bar.cc = data;
}
break;
- case 0x1c: /* CSTS */
+ case NVME_REG_CSTS:
if (data & (1 << 4)) {
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_ssreset_w1c_unsupported,
"attempted to W1C CSTS.NSSRO"
@@ -5809,7 +5809,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
" of controller status");
}
break;
- case 0x20: /* NSSR */
+ case NVME_REG_NSSR:
if (data == 0x4e564d65) {
trace_pci_nvme_ub_mmiowr_ssreset_unsupported();
} else {
@@ -5817,38 +5817,38 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
return;
}
break;
- case 0x24: /* AQA */
+ case NVME_REG_AQA:
n->bar.aqa = data & 0xffffffff;
trace_pci_nvme_mmio_aqattr(data & 0xffffffff);
break;
- case 0x28: /* ASQ */
+ case NVME_REG_ASQ:
n->bar.asq = size == 8 ? data :
(n->bar.asq & ~0xffffffffULL) | (data & 0xffffffff);
trace_pci_nvme_mmio_asqaddr(data);
break;
- case 0x2c: /* ASQ hi */
+ case NVME_REG_ASQ + 4:
n->bar.asq = (n->bar.asq & 0xffffffff) | (data << 32);
trace_pci_nvme_mmio_asqaddr_hi(data, n->bar.asq);
break;
- case 0x30: /* ACQ */
+ case NVME_REG_ACQ:
trace_pci_nvme_mmio_acqaddr(data);
n->bar.acq = size == 8 ? data :
(n->bar.acq & ~0xffffffffULL) | (data & 0xffffffff);
break;
- case 0x34: /* ACQ hi */
+ case NVME_REG_ACQ + 4:
n->bar.acq = (n->bar.acq & 0xffffffff) | (data << 32);
trace_pci_nvme_mmio_acqaddr_hi(data, n->bar.acq);
break;
- case 0x38: /* CMBLOC */
+ case NVME_REG_CMBLOC:
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_cmbloc_reserved,
"invalid write to reserved CMBLOC"
" when CMBSZ is zero, ignored");
return;
- case 0x3C: /* CMBSZ */
+ case NVME_REG_CMBSZ:
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_cmbsz_readonly,
"invalid write to read only CMBSZ, ignored");
return;
- case 0x50: /* CMBMSC */
+ case NVME_REG_CMBMSC:
if (!NVME_CAP_CMBS(n->bar.cap)) {
return;
}
@@ -5876,15 +5876,15 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
}
return;
- case 0x54: /* CMBMSC hi */
+ case NVME_REG_CMBMSC + 4:
n->bar.cmbmsc = (n->bar.cmbmsc & 0xffffffff) | (data << 32);
return;
- case 0xe00: /* PMRCAP */
+ case NVME_REG_PMRCAP:
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrcap_readonly,
"invalid write to PMRCAP register, ignored");
return;
- case 0xe04: /* PMRCTL */
+ case NVME_REG_PMRCTL:
if (!NVME_CAP_PMRS(n->bar.cap)) {
return;
}
@@ -5899,19 +5899,19 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
n->pmr.cmse = false;
}
return;
- case 0xe08: /* PMRSTS */
+ case NVME_REG_PMRSTS:
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrsts_readonly,
"invalid write to PMRSTS register, ignored");
return;
- case 0xe0C: /* PMREBS */
+ case NVME_REG_PMREBS:
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrebs_readonly,
"invalid write to PMREBS register, ignored");
return;
- case 0xe10: /* PMRSWTP */
+ case NVME_REG_PMRSWTP:
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrswtp_readonly,
"invalid write to PMRSWTP register, ignored");
return;
- case 0xe14: /* PMRMSCL */
+ case NVME_REG_PMRMSCL:
if (!NVME_CAP_PMRS(n->bar.cap)) {
return;
}
@@ -5932,7 +5932,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
}
return;
- case 0xe18: /* PMRMSCU */
+ case NVME_REG_PMRMSCU:
if (!NVME_CAP_PMRS(n->bar.cap)) {
return;
}
@@ -5974,7 +5974,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 == NVME_REG_PMRSTS &&
(NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
memory_region_msync(&n->pmr.dev->mr, 0, n->pmr.dev->size);
}
--
2.32.0
On 7/13/21 9:24 PM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> Add the NvmeBarRegs enum and use these instead of explicit register
> offsets.
>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
> include/block/nvme.h | 27 +++++++++++++++++++++++++++
> hw/nvme/ctrl.c | 44 ++++++++++++++++++++++----------------------
> 2 files changed, 49 insertions(+), 22 deletions(-)
>
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 84053b68b987..082d4bddbf9f 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -31,6 +31,33 @@ typedef struct QEMU_PACKED NvmeBar {
> uint8_t css[484];
> } NvmeBar;
>
> +enum NvmeBarRegs {
> + NVME_REG_CAP = 0x0,
> + NVME_REG_VS = 0x8,
> + NVME_REG_INTMS = 0xc,
> + NVME_REG_INTMC = 0x10,
> + NVME_REG_CC = 0x14,
> + NVME_REG_CSTS = 0x1c,
> + NVME_REG_NSSR = 0x20,
> + NVME_REG_AQA = 0x24,
> + NVME_REG_ASQ = 0x28,
> + NVME_REG_ACQ = 0x30,
> + NVME_REG_CMBLOC = 0x38,
> + NVME_REG_CMBSZ = 0x3c,
> + NVME_REG_BPINFO = 0x40,
> + NVME_REG_BPRSEL = 0x44,
> + NVME_REG_BPMBL = 0x48,
> + NVME_REG_CMBMSC = 0x50,
> + NVME_REG_CMBSTS = 0x58,
> + NVME_REG_PMRCAP = 0xe00,
> + NVME_REG_PMRCTL = 0xe04,
> + NVME_REG_PMRSTS = 0xe08,
> + NVME_REG_PMREBS = 0xe0c,
> + NVME_REG_PMRSWTP = 0xe10,
> + NVME_REG_PMRMSCL = 0xe14,
> + NVME_REG_PMRMSCU = 0xe18,
> +};
> +
> enum NvmeCapShift {
> CAP_MQES_SHIFT = 0,
> CAP_CQR_SHIFT = 16,
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 28299c6f3764..8c305315f41c 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -5740,7 +5740,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
> }
>
> switch (offset) {
> - case 0xc: /* INTMS */
> + case NVME_REG_INTMS:
What about using offsetof(NvmeBar, intms) instead?
On 7/14/21 12:12 AM, Philippe Mathieu-Daudé wrote:
> On 7/13/21 9:24 PM, Klaus Jensen wrote:
>> From: Klaus Jensen <k.jensen@samsung.com>
>>
>> Add the NvmeBarRegs enum and use these instead of explicit register
>> offsets.
>>
>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>> ---
>> include/block/nvme.h | 27 +++++++++++++++++++++++++++
>> hw/nvme/ctrl.c | 44 ++++++++++++++++++++++----------------------
>> 2 files changed, 49 insertions(+), 22 deletions(-)
>>
>> diff --git a/include/block/nvme.h b/include/block/nvme.h
>> index 84053b68b987..082d4bddbf9f 100644
>> --- a/include/block/nvme.h
>> +++ b/include/block/nvme.h
>> @@ -31,6 +31,33 @@ typedef struct QEMU_PACKED NvmeBar {
>> uint8_t css[484];
>> } NvmeBar;
>>
>> +enum NvmeBarRegs {
>> + NVME_REG_CAP = 0x0,
>> + NVME_REG_VS = 0x8,
>> + NVME_REG_INTMS = 0xc,
>> + NVME_REG_INTMC = 0x10,
>> + NVME_REG_CC = 0x14,
>> + NVME_REG_CSTS = 0x1c,
>> + NVME_REG_NSSR = 0x20,
>> + NVME_REG_AQA = 0x24,
>> + NVME_REG_ASQ = 0x28,
>> + NVME_REG_ACQ = 0x30,
>> + NVME_REG_CMBLOC = 0x38,
>> + NVME_REG_CMBSZ = 0x3c,
>> + NVME_REG_BPINFO = 0x40,
>> + NVME_REG_BPRSEL = 0x44,
>> + NVME_REG_BPMBL = 0x48,
>> + NVME_REG_CMBMSC = 0x50,
>> + NVME_REG_CMBSTS = 0x58,
>> + NVME_REG_PMRCAP = 0xe00,
>> + NVME_REG_PMRCTL = 0xe04,
>> + NVME_REG_PMRSTS = 0xe08,
>> + NVME_REG_PMREBS = 0xe0c,
>> + NVME_REG_PMRSWTP = 0xe10,
>> + NVME_REG_PMRMSCL = 0xe14,
>> + NVME_REG_PMRMSCU = 0xe18,
>> +};
>> +
>> enum NvmeCapShift {
>> CAP_MQES_SHIFT = 0,
>> CAP_CQR_SHIFT = 16,
>> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>> index 28299c6f3764..8c305315f41c 100644
>> --- a/hw/nvme/ctrl.c
>> +++ b/hw/nvme/ctrl.c
>> @@ -5740,7 +5740,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>> }
>>
>> switch (offset) {
>> - case 0xc: /* INTMS */
>> + case NVME_REG_INTMS:
>
> What about using offsetof(NvmeBar, intms) instead?
BTW I'm not suggesting this is better, I just wonder how we can avoid
to duplicate the definitions. Alternative is declaring:
enum NvmeBarRegs {
NVME_REG_CAP = offsetof(NvmeBar, cap),
NVME_REG_VS = offsetof(NvmeBar, vs),
...
Or keeping your patch.
On Jul 14 00:21, Philippe Mathieu-Daudé wrote:
> On 7/14/21 12:12 AM, Philippe Mathieu-Daudé wrote:
> > On 7/13/21 9:24 PM, Klaus Jensen wrote:
> >> From: Klaus Jensen <k.jensen@samsung.com>
> >>
> >> Add the NvmeBarRegs enum and use these instead of explicit register
> >> offsets.
> >>
> >> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> >> ---
> >> include/block/nvme.h | 27 +++++++++++++++++++++++++++
> >> hw/nvme/ctrl.c | 44 ++++++++++++++++++++++----------------------
> >> 2 files changed, 49 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/include/block/nvme.h b/include/block/nvme.h
> >> index 84053b68b987..082d4bddbf9f 100644
> >> --- a/include/block/nvme.h
> >> +++ b/include/block/nvme.h
> >> @@ -31,6 +31,33 @@ typedef struct QEMU_PACKED NvmeBar {
> >> uint8_t css[484];
> >> } NvmeBar;
> >>
> >> +enum NvmeBarRegs {
> >> + NVME_REG_CAP = 0x0,
> >> + NVME_REG_VS = 0x8,
> >> + NVME_REG_INTMS = 0xc,
> >> + NVME_REG_INTMC = 0x10,
> >> + NVME_REG_CC = 0x14,
> >> + NVME_REG_CSTS = 0x1c,
> >> + NVME_REG_NSSR = 0x20,
> >> + NVME_REG_AQA = 0x24,
> >> + NVME_REG_ASQ = 0x28,
> >> + NVME_REG_ACQ = 0x30,
> >> + NVME_REG_CMBLOC = 0x38,
> >> + NVME_REG_CMBSZ = 0x3c,
> >> + NVME_REG_BPINFO = 0x40,
> >> + NVME_REG_BPRSEL = 0x44,
> >> + NVME_REG_BPMBL = 0x48,
> >> + NVME_REG_CMBMSC = 0x50,
> >> + NVME_REG_CMBSTS = 0x58,
> >> + NVME_REG_PMRCAP = 0xe00,
> >> + NVME_REG_PMRCTL = 0xe04,
> >> + NVME_REG_PMRSTS = 0xe08,
> >> + NVME_REG_PMREBS = 0xe0c,
> >> + NVME_REG_PMRSWTP = 0xe10,
> >> + NVME_REG_PMRMSCL = 0xe14,
> >> + NVME_REG_PMRMSCU = 0xe18,
> >> +};
> >> +
> >> enum NvmeCapShift {
> >> CAP_MQES_SHIFT = 0,
> >> CAP_CQR_SHIFT = 16,
> >> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> >> index 28299c6f3764..8c305315f41c 100644
> >> --- a/hw/nvme/ctrl.c
> >> +++ b/hw/nvme/ctrl.c
> >> @@ -5740,7 +5740,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
> >> }
> >>
> >> switch (offset) {
> >> - case 0xc: /* INTMS */
> >> + case NVME_REG_INTMS:
> >
> > What about using offsetof(NvmeBar, intms) instead?
>
> BTW I'm not suggesting this is better, I just wonder how we can avoid
> to duplicate the definitions. Alternative is declaring:
>
> enum NvmeBarRegs {
> NVME_REG_CAP = offsetof(NvmeBar, cap),
> NVME_REG_VS = offsetof(NvmeBar, vs),
> ...
>
I like this suggestion!
© 2016 - 2026 Red Hat, Inc.