| Message ID | 20210210195252.19339-4-minwoo.im.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | support command retry | expand |
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 >
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.
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.
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
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.
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. > >
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.
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.
* 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 --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);
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(+)