On Jul 3 11:21, Philippe Mathieu-Daudé wrote:
> On 7/3/20 10:46 AM, Klaus Jensen wrote:
> > On Jul 3 10:31, Philippe Mathieu-Daudé wrote:
> >> On 7/3/20 8:34 AM, Klaus Jensen wrote:
> >>> From: Klaus Jensen <k.jen...@samsung.com>
> >>>
> >>> Add support for any remaining mandatory controller operating parameters
> >>> (features).
> >>>
> >>> Signed-off-by: Klaus Jensen <k.jen...@samsung.com>
> >>> Reviewed-by: Dmitry Fomichev <dmitry.fomic...@wdc.com>
> >>> ---
> >>> 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 ba523f6768bf..affb9a967534 100644
> >>> --- a/hw/block/nvme.c
> >>> +++ b/hw/block/nvme.c
> >>> @@ -1056,8 +1056,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;
> >>>
> >>> @@ -1088,14 +1096,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 & 0xffff;
> >>> + 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]);
> >>
> >> So here we expect uninitialized fid entries to return 0, right?
> >>
> >
> > Yes, if defaults are not 0 (like NVME_ARBITRATION), it is explicitly set.
> >
> >>> + break;
> >>> }
> >>>
> >>> req->cqe.result = result;
> >>> @@ -1124,8 +1145,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;
> >>> @@ -1172,8 +1200,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_CHANGEABLE | NVME_DNR;
> >>> }
> >>> return NVME_SUCCESS;
> >>> }
> >>> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> >>> index f8940435f9ef..8ad1e3c89cee 100644
> >>> --- a/hw/block/nvme.h
> >>> +++ b/hw/block/nvme.h
> >>> @@ -87,6 +87,24 @@ typedef struct NvmeFeatureVal {
> >>> uint32_t async_config;
> >>> } NvmeFeatureVal;
>
> What do you think about adding:
>
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -804,7 +804,8 @@ enum NvmeFeatureIds {
> NVME_WRITE_ATOMICITY = 0xa,
> NVME_ASYNCHRONOUS_EVENT_CONF = 0xb,
> NVME_TIMESTAMP = 0xe,
> - NVME_SOFTWARE_PROGRESS_MARKER = 0x80
> + NVME_SOFTWARE_PROGRESS_MARKER = 0x80,
> + NVME_FEATURE_ID_COUNT = 0x100
> };
>
I can definitely go with that. I added it as NVME_FID_MAX.
> >>>
> >>> +static const uint32_t nvme_feature_default[0x100] = {
>
> Why uint32_t and not uint16_t?
>
The feature values are by definition 32 bits wide, so even though the
defaults here do not require it, I think uin32_t should be used.
> With the previously suggested enum you can now replace 0x100
> by NVME_FEATURE_ID_COUNT.
>
> >>> + [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
> >>> +};
> >>> +
> >>> +static const bool nvme_feature_support[0x100] = {
>
> Ditto NVME_FEATURE_ID_COUNT.
>
Fixed!
