[v2,11/18] hw/block/nvme: add remaining mandatory controller parameters
diff mbox series

Message ID 20200703063420.2241014-12-its@irrelevant.dk
State New
Headers show
Series
  • hw/block/nvme: bump to v1.3
Related show

Commit Message

Klaus Jensen July 3, 2020, 6:34 a.m. UTC
From: Klaus Jensen <k.jensen@samsung.com>

Add support for any remaining mandatory controller operating parameters
(features).

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Dmitry Fomichev <dmitry.fomichev@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(-)

Comments

Philippe Mathieu-Daudé July 3, 2020, 8:31 a.m. UTC | #1
On 7/3/20 8:34 AM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Add support for any remaining mandatory controller operating parameters
> (features).
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> Reviewed-by: Dmitry Fomichev <dmitry.fomichev@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?

> +        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;
>  
> +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,
> +};

Nack. No variable assignation in header please.
Move that to the source file.

> +
>  typedef struct NvmeCtrl {
>      PCIDevice    parent_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, responding with cq_count=%d sq_count=%d"
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 0dce15af6bcf..406648d4cf94 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -662,6 +662,7 @@ enum NvmeStatusCodes {
>      NVME_FW_REQ_RESET           = 0x010b,
>      NVME_INVALID_QUEUE_DEL      = 0x010c,
>      NVME_FID_NOT_SAVEABLE       = 0x010d,
> +    NVME_FEAT_NOT_CHANGEABLE    = 0x010e,
>      NVME_FID_NOT_NSID_SPEC      = 0x010f,
>      NVME_FW_REQ_SUSYSTEM_RESET  = 0x0110,
>      NVME_CONFLICTING_ATTRS      = 0x0180,
> @@ -866,6 +867,7 @@ enum NvmeIdCtrlLpa {
>  #define NVME_CTRL_SGLS_ADDR_OFFSET         (0x1 << 20)
>  
>  #define NVME_ARB_AB(arb)    (arb & 0x7)
> +#define NVME_ARB_AB_NOLIMIT 0x7
>  #define NVME_ARB_LPW(arb)   ((arb >> 8) & 0xff)
>  #define NVME_ARB_MPW(arb)   ((arb >> 16) & 0xff)
>  #define NVME_ARB_HPW(arb)   ((arb >> 24) & 0xff)
> @@ -873,6 +875,8 @@ enum NvmeIdCtrlLpa {
>  #define NVME_INTC_THR(intc)     (intc & 0xff)
>  #define NVME_INTC_TIME(intc)    ((intc >> 8) & 0xff)
>  
> +#define NVME_INTVC_NOCOALESCING (0x1 << 16)
> +
>  #define NVME_TEMP_THSEL(temp)  ((temp >> 20) & 0x3)
>  #define NVME_TEMP_THSEL_OVER   0x0
>  #define NVME_TEMP_THSEL_UNDER  0x1
> @@ -902,6 +906,9 @@ enum NvmeFeatureIds {
>      NVME_SOFTWARE_PROGRESS_MARKER   = 0x80
>  };
>  
> +#define NVME_GETSETFEAT_FID_MASK 0xff
> +#define NVME_GETSETFEAT_FID(dw10) (dw10 & NVME_GETSETFEAT_FID_MASK)
> +
>  typedef struct NvmeRangeType {
>      uint8_t     type;
>      uint8_t     attributes;
>
Klaus Jensen July 3, 2020, 8:46 a.m. UTC | #2
On Jul  3 10:31, Philippe Mathieu-Daudé wrote:
> On 7/3/20 8:34 AM, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Add support for any remaining mandatory controller operating parameters
> > (features).
> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > Reviewed-by: Dmitry Fomichev <dmitry.fomichev@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;
> >  
> > +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,
> > +};
> 
> Nack. No variable assignation in header please.
> Move that to the source file.
> 

Understood :)

> > +
> >  typedef struct NvmeCtrl {
> >      PCIDevice    parent_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, responding with cq_count=%d sq_count=%d"
> > diff --git a/include/block/nvme.h b/include/block/nvme.h
> > index 0dce15af6bcf..406648d4cf94 100644
> > --- a/include/block/nvme.h
> > +++ b/include/block/nvme.h
> > @@ -662,6 +662,7 @@ enum NvmeStatusCodes {
> >      NVME_FW_REQ_RESET           = 0x010b,
> >      NVME_INVALID_QUEUE_DEL      = 0x010c,
> >      NVME_FID_NOT_SAVEABLE       = 0x010d,
> > +    NVME_FEAT_NOT_CHANGEABLE    = 0x010e,
> >      NVME_FID_NOT_NSID_SPEC      = 0x010f,
> >      NVME_FW_REQ_SUSYSTEM_RESET  = 0x0110,
> >      NVME_CONFLICTING_ATTRS      = 0x0180,
> > @@ -866,6 +867,7 @@ enum NvmeIdCtrlLpa {
> >  #define NVME_CTRL_SGLS_ADDR_OFFSET         (0x1 << 20)
> >  
> >  #define NVME_ARB_AB(arb)    (arb & 0x7)
> > +#define NVME_ARB_AB_NOLIMIT 0x7
> >  #define NVME_ARB_LPW(arb)   ((arb >> 8) & 0xff)
> >  #define NVME_ARB_MPW(arb)   ((arb >> 16) & 0xff)
> >  #define NVME_ARB_HPW(arb)   ((arb >> 24) & 0xff)
> > @@ -873,6 +875,8 @@ enum NvmeIdCtrlLpa {
> >  #define NVME_INTC_THR(intc)     (intc & 0xff)
> >  #define NVME_INTC_TIME(intc)    ((intc >> 8) & 0xff)
> >  
> > +#define NVME_INTVC_NOCOALESCING (0x1 << 16)
> > +
> >  #define NVME_TEMP_THSEL(temp)  ((temp >> 20) & 0x3)
> >  #define NVME_TEMP_THSEL_OVER   0x0
> >  #define NVME_TEMP_THSEL_UNDER  0x1
> > @@ -902,6 +906,9 @@ enum NvmeFeatureIds {
> >      NVME_SOFTWARE_PROGRESS_MARKER   = 0x80
> >  };
> >  
> > +#define NVME_GETSETFEAT_FID_MASK 0xff
> > +#define NVME_GETSETFEAT_FID(dw10) (dw10 & NVME_GETSETFEAT_FID_MASK)
> > +
> >  typedef struct NvmeRangeType {
> >      uint8_t     type;
> >      uint8_t     attributes;
> > 
>
Philippe Mathieu-Daudé July 3, 2020, 9:21 a.m. UTC | #3
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.jensen@samsung.com>
>>>
>>> Add support for any remaining mandatory controller operating parameters
>>> (features).
>>>
>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>> Reviewed-by: Dmitry Fomichev <dmitry.fomichev@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
 };

>>>  
>>> +static const uint32_t nvme_feature_default[0x100] = {

Why uint32_t and not uint16_t?

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.

>>> +    [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,
>>> +};
>>
>> Nack. No variable assignation in header please.

Rationale is if another C source file include this header,
the array will be redeclared (statically) and eventually
unused.

>> Move that to the source file.
>>
> 
> Understood :)
> 
>>> +
>>>  typedef struct NvmeCtrl {
>>>      PCIDevice    parent_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, responding with cq_count=%d sq_count=%d"
>>> diff --git a/include/block/nvme.h b/include/block/nvme.h
>>> index 0dce15af6bcf..406648d4cf94 100644
>>> --- a/include/block/nvme.h
>>> +++ b/include/block/nvme.h
>>> @@ -662,6 +662,7 @@ enum NvmeStatusCodes {
>>>      NVME_FW_REQ_RESET           = 0x010b,
>>>      NVME_INVALID_QUEUE_DEL      = 0x010c,
>>>      NVME_FID_NOT_SAVEABLE       = 0x010d,
>>> +    NVME_FEAT_NOT_CHANGEABLE    = 0x010e,
>>>      NVME_FID_NOT_NSID_SPEC      = 0x010f,
>>>      NVME_FW_REQ_SUSYSTEM_RESET  = 0x0110,
>>>      NVME_CONFLICTING_ATTRS      = 0x0180,
>>> @@ -866,6 +867,7 @@ enum NvmeIdCtrlLpa {
>>>  #define NVME_CTRL_SGLS_ADDR_OFFSET         (0x1 << 20)
>>>  
>>>  #define NVME_ARB_AB(arb)    (arb & 0x7)
>>> +#define NVME_ARB_AB_NOLIMIT 0x7
>>>  #define NVME_ARB_LPW(arb)   ((arb >> 8) & 0xff)
>>>  #define NVME_ARB_MPW(arb)   ((arb >> 16) & 0xff)
>>>  #define NVME_ARB_HPW(arb)   ((arb >> 24) & 0xff)
>>> @@ -873,6 +875,8 @@ enum NvmeIdCtrlLpa {
>>>  #define NVME_INTC_THR(intc)     (intc & 0xff)
>>>  #define NVME_INTC_TIME(intc)    ((intc >> 8) & 0xff)
>>>  
>>> +#define NVME_INTVC_NOCOALESCING (0x1 << 16)
>>> +
>>>  #define NVME_TEMP_THSEL(temp)  ((temp >> 20) & 0x3)
>>>  #define NVME_TEMP_THSEL_OVER   0x0
>>>  #define NVME_TEMP_THSEL_UNDER  0x1
>>> @@ -902,6 +906,9 @@ enum NvmeFeatureIds {
>>>      NVME_SOFTWARE_PROGRESS_MARKER   = 0x80
>>>  };
>>>  
>>> +#define NVME_GETSETFEAT_FID_MASK 0xff
>>> +#define NVME_GETSETFEAT_FID(dw10) (dw10 & NVME_GETSETFEAT_FID_MASK)
>>> +
>>>  typedef struct NvmeRangeType {
>>>      uint8_t     type;
>>>      uint8_t     attributes;
>>>
>>
>
Klaus Jensen July 3, 2020, 10:10 a.m. UTC | #4
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.jensen@samsung.com>
> >>>
> >>> Add support for any remaining mandatory controller operating parameters
> >>> (features).
> >>>
> >>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> >>> Reviewed-by: Dmitry Fomichev <dmitry.fomichev@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!
Philippe Mathieu-Daudé July 3, 2020, 11:02 a.m. UTC | #5
On 7/3/20 12:10 PM, Klaus Jensen wrote:
> 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.jensen@samsung.com>
>>>>>
>>>>> Add support for any remaining mandatory controller operating parameters
>>>>> (features).
>>>>>
>>>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>>>> Reviewed-by: Dmitry Fomichev <dmitry.fomichev@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.

Good name.

> 
>>>>>  
>>>>> +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.

Can you prepend a new patch making nvme_get_feature() return uin32_t
instead of uint16_t? That would be clearer.

> 
>> 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!
>
Klaus Jensen July 3, 2020, 2:37 p.m. UTC | #6
On Jul  3 13:02, Philippe Mathieu-Daudé wrote:
> On 7/3/20 12:10 PM, Klaus Jensen wrote:
> > 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.jensen@samsung.com>
> >>>>>
> >>>>> Add support for any remaining mandatory controller operating parameters
> >>>>> (features).
> >>>>>
> >>>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> >>>>> Reviewed-by: Dmitry Fomichev <dmitry.fomichev@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.
> 
> Good name.
> 
> > 
> >>>>>  
> >>>>> +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.
> 
> Can you prepend a new patch making nvme_get_feature() return uin32_t
> instead of uint16_t? That would be clearer.
> 

Ah. Now I see where the confusion comes from ;)

nvme_get_feature() does not return the value of the feature. It returns
a uint16_t with a value from the NvmeStatusCodes enum (which really
should be a typedef used all over the place.
Philippe Mathieu-Daudé July 4, 2020, 9:33 p.m. UTC | #7
On 7/3/20 4:37 PM, Klaus Jensen wrote:
> On Jul  3 13:02, Philippe Mathieu-Daudé wrote:
>> On 7/3/20 12:10 PM, Klaus Jensen wrote:
>>> 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.jensen@samsung.com>
>>>>>>>
>>>>>>> Add support for any remaining mandatory controller operating parameters
>>>>>>> (features).
>>>>>>>
>>>>>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>>>>>> Reviewed-by: Dmitry Fomichev <dmitry.fomichev@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.
>>
>> Good name.
>>
>>>
>>>>>>>  
>>>>>>> +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.
>>
>> Can you prepend a new patch making nvme_get_feature() return uin32_t
>> instead of uint16_t? That would be clearer.
>>
> 
> Ah. Now I see where the confusion comes from ;)
> 
> nvme_get_feature() does not return the value of the feature. It returns
> a uint16_t with a value from the NvmeStatusCodes enum (which really
> should be a typedef used all over the place.

Ah! You are right, I misread it indeed :)

Patch
diff mbox series

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]);
+        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;
 
+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 {
     PCIDevice    parent_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, responding with cq_count=%d sq_count=%d"
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 0dce15af6bcf..406648d4cf94 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -662,6 +662,7 @@  enum NvmeStatusCodes {
     NVME_FW_REQ_RESET           = 0x010b,
     NVME_INVALID_QUEUE_DEL      = 0x010c,
     NVME_FID_NOT_SAVEABLE       = 0x010d,
+    NVME_FEAT_NOT_CHANGEABLE    = 0x010e,
     NVME_FID_NOT_NSID_SPEC      = 0x010f,
     NVME_FW_REQ_SUSYSTEM_RESET  = 0x0110,
     NVME_CONFLICTING_ATTRS      = 0x0180,
@@ -866,6 +867,7 @@  enum NvmeIdCtrlLpa {
 #define NVME_CTRL_SGLS_ADDR_OFFSET         (0x1 << 20)
 
 #define NVME_ARB_AB(arb)    (arb & 0x7)
+#define NVME_ARB_AB_NOLIMIT 0x7
 #define NVME_ARB_LPW(arb)   ((arb >> 8) & 0xff)
 #define NVME_ARB_MPW(arb)   ((arb >> 16) & 0xff)
 #define NVME_ARB_HPW(arb)   ((arb >> 24) & 0xff)
@@ -873,6 +875,8 @@  enum NvmeIdCtrlLpa {
 #define NVME_INTC_THR(intc)     (intc & 0xff)
 #define NVME_INTC_TIME(intc)    ((intc >> 8) & 0xff)
 
+#define NVME_INTVC_NOCOALESCING (0x1 << 16)
+
 #define NVME_TEMP_THSEL(temp)  ((temp >> 20) & 0x3)
 #define NVME_TEMP_THSEL_OVER   0x0
 #define NVME_TEMP_THSEL_UNDER  0x1
@@ -902,6 +906,9 @@  enum NvmeFeatureIds {
     NVME_SOFTWARE_PROGRESS_MARKER   = 0x80
 };
 
+#define NVME_GETSETFEAT_FID_MASK 0xff
+#define NVME_GETSETFEAT_FID(dw10) (dw10 & NVME_GETSETFEAT_FID_MASK)
+
 typedef struct NvmeRangeType {
     uint8_t     type;
     uint8_t     attributes;