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

[Klaus Jensen]

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.

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

signature.asc
Description: PGP signature