Re: [PATCH v2 05/18] hw/block/nvme: Introduce the Namespace Types definitions

[Niklas Cassel <Niklas.Cassel@wdc.com>]

On Mon, Jun 29, 2020 at 07:12:47PM -0700, Alistair Francis wrote:
> On Wed, Jun 17, 2020 at 2:47 PM Dmitry Fomichev <dmitry.fomichev@wdc.com> wrote:
> >
> > From: Niklas Cassel <niklas.cassel@wdc.com>
> >
> > Define the structures and constants required to implement
> > Namespace Types support.
> >
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> > ---
> >  hw/block/nvme.h      |  3 ++
> >  include/block/nvme.h | 75 +++++++++++++++++++++++++++++++++++++++++---
> >  2 files changed, 73 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > index 4f0dac39ae..4fd155c409 100644
> > --- a/hw/block/nvme.h
> > +++ b/hw/block/nvme.h
> > @@ -63,6 +63,9 @@ typedef struct NvmeCQueue {
> >
> >  typedef struct NvmeNamespace {
> >      NvmeIdNs        id_ns;
> > +    uint32_t        nsid;
> > +    uint8_t         csi;
> > +    QemuUUID        uuid;
> >  } NvmeNamespace;
> >
> >  static inline NvmeLBAF *nvme_ns_lbaf(NvmeNamespace *ns)
> > diff --git a/include/block/nvme.h b/include/block/nvme.h
> > index 6a58bac0c2..5a1e5e137c 100644
> > --- a/include/block/nvme.h
> > +++ b/include/block/nvme.h
> > @@ -50,6 +50,11 @@ enum NvmeCapMask {
> >      CAP_PMR_MASK       = 0x1,
> >  };
> >
> > +enum NvmeCapCssBits {
> > +    CAP_CSS_NVM        = 0x01,
> > +    CAP_CSS_CSI_SUPP   = 0x40,
> > +};
> > +
> >  #define NVME_CAP_MQES(cap)  (((cap) >> CAP_MQES_SHIFT)   & CAP_MQES_MASK)
> >  #define NVME_CAP_CQR(cap)   (((cap) >> CAP_CQR_SHIFT)    & CAP_CQR_MASK)
> >  #define NVME_CAP_AMS(cap)   (((cap) >> CAP_AMS_SHIFT)    & CAP_AMS_MASK)
> > @@ -101,6 +106,12 @@ enum NvmeCcMask {
> >      CC_IOCQES_MASK  = 0xf,
> >  };
> >
> > +enum NvmeCcCss {
> > +    CSS_NVM_ONLY        = 0,
> > +    CSS_ALL_NSTYPES     = 6,
> > +    CSS_ADMIN_ONLY      = 7,
> > +};
> > +
> >  #define NVME_CC_EN(cc)     ((cc >> CC_EN_SHIFT)     & CC_EN_MASK)
> >  #define NVME_CC_CSS(cc)    ((cc >> CC_CSS_SHIFT)    & CC_CSS_MASK)
> >  #define NVME_CC_MPS(cc)    ((cc >> CC_MPS_SHIFT)    & CC_MPS_MASK)
> > @@ -109,6 +120,21 @@ enum NvmeCcMask {
> >  #define NVME_CC_IOSQES(cc) ((cc >> CC_IOSQES_SHIFT) & CC_IOSQES_MASK)
> >  #define NVME_CC_IOCQES(cc) ((cc >> CC_IOCQES_SHIFT) & CC_IOCQES_MASK)
> >
> > +#define NVME_SET_CC_EN(cc, val)     \
> > +    (cc |= (uint32_t)((val) & CC_EN_MASK) << CC_EN_SHIFT)
> > +#define NVME_SET_CC_CSS(cc, val)    \
> > +    (cc |= (uint32_t)((val) & CC_CSS_MASK) << CC_CSS_SHIFT)
> > +#define NVME_SET_CC_MPS(cc, val)    \
> > +    (cc |= (uint32_t)((val) & CC_MPS_MASK) << CC_MPS_SHIFT)
> > +#define NVME_SET_CC_AMS(cc, val)    \
> > +    (cc |= (uint32_t)((val) & CC_AMS_MASK) << CC_AMS_SHIFT)
> > +#define NVME_SET_CC_SHN(cc, val)    \
> > +    (cc |= (uint32_t)((val) & CC_SHN_MASK) << CC_SHN_SHIFT)
> > +#define NVME_SET_CC_IOSQES(cc, val) \
> > +    (cc |= (uint32_t)((val) & CC_IOSQES_MASK) << CC_IOSQES_SHIFT)
> > +#define NVME_SET_CC_IOCQES(cc, val) \
> > +    (cc |= (uint32_t)((val) & CC_IOCQES_MASK) << CC_IOCQES_SHIFT)
> > +
> >  enum NvmeCstsShift {
> >      CSTS_RDY_SHIFT      = 0,
> >      CSTS_CFS_SHIFT      = 1,
> > @@ -482,10 +508,41 @@ typedef struct NvmeIdentify {
> >      uint64_t    rsvd2[2];
> >      uint64_t    prp1;
> >      uint64_t    prp2;
> > -    uint32_t    cns;
> > -    uint32_t    rsvd11[5];
> > +    uint8_t     cns;
> > +    uint8_t     rsvd4;
> > +    uint16_t    ctrlid;
> 
> Shouldn't this be CNTID?

From the NVMe spec:
https://nvmexpress.org/wp-content/uploads/NVM-Express-1_4-2019.06.10-Ratified.pdf

Figure 241:
Controller  Identifier  (CNTID)

So you are correct, this is the official abbreviation.

I guess that I tried wanted to keep it in sync with Linux:
https://github.com/torvalds/linux/blob/master/include/linux/nvme.h#L974

Which uses ctrlid.


Looking further at the NVMe spec:
In Figure 247 (Identify Controller Data Structure) they use other names
for fields:

Controller  ID  (CNTLID)
Controller Attributes (CTRATT)

I can understand if they want to have unique names for fields, but it
seems like they have trouble deciding how to abbreviate controller :)

Personally I think that ctrlid is more obvious that we are talking about
a controller and not a count. But I'm fine regardless.


Kind regards,
Niklas

> 
> Alistair
> 
> > +    uint16_t    nvmsetid;
> > +    uint8_t     rsvd3;
> > +    uint8_t     csi;
> > +    uint32_t    rsvd12[4];
> >  } NvmeIdentify;
> >
> > +typedef struct NvmeNsIdDesc {
> > +    uint8_t     nidt;
> > +    uint8_t     nidl;
> > +    uint16_t    rsvd2;
> > +} NvmeNsIdDesc;
> > +
> > +enum NvmeNidType {
> > +    NVME_NIDT_EUI64             = 0x01,
> > +    NVME_NIDT_NGUID             = 0x02,
> > +    NVME_NIDT_UUID              = 0x03,
> > +    NVME_NIDT_CSI               = 0x04,
> > +};
> > +
> > +enum NvmeNidLength {
> > +    NVME_NIDL_EUI64             = 8,
> > +    NVME_NIDL_NGUID             = 16,
> > +    NVME_NIDL_UUID              = 16,
> > +    NVME_NIDL_CSI               = 1,
> > +};
> > +
> > +enum NvmeCsi {
> > +    NVME_CSI_NVM                = 0x00,
> > +};
> > +
> > +#define NVME_SET_CSI(vec, csi) (vec |= (uint8_t)(1 << (csi)))
> > +
> >  typedef struct NvmeRwCmd {
> >      uint8_t     opcode;
> >      uint8_t     flags;
> > @@ -603,6 +660,7 @@ enum NvmeStatusCodes {
> >      NVME_CMD_ABORT_MISSING_FUSE = 0x000a,
> >      NVME_INVALID_NSID           = 0x000b,
> >      NVME_CMD_SEQ_ERROR          = 0x000c,
> > +    NVME_CMD_SET_CMB_REJECTED   = 0x002b,
> >      NVME_LBA_RANGE              = 0x0080,
> >      NVME_CAP_EXCEEDED           = 0x0081,
> >      NVME_NS_NOT_READY           = 0x0082,
> > @@ -729,9 +787,14 @@ typedef struct NvmePSD {
> >  #define NVME_IDENTIFY_DATA_SIZE 4096
> >
> >  enum {
> > -    NVME_ID_CNS_NS             = 0x0,
> > -    NVME_ID_CNS_CTRL           = 0x1,
> > -    NVME_ID_CNS_NS_ACTIVE_LIST = 0x2,
> > +    NVME_ID_CNS_NS                = 0x0,
> > +    NVME_ID_CNS_CTRL              = 0x1,
> > +    NVME_ID_CNS_NS_ACTIVE_LIST    = 0x2,
> > +    NVME_ID_CNS_NS_DESC_LIST      = 0x03,
> > +    NVME_ID_CNS_CS_NS             = 0x05,
> > +    NVME_ID_CNS_CS_CTRL           = 0x06,
> > +    NVME_ID_CNS_CS_NS_ACTIVE_LIST = 0x07,
> > +    NVME_ID_CNS_IO_COMMAND_SET    = 0x1c,
> >  };
> >
> >  typedef struct NvmeIdCtrl {
> > @@ -825,6 +888,7 @@ enum NvmeFeatureIds {
> >      NVME_WRITE_ATOMICITY            = 0xa,
> >      NVME_ASYNCHRONOUS_EVENT_CONF    = 0xb,
> >      NVME_TIMESTAMP                  = 0xe,
> > +    NVME_COMMAND_SET_PROFILE        = 0x19,
> >      NVME_SOFTWARE_PROGRESS_MARKER   = 0x80
> >  };
> >
> > @@ -914,6 +978,7 @@ static inline void _nvme_check_size(void)
> >      QEMU_BUILD_BUG_ON(sizeof(NvmeFwSlotInfoLog) != 512);
> >      QEMU_BUILD_BUG_ON(sizeof(NvmeSmartLog) != 512);
> >      QEMU_BUILD_BUG_ON(sizeof(NvmeIdCtrl) != 4096);
> > +    QEMU_BUILD_BUG_ON(sizeof(NvmeNsIdDesc) != 4);
> >      QEMU_BUILD_BUG_ON(sizeof(NvmeIdNs) != 4096);
> >      QEMU_BUILD_BUG_ON(sizeof(NvmeEffectsLog) != 4096);
> >  }
> > --
> > 2.21.0
> >
> >