Re: [PATCH 11/17] hw/block/nvme: add remaining mandatory controller parameters
On Jul 3 00:46, Dmitry Fomichev wrote:
> LGTM with one small nit (see below)...
>
> Reviewed-by: Dmitry Fomichev
>
> On Mon, 2020-06-29 at 20:26 +0200, Klaus Jensen wrote:
> > From: Klaus Jensen
> >
> > Add support for any remaining mandatory controller operating parameters
> > (features).
> >
> > Signed-off-by: Klaus Jensen
> > ---
> > hw/block/nvme.c | 39 +--
> > hw/block/nvme.h | 18 ++
> > hw/block/trace-events | 2 ++
> > include/block/nvme.h | 7 +++
> > 4 files changed, 60 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index da13ca1ddb60..647f408854ae 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1057,8 +1057,16 @@ static uint16_t nvme_get_feature(NvmeCtrl *n,
> > NvmeCmd *cmd, NvmeRequest *req)
> > uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> > uint32_t dw11 = le32_to_cpu(cmd->cdw11);
> > uint32_t result;
> > +uint8_t fid = NVME_GETSETFEAT_FID(dw10);
> > +uint16_t iv;
> >
> > -switch (dw10) {
> > +trace_pci_nvme_getfeat(nvme_cid(req), fid, dw11);
> > +
> > +if (!nvme_feature_support[fid]) {
> > +return NVME_INVALID_FIELD | NVME_DNR;
> > +}
> > +
> > +switch (fid) {
> > case NVME_TEMPERATURE_THRESHOLD:
> > result = 0;
> >
> > @@ -1089,14 +1097,27 @@ static uint16_t nvme_get_feature(NvmeCtrl *n,
> > NvmeCmd *cmd, NvmeRequest *req)
> > ((n->params.max_ioqpairs - 1) << 16));
> > trace_pci_nvme_getfeat_numq(result);
> > break;
> > +case NVME_INTERRUPT_VECTOR_CONF:
> > +iv = dw11 & 0x;
> > +if (iv >= n->params.max_ioqpairs + 1) {
> > +return NVME_INVALID_FIELD | NVME_DNR;
> > +}
> > +
> > +result = iv;
> > +if (iv == n->admin_cq.vector) {
> > +result |= NVME_INTVC_NOCOALESCING;
> > +}
> > +
> > +result = cpu_to_le32(result);
> > +break;
> > case NVME_ASYNCHRONOUS_EVENT_CONF:
> > result = cpu_to_le32(n->features.async_config);
> > break;
> > case NVME_TIMESTAMP:
> > return nvme_get_feature_timestamp(n, cmd);
> > default:
> > -trace_pci_nvme_err_invalid_getfeat(dw10);
> > -return NVME_INVALID_FIELD | NVME_DNR;
> > +result = cpu_to_le32(nvme_feature_default[fid]);
> > +break;
> > }
> >
> > req->cqe.result = result;
> > @@ -1125,8 +1146,15 @@ static uint16_t nvme_set_feature(NvmeCtrl *n,
> > NvmeCmd *cmd, NvmeRequest *req)
> > {
> > uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> > uint32_t dw11 = le32_to_cpu(cmd->cdw11);
> > +uint8_t fid = NVME_GETSETFEAT_FID(dw10);
> >
> > -switch (dw10) {
> > +trace_pci_nvme_setfeat(nvme_cid(req), fid, dw11);
> > +
> > +if (!nvme_feature_support[fid]) {
> > +return NVME_INVALID_FIELD | NVME_DNR;
> > +}
> > +
> > +switch (fid) {
> > case NVME_TEMPERATURE_THRESHOLD:
> > if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
> > break;
> > @@ -1173,8 +1201,7 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd
> > *cmd, NvmeRequest *req)
> > case NVME_TIMESTAMP:
> > return nvme_set_feature_timestamp(n, cmd);
> > default:
> > -trace_pci_nvme_err_invalid_setfeat(dw10);
> > -return NVME_INVALID_FIELD | NVME_DNR;
> > +return NVME_FEAT_NOT_CHANGABLE | NVME_DNR;
>
> In spec, it is "Changeable", could as well add that 'e' here
Good catch, typo fixed.
>
> > }
> > return NVME_SUCCESS;
> > }
> > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > index 16a254d30b4e..d0763eb59e5d 100644
> > --- a/hw/block/nvme.h
> > +++ b/hw/block/nvme.h
> > @@ -90,6 +90,24 @@ typedef struct NvmeFeatureVal {
> > uint32_tasync_config;
> > } NvmeFeatureVal;
> >
> > +static const uint32_t nvme_feature_default[0x100] = {
> > +[NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
> > +};
> > +
> > +static const bool nvme_feature_support[0x100] = {
> > +[NVME_ARBITRATION] = true,
> > +[NVME_POWER_MANAGEMENT] = true,
> > +[NVME_TEMPERATURE_THRESHOLD]= true,
> > +[NVME_ERROR_RECOVERY] = true,
> > +[NVME_VOLATILE_WRITE_CACHE] = true,
> > +[NVME_NUMBER_OF_QUEUES] = true,
> > +[NVME_INTERRUPT_COALESCING] = true,
> > +[NVME_INTERRUPT_VECTOR_CONF]= true,
> > +[NVME_WRITE_ATOMICITY] = true,
> > +[NVME_ASYNCHRONOUS_EVENT_CONF] = true,
> > +[NVME_TIMESTAMP]= true,
> > +};
> > +
> > typedef struct NvmeCtrl {
> > PCIDeviceparent_obj;
> > MemoryRegion iomem;
> > diff --git a/hw/block/trace-events b/hw/block/trace-events
> > index 091af16ca7d7..42e62f4649f8 100644
> > --- a/hw/block/trace-events
> > +++ b/hw/block/trace-events
> > @@ -46,6 +46,8 @@ pci_nvme_identify_ctrl(void) "identify controller"
> >
Re: [PATCH 11/17] hw/block/nvme: add remaining mandatory controller parameters
LGTM with one small nit (see below)...
Reviewed-by: Dmitry Fomichev
On Mon, 2020-06-29 at 20:26 +0200, Klaus Jensen wrote:
> From: Klaus Jensen
>
> Add support for any remaining mandatory controller operating parameters
> (features).
>
> Signed-off-by: Klaus Jensen
> ---
> hw/block/nvme.c | 39 +--
> hw/block/nvme.h | 18 ++
> hw/block/trace-events | 2 ++
> include/block/nvme.h | 7 +++
> 4 files changed, 60 insertions(+), 6 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index da13ca1ddb60..647f408854ae 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1057,8 +1057,16 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
> *cmd, NvmeRequest *req)
> uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> uint32_t dw11 = le32_to_cpu(cmd->cdw11);
> uint32_t result;
> +uint8_t fid = NVME_GETSETFEAT_FID(dw10);
> +uint16_t iv;
>
> -switch (dw10) {
> +trace_pci_nvme_getfeat(nvme_cid(req), fid, dw11);
> +
> +if (!nvme_feature_support[fid]) {
> +return NVME_INVALID_FIELD | NVME_DNR;
> +}
> +
> +switch (fid) {
> case NVME_TEMPERATURE_THRESHOLD:
> result = 0;
>
> @@ -1089,14 +1097,27 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
> *cmd, NvmeRequest *req)
> ((n->params.max_ioqpairs - 1) << 16));
> trace_pci_nvme_getfeat_numq(result);
> break;
> +case NVME_INTERRUPT_VECTOR_CONF:
> +iv = dw11 & 0x;
> +if (iv >= n->params.max_ioqpairs + 1) {
> +return NVME_INVALID_FIELD | NVME_DNR;
> +}
> +
> +result = iv;
> +if (iv == n->admin_cq.vector) {
> +result |= NVME_INTVC_NOCOALESCING;
> +}
> +
> +result = cpu_to_le32(result);
> +break;
> case NVME_ASYNCHRONOUS_EVENT_CONF:
> result = cpu_to_le32(n->features.async_config);
> break;
> case NVME_TIMESTAMP:
> return nvme_get_feature_timestamp(n, cmd);
> default:
> -trace_pci_nvme_err_invalid_getfeat(dw10);
> -return NVME_INVALID_FIELD | NVME_DNR;
> +result = cpu_to_le32(nvme_feature_default[fid]);
> +break;
> }
>
> req->cqe.result = result;
> @@ -1125,8 +1146,15 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd
> *cmd, NvmeRequest *req)
> {
> uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> uint32_t dw11 = le32_to_cpu(cmd->cdw11);
> +uint8_t fid = NVME_GETSETFEAT_FID(dw10);
>
> -switch (dw10) {
> +trace_pci_nvme_setfeat(nvme_cid(req), fid, dw11);
> +
> +if (!nvme_feature_support[fid]) {
> +return NVME_INVALID_FIELD | NVME_DNR;
> +}
> +
> +switch (fid) {
> case NVME_TEMPERATURE_THRESHOLD:
> if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
> break;
> @@ -1173,8 +1201,7 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd
> *cmd, NvmeRequest *req)
> case NVME_TIMESTAMP:
> return nvme_set_feature_timestamp(n, cmd);
> default:
> -trace_pci_nvme_err_invalid_setfeat(dw10);
> -return NVME_INVALID_FIELD | NVME_DNR;
> +return NVME_FEAT_NOT_CHANGABLE | NVME_DNR;
In spec, it is "Changeable", could as well add that 'e' here
> }
> return NVME_SUCCESS;
> }
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index 16a254d30b4e..d0763eb59e5d 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -90,6 +90,24 @@ typedef struct NvmeFeatureVal {
> uint32_tasync_config;
> } NvmeFeatureVal;
>
> +static const uint32_t nvme_feature_default[0x100] = {
> +[NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
> +};
> +
> +static const bool nvme_feature_support[0x100] = {
> +[NVME_ARBITRATION] = true,
> +[NVME_POWER_MANAGEMENT] = true,
> +[NVME_TEMPERATURE_THRESHOLD]= true,
> +[NVME_ERROR_RECOVERY] = true,
> +[NVME_VOLATILE_WRITE_CACHE] = true,
> +[NVME_NUMBER_OF_QUEUES] = true,
> +[NVME_INTERRUPT_COALESCING] = true,
> +[NVME_INTERRUPT_VECTOR_CONF]= true,
> +[NVME_WRITE_ATOMICITY] = true,
> +[NVME_ASYNCHRONOUS_EVENT_CONF] = true,
> +[NVME_TIMESTAMP]= true,
> +};
> +
> typedef struct NvmeCtrl {
> PCIDeviceparent_obj;
> MemoryRegion iomem;
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index 091af16ca7d7..42e62f4649f8 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -46,6 +46,8 @@ pci_nvme_identify_ctrl(void) "identify controller"
> pci_nvme_identify_ns(uint32_t ns) "nsid %"PRIu32""
> pci_nvme_identify_nslist(uint32_t ns) "nsid %"PRIu32""
> pci_nvme_get_log(uint16_t cid, uint8_t lid, uint8_t lsp, uint8_t rae,
> uint32_t len, uint64_t off) "cid %"PRIu16" lid 0x%"PRIx8" lsp 0x%"PRIx8" rae
> 0x%"PRIx8" len %"PRIu32" off %"PRIu64""
> +pci_nvme_getfeat(uint16_t cid, uint
[PATCH 11/17] hw/block/nvme: add remaining mandatory controller parameters
From: Klaus Jensen
Add support for any remaining mandatory controller operating parameters
(features).
Signed-off-by: Klaus Jensen
---
hw/block/nvme.c | 39 +--
hw/block/nvme.h | 18 ++
hw/block/trace-events | 2 ++
include/block/nvme.h | 7 +++
4 files changed, 60 insertions(+), 6 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index da13ca1ddb60..647f408854ae 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1057,8 +1057,16 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
*cmd, NvmeRequest *req)
uint32_t dw10 = le32_to_cpu(cmd->cdw10);
uint32_t dw11 = le32_to_cpu(cmd->cdw11);
uint32_t result;
+uint8_t fid = NVME_GETSETFEAT_FID(dw10);
+uint16_t iv;
-switch (dw10) {
+trace_pci_nvme_getfeat(nvme_cid(req), fid, dw11);
+
+if (!nvme_feature_support[fid]) {
+return NVME_INVALID_FIELD | NVME_DNR;
+}
+
+switch (fid) {
case NVME_TEMPERATURE_THRESHOLD:
result = 0;
@@ -1089,14 +1097,27 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
*cmd, NvmeRequest *req)
((n->params.max_ioqpairs - 1) << 16));
trace_pci_nvme_getfeat_numq(result);
break;
+case NVME_INTERRUPT_VECTOR_CONF:
+iv = dw11 & 0x;
+if (iv >= n->params.max_ioqpairs + 1) {
+return NVME_INVALID_FIELD | NVME_DNR;
+}
+
+result = iv;
+if (iv == n->admin_cq.vector) {
+result |= NVME_INTVC_NOCOALESCING;
+}
+
+result = cpu_to_le32(result);
+break;
case NVME_ASYNCHRONOUS_EVENT_CONF:
result = cpu_to_le32(n->features.async_config);
break;
case NVME_TIMESTAMP:
return nvme_get_feature_timestamp(n, cmd);
default:
-trace_pci_nvme_err_invalid_getfeat(dw10);
-return NVME_INVALID_FIELD | NVME_DNR;
+result = cpu_to_le32(nvme_feature_default[fid]);
+break;
}
req->cqe.result = result;
@@ -1125,8 +1146,15 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd
*cmd, NvmeRequest *req)
{
uint32_t dw10 = le32_to_cpu(cmd->cdw10);
uint32_t dw11 = le32_to_cpu(cmd->cdw11);
+uint8_t fid = NVME_GETSETFEAT_FID(dw10);
-switch (dw10) {
+trace_pci_nvme_setfeat(nvme_cid(req), fid, dw11);
+
+if (!nvme_feature_support[fid]) {
+return NVME_INVALID_FIELD | NVME_DNR;
+}
+
+switch (fid) {
case NVME_TEMPERATURE_THRESHOLD:
if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
break;
@@ -1173,8 +1201,7 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd
*cmd, NvmeRequest *req)
case NVME_TIMESTAMP:
return nvme_set_feature_timestamp(n, cmd);
default:
-trace_pci_nvme_err_invalid_setfeat(dw10);
-return NVME_INVALID_FIELD | NVME_DNR;
+return NVME_FEAT_NOT_CHANGABLE | NVME_DNR;
}
return NVME_SUCCESS;
}
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 16a254d30b4e..d0763eb59e5d 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -90,6 +90,24 @@ typedef struct NvmeFeatureVal {
uint32_tasync_config;
} NvmeFeatureVal;
+static const uint32_t nvme_feature_default[0x100] = {
+[NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
+};
+
+static const bool nvme_feature_support[0x100] = {
+[NVME_ARBITRATION] = true,
+[NVME_POWER_MANAGEMENT] = true,
+[NVME_TEMPERATURE_THRESHOLD]= true,
+[NVME_ERROR_RECOVERY] = true,
+[NVME_VOLATILE_WRITE_CACHE] = true,
+[NVME_NUMBER_OF_QUEUES] = true,
+[NVME_INTERRUPT_COALESCING] = true,
+[NVME_INTERRUPT_VECTOR_CONF]= true,
+[NVME_WRITE_ATOMICITY] = true,
+[NVME_ASYNCHRONOUS_EVENT_CONF] = true,
+[NVME_TIMESTAMP]= true,
+};
+
typedef struct NvmeCtrl {
PCIDeviceparent_obj;
MemoryRegion iomem;
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 091af16ca7d7..42e62f4649f8 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -46,6 +46,8 @@ pci_nvme_identify_ctrl(void) "identify controller"
pci_nvme_identify_ns(uint32_t ns) "nsid %"PRIu32""
pci_nvme_identify_nslist(uint32_t ns) "nsid %"PRIu32""
pci_nvme_get_log(uint16_t cid, uint8_t lid, uint8_t lsp, uint8_t rae, uint32_t
len, uint64_t off) "cid %"PRIu16" lid 0x%"PRIx8" lsp 0x%"PRIx8" rae 0x%"PRIx8"
len %"PRIu32" off %"PRIu64""
+pci_nvme_getfeat(uint16_t cid, uint8_t fid, uint32_t cdw11) "cid %"PRIu16" fid
0x%"PRIx8" cdw11 0x%"PRIx32""
+pci_nvme_setfeat(uint16_t cid, uint8_t fid, uint32_t cdw11) "cid %"PRIu16" fid
0x%"PRIx8" cdw11 0x%"PRIx32""
pci_nvme_getfeat_vwcache(const char* result) "get feature volatile write
cache, result=%s"
pci_nvme_getfeat_numq(int result) "get feature number of queues, result=%d"
pci_nvme_setfeat_numq(int reqcq, int reqsq, int gotcq, int gotsq) "requested
cq_count=%d sq_count=%d, r
