diff mbox series

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

Message ID 20210210195252.19339-4-minwoo.im.dev@gmail.com (mailing list archive)
State New, archived
Headers show
Series support command retry | expand

Commit Message

Minwoo Im Feb. 10, 2021, 7:52 p.m. UTC
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.

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(+)

Comments

Klaus Jensen Feb. 10, 2021, 8:33 p.m. UTC | #1
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
>
Keith Busch Feb. 11, 2021, 3 a.m. UTC | #2
On Thu, Feb 11, 2021 at 04:52:52AM +0900, 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.

Once the user sets the injected state, all commands return that status
until the user injects the normal state, so the CRD time is meaningless
here. If we're really going this route, the state needs to return to
normal on it's own.

But I would prefer to see advanced retry tied to real errors that can be
retried, like if we got an EBUSY or EAGAIN errno or something like that.

The interface you found to implement this is very interesting though.
Minwoo Im Feb. 11, 2021, 3:23 a.m. UTC | #3
On 21-02-10 21:33:50, Klaus Jensen 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.

This interface is super easy to inject some errors to the running
device with a function call-back.

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

Me neither, but I've seen the PCI AER error injection feature from
the existing commands, so I suggested this command to control the
NVMe device itself like error injection.

> If an HMP command is out, then we should be able to make do with the
> qom-set approach just fine though.

Hope so.
Minwoo Im Feb. 11, 2021, 3:38 a.m. UTC | #4
On 21-02-11 12:00:11, Keith Busch wrote:
> On Thu, Feb 11, 2021 at 04:52:52AM +0900, 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.
> 
> Once the user sets the injected state, all commands return that status
> until the user injects the normal state, so the CRD time is meaningless
> here. If we're really going this route, the state needs to return to
> normal on it's own.

That would also be fine to me.

> But I would prefer to see advanced retry tied to real errors that can be
> retried, like if we got an EBUSY or EAGAIN errno or something like that.

I have seen a thread [1] about ACRE.  Forgive me If I misunderstood this
thread or missed something after this thread.  It looks like CRD field in
the CQE can be set for any NVMe error state which means it *may* depend on
the device status.  And this patch just introduced a internal temporarily
error state of the controller by returning Command Intrrupted status.

I think, in this stage, we can go with some errors in the middle of the
AIO (nvme_aio_err()) for advanced retry.  Shouldn't AIO errors are
retry-able and supposed to be retried ?

> The interface you found to implement this is very interesting though.

Thanks, I just wanted to suggest a scheme to inject something to a
running NVMe device model for various testing.

[1] https://www.spinics.net/lists/dm-devel/msg42165.html
Keith Busch Feb. 11, 2021, 4:24 a.m. UTC | #5
On Thu, Feb 11, 2021 at 12:38:48PM +0900, Minwoo Im wrote:
> On 21-02-11 12:00:11, Keith Busch wrote:
> > But I would prefer to see advanced retry tied to real errors that can be
> > retried, like if we got an EBUSY or EAGAIN errno or something like that.
> 
> I have seen a thread [1] about ACRE.  Forgive me If I misunderstood this
> thread or missed something after this thread.  It looks like CRD field in
> the CQE can be set for any NVMe error state which means it *may* depend on
> the device status.

Right! Setting CRD values is at the controller's discretion for any
error status as long as the host enables ACRE.

> And this patch just introduced a internal temporarily error state of
> the controller by returning Command Intrrupted status.

It's just purely synthetic, though. I was hoping something more natural
could trigger the status. That might not provide the deterministic
scenario you're looking for, though.

I'm not completely against using QEMU as a development/test vehicle for
corner cases like this, but we are introducing a whole lot of knobs
recently, and you practically need to be a QEMU developer to even find
them. We probably should step up the documentation in the wiki along
with these types of features.
 
> I think, in this stage, we can go with some errors in the middle of the
> AIO (nvme_aio_err()) for advanced retry.  Shouldn't AIO errors are
> retry-able and supposed to be retried ?

Sure, we can assume that receiving an error in the AIO callback means
the lower layers exhausted available recovery mechanisms.
Warner Losh Feb. 11, 2021, 4:40 a.m. UTC | #6
On Wed, Feb 10, 2021, 9:26 PM Keith Busch <kbusch@kernel.org> wrote:

> On Thu, Feb 11, 2021 at 12:38:48PM +0900, Minwoo Im wrote:
> > On 21-02-11 12:00:11, Keith Busch wrote:
> > > But I would prefer to see advanced retry tied to real errors that can
> be
> > > retried, like if we got an EBUSY or EAGAIN errno or something like
> that.
> >
> > I have seen a thread [1] about ACRE.  Forgive me If I misunderstood this
> > thread or missed something after this thread.  It looks like CRD field in
> > the CQE can be set for any NVMe error state which means it *may* depend
> on
> > the device status.
>
> Right! Setting CRD values is at the controller's discretion for any
> error status as long as the host enables ACRE.
>
> > And this patch just introduced a internal temporarily error state of
> > the controller by returning Command Intrrupted status.
>
> It's just purely synthetic, though. I was hoping something more natural
> could trigger the status. That might not provide the deterministic
> scenario you're looking for, though.
>
> I'm not completely against using QEMU as a development/test vehicle for
> corner cases like this, but we are introducing a whole lot of knobs
> recently, and you practically need to be a QEMU developer to even find
> them. We probably should step up the documentation in the wiki along
> with these types of features.
>

I'd love that too... I need to test FreeBSD's nvme driver for different
error conditions. I know qemu can help, but it's a bit obscure.

Warner

> I think, in this stage, we can go with some errors in the middle of the
> > AIO (nvme_aio_err()) for advanced retry.  Shouldn't AIO errors are
> > retry-able and supposed to be retried ?
>
> Sure, we can assume that receiving an error in the AIO callback means
> the lower layers exhausted available recovery mechanisms.
>
>
Minwoo Im Feb. 11, 2021, 6:06 a.m. UTC | #7
On 21-02-11 13:24:22, Keith Busch wrote:
> On Thu, Feb 11, 2021 at 12:38:48PM +0900, Minwoo Im wrote:
> > On 21-02-11 12:00:11, Keith Busch wrote:
> > > But I would prefer to see advanced retry tied to real errors that can be
> > > retried, like if we got an EBUSY or EAGAIN errno or something like that.
> > 
> > I have seen a thread [1] about ACRE.  Forgive me If I misunderstood this
> > thread or missed something after this thread.  It looks like CRD field in
> > the CQE can be set for any NVMe error state which means it *may* depend on
> > the device status.
> 
> Right! Setting CRD values is at the controller's discretion for any
> error status as long as the host enables ACRE.
> 
> > And this patch just introduced a internal temporarily error state of
> > the controller by returning Command Intrrupted status.
> 
> It's just purely synthetic, though. I was hoping something more natural
> could trigger the status. That might not provide the deterministic
> scenario you're looking for, though.

That makes snese.  If some status can be triggered more naturally,  that
would be much better.

> I'm not completely against using QEMU as a development/test vehicle for
> corner cases like this, but we are introducing a whole lot of knobs
> recently, and you practically need to be a QEMU developer to even find
> them. We probably should step up the documentation in the wiki along
> with these types of features.

Oh, that's a really good advice, really appreciate that one.

> > I think, in this stage, we can go with some errors in the middle of the
> > AIO (nvme_aio_err()) for advanced retry.  Shouldn't AIO errors are
> > retry-able and supposed to be retried ?
> 
> Sure, we can assume that receiving an error in the AIO callback means
> the lower layers exhausted available recovery mechanisms.

Okay, please let me find a way to trigger this kind of errors more
naturally.  I think this HMP command should be the last one to try if
there's nothing we can do really.
Klaus Jensen Feb. 11, 2021, 7:26 a.m. UTC | #8
On Feb 11 13:24, Keith Busch wrote:
> On Thu, Feb 11, 2021 at 12:38:48PM +0900, Minwoo Im wrote:
> > On 21-02-11 12:00:11, Keith Busch wrote:
> > > But I would prefer to see advanced retry tied to real errors that can be
> > > retried, like if we got an EBUSY or EAGAIN errno or something like that.
> > 
> > I have seen a thread [1] about ACRE.  Forgive me If I misunderstood this
> > thread or missed something after this thread.  It looks like CRD field in
> > the CQE can be set for any NVMe error state which means it *may* depend on
> > the device status.
> 
> Right! Setting CRD values is at the controller's discretion for any
> error status as long as the host enables ACRE.
> 
> > And this patch just introduced a internal temporarily error state of
> > the controller by returning Command Intrrupted status.
> 
> It's just purely synthetic, though. I was hoping something more natural
> could trigger the status. That might not provide the deterministic
> scenario you're looking for, though.
> 
> I'm not completely against using QEMU as a development/test vehicle for
> corner cases like this, but we are introducing a whole lot of knobs
> recently, and you practically need to be a QEMU developer to even find
> them. We probably should step up the documentation in the wiki along
> with these types of features.
>  

Understood, I'll make docs/specs/nvme.txt and wiki documentation a
priority for 6.0.

> > I think, in this stage, we can go with some errors in the middle of the
> > AIO (nvme_aio_err()) for advanced retry.  Shouldn't AIO errors are
> > retry-able and supposed to be retried ?
> 
> Sure, we can assume that receiving an error in the AIO callback means
> the lower layers exhausted available recovery mechanisms.
Dr. David Alan Gilbert Feb. 11, 2021, 10:02 a.m. UTC | #9
* 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.
diff mbox series

Patch

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);