Re: [RFC PATCH 3/3] hw/block/nvme: add nvme_inject_state HMP command

[Dr. David Alan Gilbert]

* Klaus Jensen (its@irrelevant.dk) wrote:
> On Feb 11 04:52, Minwoo Im wrote:
> > nvme_inject_state command is to give a controller state to be.
> > Human Monitor Interface(HMP) supports users to make controller to a
> > specified state of:
> > 
> >     normal:                 Normal state (no injection)
> >     cmd-interrupted:        Commands will be interrupted internally
> > 
> > This patch is just a start to give dynamic command from the HMP to the
> > QEMU NVMe device model.  If "cmd-interrupted" state is given, then the
> > controller will return all the CQ entries with Command Interrupts status
> > code.
> > 
> > Usage:
> >     -device nvme,id=nvme0,....
> > 
> >     (qemu) nvme_inject_state nvme0 cmd-interrupted
> > 
> >     <All the commands will be interrupted internally>
> > 
> >     (qemu) nvme_inject_state nvme0 normal
> > 
> > This feature is required to test Linux kernel NVMe driver for the
> > command retry feature.
> > 
> 
> This is super cool and commands like this feel much nicer than the
> qom-set approach that the SMART critical warning feature took.
> 
> But... looking at the existing commands I don't think we can "bloat" it
> up with a device specific command like this, but I don't know the policy
> around this.
> 
> If an HMP command is out, then we should be able to make do with the
> qom-set approach just fine though.

HMP is there to make life easy for Humans debugging; if it makes sense from an
NVMe perspective for test/debug then I'm OK with it from an HMP side.
Note that if it was for more common use than debug/test then you'd want
to make it go via QMP and make sure it was a stable interface that was
going to live for a longtime; but for test uses HMP is fine.

Dave

> > Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> > ---
> >  hmp-commands.hx       | 13 ++++++++++++
> >  hw/block/nvme.c       | 49 +++++++++++++++++++++++++++++++++++++++++++
> >  hw/block/nvme.h       |  8 +++++++
> >  include/monitor/hmp.h |  1 +
> >  4 files changed, 71 insertions(+)
> > 
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index d4001f9c5dc6..ef288c567b46 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -1307,6 +1307,19 @@ SRST
> >    Inject PCIe AER error
> >  ERST
> >  
> > +    {
> > +        .name       = "nvme_inject_state",
> > +        .args_type  = "id:s,state:s",
> > +        .params     = "id [normal|cmd-interrupted]",
> > +        .help       = "inject controller/namespace state",
> > +        .cmd        = hmp_nvme_inject_state,
> > +    },
> > +
> > +SRST
> > +``nvme_inject_state``
> > +  Inject NVMe controller/namespace state
> > +ERST
> > +
> >      {
> >          .name       = "netdev_add",
> >          .args_type  = "netdev:O",
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 6d3c554a0e99..42cf5bd113e6 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -123,6 +123,7 @@
> >  #include "sysemu/sysemu.h"
> >  #include "qapi/error.h"
> >  #include "qapi/visitor.h"
> > +#include "qapi/qmp/qdict.h"
> >  #include "sysemu/hostmem.h"
> >  #include "sysemu/block-backend.h"
> >  #include "exec/memory.h"
> > @@ -132,6 +133,7 @@
> >  #include "trace.h"
> >  #include "nvme.h"
> >  #include "nvme-ns.h"
> > +#include "monitor/monitor.h"
> >  
> >  #define NVME_MAX_IOQPAIRS 0xffff
> >  #define NVME_DB_SIZE  4
> > @@ -966,6 +968,14 @@ static void nvme_enqueue_req_completion(NvmeCQueue 
> > *cq, NvmeRequest *req)
> >  {
> >      assert(cq->cqid == req->sq->cqid);
> >  
> > +    /*
> > +     * Override request status field if controller state has been injected 
> > by
> > +     * the QMP.
> > +     */
> > +    if (cq->ctrl->state == NVME_STATE_CMD_INTERRUPTED) {
> > +        req->status = NVME_COMMAND_INTERRUPTED;
> > +    }
> > +
> >      if (req->status != NVME_SUCCESS) {
> >          if (cq->ctrl->features.acre && nvme_should_retry(req)) {
> >              if (cq->ctrl->params.cmd_retry_delay > 0) {
> > @@ -5025,4 +5035,43 @@ static void nvme_register_types(void)
> >      type_register_static(&nvme_bus_info);
> >  }
> >  
> > +static void nvme_inject_state(NvmeCtrl *n, NvmeState state)
> > +{
> > +    n->state = state;
> > +}
> > +
> > +static const char *nvme_states[] = {
> > +    [NVME_STATE_NORMAL]             = "normal",
> > +    [NVME_STATE_CMD_INTERRUPTED]    = "cmd-interrupted",
> > +};
> > +
> > +void hmp_nvme_inject_state(Monitor *mon, const QDict *qdict)
> > +{
> > +    const char *id = qdict_get_str(qdict, "id");
> > +    const char *state = qdict_get_str(qdict, "state");
> > +    PCIDevice *dev;
> > +    NvmeCtrl *n;
> > +    int ret, i;
> > +
> > +    ret = pci_qdev_find_device(id, &dev);
> > +    if (ret < 0) {
> > +        monitor_printf(mon, "invalid device id %s\n", id);
> > +        return;
> > +    }
> > +
> > +    n = NVME(dev);
> > +
> > +    for (i = 0; i < ARRAY_SIZE(nvme_states); i++) {
> > +        if (!strcmp(nvme_states[i], state)) {
> > +            nvme_inject_state(n, i);
> > +            monitor_printf(mon,
> > +                           "-device nvme,id=%s: state %s injected\n",
> > +                           id, state);
> > +            return;
> > +        }
> > +    }
> > +
> > +    monitor_printf(mon, "invalid state %s\n", state);
> > +}
> > +
> >  type_init(nvme_register_types)
> > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > index 37940b3ac2d2..1af1e0380d9b 100644
> > --- a/hw/block/nvme.h
> > +++ b/hw/block/nvme.h
> > @@ -128,6 +128,11 @@ typedef struct NvmeFeatureVal {
> >      uint8_t     acre;
> >  } NvmeFeatureVal;
> >  
> > +typedef enum NvmeState {
> > +    NVME_STATE_NORMAL,
> > +    NVME_STATE_CMD_INTERRUPTED,
> > +} NvmeState;
> > +
> >  typedef struct NvmeCtrl {
> >      PCIDevice    parent_obj;
> >      MemoryRegion bar0;
> > @@ -185,6 +190,8 @@ typedef struct NvmeCtrl {
> >      NvmeCQueue      admin_cq;
> >      NvmeIdCtrl      id_ctrl;
> >      NvmeFeatureVal  features;
> > +
> > +    NvmeState       state;
> >  } NvmeCtrl;
> >  
> >  static inline NvmeNamespace *nvme_ns(NvmeCtrl *n, uint32_t nsid)
> > @@ -212,4 +219,5 @@ static inline NvmeCtrl *nvme_ctrl(NvmeRequest *req)
> >  
> >  int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp);
> >  
> > +void hmp_nvme_inject_state(Monitor *mon, const QDict *qdict);
> >  #endif /* HW_NVME_H */
> > diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> > index ed2913fd18e8..668384ea2e34 100644
> > --- a/include/monitor/hmp.h
> > +++ b/include/monitor/hmp.h
> > @@ -79,6 +79,7 @@ void hmp_migrate(Monitor *mon, const QDict *qdict);
> >  void hmp_device_add(Monitor *mon, const QDict *qdict);
> >  void hmp_device_del(Monitor *mon, const QDict *qdict);
> >  void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
> > +void hmp_nvme_inject_state(Monitor *mon, const QDict *qdict);
> >  void hmp_netdev_add(Monitor *mon, const QDict *qdict);
> >  void hmp_netdev_del(Monitor *mon, const QDict *qdict);
> >  void hmp_getfd(Monitor *mon, const QDict *qdict);
> > -- 
> > 2.17.1
> > 
> 
> -- 
> One of us - No more doubt, silence or taboo about mental illness.


-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK