Re: [PATCH 0/3] hw/block/nvme: mdts/zasl cleanup

2021-02-22 Thread Keith Busch
These look good. Reviewed-by: Keith Busch

Re: [PATCH v2 5/5] hw/block/nvme: report non-mdts command size limit for dsm

2021-02-22 Thread Keith Busch
On Mon, Feb 22, 2021 at 07:47:59PM +0100, Klaus Jensen wrote: > +typedef struct NvmeIdCtrlNvm { > +uint8_t vsl; > +uint8_t wzsl; > +uint8_t wusl; > +uint8_t dmrl; > +uint32_tdmrsl; > +uint64_tdmsl; > +uint8_t rsvd16[4080]; > +} NvmeIdCtrlNvm;

Re: [PATCH RFC v3 00/12] hw/block/nvme: metadata and end-to-end data protection support

2021-02-17 Thread Keith Busch
On Wed, Feb 17, 2021 at 10:06:49AM +0100, Klaus Jensen wrote: > On Feb 16 16:19, Keith Busch wrote: > > The verify implementation looked fine, but lacking a generic backing for > > it sounds to me the use cases aren't there to justify taking on this > > feature. > &

Re: [PATCH RFC v3 00/12] hw/block/nvme: metadata and end-to-end data protection support

2021-02-16 Thread Keith Busch
end-to-end data protection is all joint work > with Gollu Appalanaidu. Patches 1 - 8 look good to me: Reviewed-by: Keith Busch I like the LBA format and protection info support too, but might need some minor changes. The verify implementation looked fine, but lacking a generic backing for it sounds to me the use cases aren't there to justify taking on this feature.

Re: [PATCH RFC v3 12/12] hw/block/nvme: add support for the format nvm command

2021-02-16 Thread Keith Busch
On Mon, Feb 15, 2021 at 12:02:40AM +0100, Klaus Jensen wrote: > From: Minwoo Im > > Format NVM admin command can make a namespace or namespaces to be > with different LBA size and metadata size with protection information > types. > > This patch introduces Format NVM command with LBA format, Met

Re: [PATCH RFC v3 11/12] hw/block/nvme: support multiple lba formats

2021-02-16 Thread Keith Busch
On Mon, Feb 15, 2021 at 12:02:39AM +0100, Klaus Jensen wrote: > From: Minwoo Im > > This patch introduces multiple LBA formats supported with the typical > logical block sizes of 512 bytes and 4096 bytes as well as metadata > sizes of 0, 8, 16 and 64 bytes. The format will be chosed based on the

Re: [PATCH RFC v3 09/12] hw/block/nvme: add verify command

2021-02-16 Thread Keith Busch
On Mon, Feb 15, 2021 at 12:02:37AM +0100, Klaus Jensen wrote: > From: Gollu Appalanaidu > > See NVM Express 1.4, section 6.14 ("Verify Command"). > > Signed-off-by: Gollu Appalanaidu > [k.jensen: rebased, refactored for e2e] > Signed-off-by: Klaus Jensen Verify is a generic block command supp

Re: [PATCH RFC v3 08/12] hw/block/nvme: end-to-end data protection

2021-02-16 Thread Keith Busch
On Mon, Feb 15, 2021 at 12:02:36AM +0100, Klaus Jensen wrote: > From: Klaus Jensen > > Add support for namespaces formatted with protection information. The > type of end-to-end data protection (i.e. Type 1, Type 2 or Type 3) is > selected with the `pi` nvme-ns device parameter. If the number of

Re: [PATCH] hw/block/nvme: add broadcast nsid support flush command

2021-02-11 Thread Keith Busch
On Wed, Feb 10, 2021 at 07:42:17AM +0100, Klaus Jensen wrote: > > Is that an Acked-by? ;) Yep, Acked-by: Keith Busch

Re: [PATCH 2/2] hw/block/nvme: add write uncorrectable command

2021-02-11 Thread Keith Busch
On Thu, Feb 11, 2021 at 09:43:05AM +0100, Klaus Jensen wrote: > On Feb 11 12:37, Keith Busch wrote: > > > Is there a use case with a real qemu guest wanting this? > > Like for the extended metadata case (which also does not have a lot of > "public" exposure, but

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

2021-02-10 Thread Keith Busch
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

Re: [PATCH 2/2] hw/block/nvme: add write uncorrectable command

2021-02-10 Thread Keith Busch
On Wed, Feb 10, 2021 at 08:06:46AM +0100, Klaus Jensen wrote: > From: Gollu Appalanaidu > > Add support for marking blocks invalid with the Write Uncorrectable > command. Block status is tracked in a (non-persistent) bitmap that is > checked on all reads and written to on all writes. This is pote

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

2021-02-10 Thread Keith Busch
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-interrup

Re: [PATCH] hw/block/nvme: add broadcast nsid support flush command

2021-02-09 Thread Keith Busch
On Mon, Feb 08, 2021 at 08:08:17PM +0100, Klaus Jensen wrote: > On Feb 9 03:59, Keith Busch wrote: > > This whole implementation would be much simpler with the synchronous > > blk_flush() routine instead of the AIO equivalent. This is not really a > > performant feature,

Re: [PATCH] hw/block/nvme: add broadcast nsid support flush command

2021-02-08 Thread Keith Busch
On Mon, Jan 25, 2021 at 09:42:31PM +0100, Klaus Jensen wrote: > @@ -1644,10 +1679,56 @@ static uint16_t nvme_compare(NvmeCtrl *n, NvmeRequest > *req) > > static uint16_t nvme_flush(NvmeCtrl *n, NvmeRequest *req) > { > -block_acct_start(blk_get_stats(req->ns->blkconf.blk), &req->acct, 0, >

Re: [PATCH RFC v2 4/8] hw/block/nvme: try to deal with the iov/qsg duality

2021-02-08 Thread Keith Busch
On Sun, Feb 07, 2021 at 10:49:36PM +0100, Klaus Jensen wrote: > From: Klaus Jensen > > Introduce NvmeSg and try to deal with that pesky qsg/iov duality that > haunts all the memory-related functions. > > Signed-off-by: Klaus Jensen > --- > hw/block/nvme.h | 8 ++- > hw/block/nvme.c | 171 +++

Re: [PATCH 0/2] hw/block/nvme: add support for telemetry log pages

2021-02-08 Thread Keith Busch
On Mon, Feb 08, 2021 at 03:10:10PM +0100, Klaus Jensen wrote: > From: Klaus Jensen > > This adds support for the telemetry log pages and fixes up the > controller IEEE OUI. Patch 1 is fine. I don't see the point for patch 2. We don't need an empty implementation for every optional spec feature

Re: [PATCH v11 00/13] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set

2021-02-05 Thread Keith Busch
On Sat, Feb 06, 2021 at 01:48:29AM +0900, Minwoo Im wrote: > Not sure if this is okay just give ctrl->tagset for the head > request_queue, but this patch works fine as far. Huh, that's probably not supposed to work: bio-based drivers should never use tagsets. Since this is getting a little more c

Re: [PATCH v11 00/13] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set

2021-02-05 Thread Keith Busch
On Sat, Feb 06, 2021 at 01:07:57AM +0900, Minwoo Im wrote: > If multipath is enabled, the namespace head and hidden namespace will be > created. In this case, /sys/block/nvme0n1/queue/nr_zones are not > returning proper value for the namespace itself. By the way, the hidden > namespace /sys/block

Re: [PATCH v11 00/13] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set

2021-02-05 Thread Keith Busch
On Sat, Feb 06, 2021 at 01:07:57AM +0900, Minwoo Im wrote: > On 21-02-05 08:02:10, Keith Busch wrote: > > On Fri, Feb 05, 2021 at 09:33:54PM +0900, Minwoo Im wrote: > > > On 21-02-05 12:42:30, Klaus Jensen wrote: > > > > On Feb 5 12:25, i...@dantalion.nl wrote:

Re: [PATCH v11 00/13] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set

2021-02-05 Thread Keith Busch
On Fri, Feb 05, 2021 at 09:33:54PM +0900, Minwoo Im wrote: > On 21-02-05 12:42:30, Klaus Jensen wrote: > > On Feb 5 12:25, i...@dantalion.nl wrote: > > > On 05-02-2021 11:39, Klaus Jensen wrote: > > > > This is a good way to report it ;) > > > > It is super helpful and super appreciated! Thanks! >

Re: [PATCH v5 0/5] hw/block/nvme: add simple copy command

2021-02-03 Thread Keith Busch
Just had the one comment on patch 4, which is really no big deal. I need to integrate tooling and/or kernel support in order to properly test this, but just from code inspection, I think it's good. Reviewed-by: Keith Busch

Re: [PATCH v5 4/5] nvme: updated shared header for copy command

2021-02-03 Thread Keith Busch
On Fri, Jan 29, 2021 at 10:15:40AM +0100, Klaus Jensen wrote: > From: Klaus Jensen > > Add new data structures and types for the Simple Copy command. > > Signed-off-by: Klaus Jensen > Reviewed-by: Minwoo Im > Acked-by: Stefan Hajnoczi > --- > include/block/nvme.h | 45 +++

Re: [PATCH 0/3] Fix zone write validation

2021-01-27 Thread Keith Busch
On Tue, Jan 26, 2021 at 09:40:44AM +0100, Klaus Jensen wrote: > On Jan 26 09:21, Klaus Jensen wrote: > > On Jan 26 14:02, Dmitry Fomichev wrote: > > > These patches solve a few problems that exist in zoned Write > > > ans Zone Append validation code. > > > > > > Dmitry Fomichev (3): > > > hw/blo

Re: [PATCH 0/2] hw/block/nvme: zoned fixes

2021-01-27 Thread Keith Busch
ions that apply. Looks good to me. Reviewed-by: Keith Busch

Re: [PATCH V6 0/6] hw/block/nvme: support multi-path for ctrl/ns

2021-01-26 Thread Keith Busch
This came out looking cleaner than I had initially expected. Thanks for seeing this feature through! Reviewed-by: Keith Busch

Re: [PATCH V6 4/6] hw/block/nvme: support for multi-controller in subsystem

2021-01-26 Thread Keith Busch
On Tue, Jan 26, 2021 at 09:52:48AM +0900, Minwoo Im wrote: > On 21-01-25 10:11:43, Keith Busch wrote: > > On Mon, Jan 25, 2021 at 07:03:32PM +0100, Klaus Jensen wrote: > > > On Jan 24 11:54, Minwoo Im wrote: > > > > We have nvme-subsys and nvme devices mapped toget

Re: [PATCH V6 4/6] hw/block/nvme: support for multi-controller in subsystem

2021-01-25 Thread Keith Busch
On Mon, Jan 25, 2021 at 07:03:32PM +0100, Klaus Jensen wrote: > On Jan 24 11:54, Minwoo Im wrote: > > We have nvme-subsys and nvme devices mapped together. To support > > multi-controller scheme to this setup, controller identifier(id) has to > > be managed. Earlier, cntlid(controller id) used to

Re: [PATCH 0/3] hw/block/nvme: misc fixes

2021-01-25 Thread Keith Busch
On Mon, Jan 25, 2021 at 09:22:24AM +0100, Klaus Jensen wrote: > From: Klaus Jensen > > Misc fixes from Gollu. Looks good. Reviewed-by: Keith Busch

Re: [PATCH V5 4/6] hw/block/nvme: support for multi-controller in subsystem

2021-01-22 Thread Keith Busch
On Fri, Jan 22, 2021 at 09:07:34PM +0900, Minwoo Im wrote: > index b525fca14103..3dedefb8ebba 100644 > --- a/hw/block/nvme.c > +++ b/hw/block/nvme.c > @@ -4435,6 +4435,9 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice > *pci_dev) > strpadcpy((char *)id->mn, sizeof(id->mn), "QEMU NVMe Ctr

Re: [PATCH V4 4/6] hw/block/nvme: support for multi-controller in subsystem

2021-01-21 Thread Keith Busch
On Fri, Jan 22, 2021 at 07:09:06AM +0900, Minwoo Im wrote: > -static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev) > +static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev, uint16_t cntlid) > { > NvmeIdCtrl *id = &n->id_ctrl; > uint8_t *pci_conf = pci_dev->config; > > +

Re: [PATCH V4 2/6] hw/block/nvme: support to map controller to a subsystem

2021-01-21 Thread Keith Busch
On Fri, Jan 22, 2021 at 07:09:04AM +0900, Minwoo Im wrote: > --- a/hw/block/nvme.c > +++ b/hw/block/nvme.c > @@ -23,6 +23,7 @@ > * max_ioqpairs=, \ > * aerl=, aer_max_queued=, \ > * mdts=,zoned.append_size_limit= \ > + * ,subsys= \ For cons

Re: [PATCH V4 1/6] hw/block/nvme: introduce nvme-subsys device

2021-01-21 Thread Keith Busch
On Fri, Jan 22, 2021 at 07:09:03AM +0900, Minwoo Im wrote: > +static void nvme_subsys_setup(NvmeSubsystem *subsys) > +{ > +char *subnqn; > + > +subnqn = g_strdup_printf("nqn.2019-08.org.qemu:%s", > subsys->parent_obj.id); > +strpadcpy((char *)subsys->subnqn, sizeof(subsys->subnqn), sub

Re: [PATCH v3 00/12] hw/block/nvme: misc cmb/pmr patches and bump to v1.4

2021-01-19 Thread Keith Busch
zero in its own BAR and free up BAR 4 for use by PMR. > This makes the patch simpler and does not impact any of the existing > address mapping code. > > [1]: > https://lore.kernel.org/qemu-devel/20200729220107.37758-1-andrzej.jakow...@linux.intel.com/ Klaus, Series looks good to me. Reviewed-by: Keith Busch

Re: [PATCH v2 02/12] hw/block/nvme: fix 64 bit register hi/lo split writes

2021-01-19 Thread Keith Busch
On Mon, Jan 18, 2021 at 08:53:24PM +0100, Klaus Jensen wrote: > On Jan 18 21:59, Minwoo Im wrote: > > > The spec requires aligned 32 bit accesses (or the size of the register), > > > so maybe it's about time we actually ignore instead of falling through. > > > > Agreed. > > > > On the other hand

Re: [RFC PATCH V3 0/8] hw/block/nvme: support multi-path for ctrl/ns

2021-01-19 Thread Keith Busch
On Tue, Jan 19, 2021 at 07:18:16PM +0100, Klaus Jensen wrote: > On Jan 20 02:01, Minwoo Im wrote: > > Run with: > > -device nvme,serial=qux,id=nvme3 > > -device nvme-ns,id=ns3,drive=drv12,nsid=3,bus=nvme3 > > > > -device nvme-subsys,id=subsys0 > > -device nvme,serial=foo,id=nvme0,subsys=su

Re: [RFC PATCH 0/5] hw/block/nvme: support multi-path for ctrl/ns

2021-01-15 Thread Keith Busch
On Fri, Jan 15, 2021 at 06:47:20PM +0100, Klaus Jensen wrote: > Cool! I thought so too :) > Question: NSIDs must be the same on each controller for shared > namespaces, but can private namespaces "share" nsid across controllers > in the subsystem? I don't think the spec is clear on that point.

Re: [RFC PATCH 0/5] hw/block/nvme: support multi-path for ctrl/ns

2021-01-15 Thread Keith Busch
On Fri, Jan 15, 2021 at 02:57:45PM +0100, Klaus Jensen wrote: > > As you already mentioned, the problem I see with this approach is that > we have separate namespaces attached to separate controllers. So we are > faking it to the max and if I/O starts going through the other > controller we end up

Re: [PATCH 1/6] hw/block/nvme: fix shutdown/reset logic

2021-01-14 Thread Keith Busch
in use. > > Fixes: 368f4e752cf9 ("hw/block/nvme: Process controller reset and shutdown > differently") > Signed-off-by: Klaus Jensen Yes, and exiting a controller shutdown state requires a controller reset, so a functioning host shouldn't require the controller behave the way it's currently done. Reviewed-by: Keith Busch

Re: [PATCH] hw/block/nvme: conditionally enable DULBE for zoned namespaces

2021-01-14 Thread Keith Busch
the configured > discard_granularity. > > Signed-off-by: Klaus Jensen Looks good. Reviewed-by: Keith Busch

Re: [PATCH] hw/block/nvme: fix zone write finalize

2021-01-14 Thread Keith Busch
d out, so they might want to strike "successful". Looks fine to me. Reviewed-by: Keith Busch > Signed-off-by: Klaus Jensen > Cc: Dmitry Fomichev > --- > hw/block/nvme.c | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/hw/

Re: [PATCH v3 4/4] hw/blocl/nvme: trigger async event during injecting smart warning

2021-01-14 Thread Keith Busch
On Thu, Jan 14, 2021 at 03:22:51PM +0800, zhenwei pi wrote: > @@ -2860,6 +2887,12 @@ static void nvme_set_smart_warning(Object *obj, > Visitor *v, const char *name, > } > > s->smart_critical_warning = value; > + > +/* test each bit of uint8_t for smart.critical_warning */ > +fo

Re: [PATCH v3 3/4] hw/block/nvme: add smart_critical_warning property

2021-01-14 Thread Keith Busch
On Thu, Jan 14, 2021 at 03:22:50PM +0800, zhenwei pi wrote: > +static void nvme_get_smart_warning(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > +NvmeCtrl *s = NVME(obj); With only one exception, all variables of type 'NvmeCt

Re: [PATCH RFC 3/3] hw/block/nvme: end-to-end data protection

2020-12-18 Thread Keith Busch
On Thu, Dec 17, 2020 at 10:02:22PM +0100, Klaus Jensen wrote: > static uint16_t nvme_rw(NvmeCtrl *n, NvmeRequest *req) > { > NvmeRwCmd *rw = (NvmeRwCmd *)&req->cmd; > NvmeNamespace *ns = req->ns; > uint32_t nlb = (uint32_t)le16_to_cpu(rw->nlb) + 1; > uint64_t slba = le64_to_cp

Re: [PATCH 0/3] hw/block/nvme: cmb enhancements and bump to v1.4

2020-12-18 Thread Keith Busch
On Fri, Dec 18, 2020 at 10:23:04AM +0100, Klaus Jensen wrote: > From: Klaus Jensen > > This adds CMB logic from v1.4. To my knowledge, this is the last piece > missing for v1.4 compliance, so bump the controller spec version. Please > retort if I am jumping the gun. > > Since the slow-moving (so

Re: [PATCH RFC 0/3] hw/block/nvme: dif-based end-to-end data protection support

2020-12-18 Thread Keith Busch
On Fri, Dec 18, 2020 at 10:39:01AM +0100, Klaus Jensen wrote: > On Dec 17 13:14, Keith Busch wrote: > > On Thu, Dec 17, 2020 at 10:02:19PM +0100, Klaus Jensen wrote: > > > > Are there any actual users of extended metadata that we care about? I'm > > aware of only

Re: [PATCH RFC 0/3] hw/block/nvme: dif-based end-to-end data protection support

2020-12-17 Thread Keith Busch
On Thu, Dec 17, 2020 at 10:02:19PM +0100, Klaus Jensen wrote: > From: Klaus Jensen > > This series adds support for extended LBAs and end-to-end data > protection. Marked RFC, since there are a bunch of issues that could use > some discussion. > > Storing metadata bytes contiguously with the log

Re: [PATCH v11 00/13] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set

2020-12-15 Thread Keith Busch
Hi Dmitry, Looks good to me, thanks for sticking with it. Reviewed-by: Keith Busch

Re: [PATCH] hw/block/nvme: fix bad clearing of CAP

2020-12-08 Thread Keith Busch
Since the entire NvmeCtrl structure > is zero-filled initially, there is no need for the explicit clearing, so > just remove it. > > Fixes: 37712e00b1f0 ("hw/block/nvme: factor out pmr setup") > Signed-off-by: Klaus Jensen Oops, nice catch. Reviewed-by: Keith Busch &

Re: [PATCH v2] hw/block/nvme: add compare command

2020-12-08 Thread Keith Busch
Appalanaidu > [k.jensen: rebased] > Signed-off-by: Klaus Jensen Looks good. Reviewed-by: Keith Busch

Re: [PATCH v3 2/2] hw/block/nvme: add simple copy command

2020-12-08 Thread Keith Busch
On Tue, Dec 08, 2020 at 09:33:39AM +0100, Klaus Jensen wrote: > +static uint16_t nvme_copy(NvmeCtrl *n, NvmeRequest *req) > +{ > +for (i = 0; i < nr; i++) { > +uint32_t _nlb = le16_to_cpu(range[i].nlb) + 1; > +if (_nlb > le16_to_cpu(ns->id_ns.mssrl)) { > +return N

Re: [PATCH v9 08/12] hw/block/nvme: Support Zoned Namespace Command Set

2020-11-17 Thread Keith Busch
On Thu, Nov 12, 2020 at 08:36:39PM +0100, Klaus Jensen wrote: > On Nov 5 11:53, Dmitry Fomichev wrote: > > @@ -133,6 +300,12 @@ static Property nvme_ns_props[] = { > > DEFINE_BLOCK_PROPERTIES(NvmeNamespace, blkconf), > > DEFINE_PROP_UINT32("nsid", NvmeNamespace, params.nsid, 0), > >

Re: [PATCH v8 5/5] hw/block/nvme: add the dataset management command

2020-11-16 Thread Keith Busch
ock driver if required (i.e. for QCOW2). > > See NVM Express 1.3d, Section 6.7 ("Dataset Management command"). Looks good to me. Reviewed-by: Keith Busch

Re: [PATCH v8 3/5] hw/block/nvme: add dulbe support

2020-11-16 Thread Keith Busch
is not > set. Thus, we always try to deallocate (the BDRV_REQ_MAY_UNMAP flag is > always set). These all sound like reasonable constraints and consistent with the spec. Reviewed-by: Keith Busch

Re: [PATCH v8 2/5] hw/block/nvme: pull aio error handling

2020-11-16 Thread Keith Busch
On Thu, Nov 12, 2020 at 08:59:42PM +0100, Klaus Jensen wrote: > +static void nvme_aio_err(NvmeRequest *req, int ret) > +{ > +uint16_t status = NVME_SUCCESS; > +Error *local_err = NULL; > + > +switch (req->cmd.opcode) { > +case NVME_CMD_READ: > +status = NVME_UNRECOVERED_READ

[PULL] nvme emulation patches for 5.2

2020-11-02 Thread Keith Busch
/block/nvme: fix prp mapping status codes hw/block/nvme: fix create IO SQ/CQ status codes hw/block/nvme: fix queue identifer validation Keith Busch (5): hw/block/nvme: remove pointless rw indirection hw/block/nvme: fix log page offset check hw/block/nvme: support per

Re: [PATCH 08/25] block/nvme: Simplify device reset

2020-10-28 Thread Keith Busch
On Wed, Oct 28, 2020 at 03:02:09PM +, Stefan Hajnoczi wrote: > On Tue, Oct 27, 2020 at 09:55:34AM -0700, Keith Busch wrote: > > On Tue, Oct 27, 2020 at 04:53:31PM +0100, Philippe Mathieu-Daudé wrote: > > > On 10/27/20 3:58 PM, Keith Busch wrote: > > > > On Tue, Oc

Re: [PATCH 08/25] block/nvme: Simplify device reset

2020-10-27 Thread Keith Busch
On Tue, Oct 27, 2020 at 04:53:31PM +0100, Philippe Mathieu-Daudé wrote: > On 10/27/20 3:58 PM, Keith Busch wrote: > > On Tue, Oct 27, 2020 at 02:55:30PM +0100, Philippe Mathieu-Daudé wrote: > >> Avoid multiple endianess conversion by using device endianess. > >> &

Re: [PATCH 08/25] block/nvme: Simplify device reset

2020-10-27 Thread Keith Busch
On Tue, Oct 27, 2020 at 02:55:30PM +0100, Philippe Mathieu-Daudé wrote: > Avoid multiple endianess conversion by using device endianess. > > Signed-off-by: Philippe Mathieu-Daudé > --- > block/nvme.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/nvme.c b/block/n

Re: [PATCH 03/25] block/nvme: Report warning with warn_report()

2020-10-27 Thread Keith Busch
On Tue, Oct 27, 2020 at 02:55:25PM +0100, Philippe Mathieu-Daudé wrote: > Instead of displaying warning on stderr, use warn_report() > which also displays it on the monitor. > > Signed-off-by: Philippe Mathieu-Daudé > --- > block/nvme.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-)

Re: [PATCH 08/25] block/nvme: Simplify device reset

2020-10-27 Thread Keith Busch
On Tue, Oct 27, 2020 at 02:55:30PM +0100, Philippe Mathieu-Daudé wrote: > Avoid multiple endianess conversion by using device endianess. > > Signed-off-by: Philippe Mathieu-Daudé > --- > block/nvme.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/nvme.c b/block/n

Re: [PATCH v6 3/3] hw/block/nvme: add the dataset management command

2020-10-26 Thread Keith Busch
ock driver if required (i.e. for QCOW2). > > See NVM Express 1.3d, Section 6.7 ("Dataset Management command"). > > Signed-off-by: Klaus Jensen Looks fine Reviewed-by: Keith Busch

Re: [PATCH v5 2/2] hw/block/nvme: add the dataset management command

2020-10-22 Thread Keith Busch
On Thu, Oct 22, 2020 at 11:02:04PM +0200, Philippe Mathieu-Daudé wrote: > On 10/22/20 8:49 PM, Klaus Jensen wrote: > > +static uint16_t nvme_dsm(NvmeCtrl *n, NvmeRequest *req) > > +{ > > +NvmeNamespace *ns = req->ns; > > +NvmeDsmCmd *dsm = (NvmeDsmCmd *) &req->cmd; > > +NvmeDsmRange *ra

Re: [PATCH v4 2/2] hw/block/nvme: add the dataset management command

2020-10-22 Thread Keith Busch
On Thu, Oct 22, 2020 at 07:43:33PM +0200, Klaus Jensen wrote: > On Oct 22 08:01, Keith Busch wrote: > > On Thu, Oct 22, 2020 at 09:33:13AM +0200, Klaus Jensen wrote: > > > +if (--(*discards)) { > > > +status = NVME_NO_COMPLETE; > > > +

Re: [PATCH 0/2] hw/block/nvme: two fixes for create sq/cq

2020-10-22 Thread Keith Busch
lty check on the given queue identifier. Looks good. Reviewed-by: Keith Busch

Re: [PATCH v4 2/2] hw/block/nvme: add the dataset management command

2020-10-22 Thread Keith Busch
On Thu, Oct 22, 2020 at 09:33:13AM +0200, Klaus Jensen wrote: > +if (--(*discards)) { > +status = NVME_NO_COMPLETE; > +} else { > +g_free(discards); > +req->opaque = NULL; This case needs a status = req->status; So that we get the e

Re: [PATCH v3 1/2] hw/block/nvme: add dulbe support

2020-10-21 Thread Keith Busch
On Thu, Oct 22, 2020 at 12:17:35AM +0200, Klaus Jensen wrote: > +for (int i = 1; i <= n->num_namespaces; i++) { You can call me old-school, but I don't think C should have allowed mixed declarations with code.

Re: [PATCH v3 2/2] hw/block/nvme: add the dataset management command

2020-10-21 Thread Keith Busch
On Thu, Oct 22, 2020 at 12:17:36AM +0200, Klaus Jensen wrote: > +static void nvme_aio_discard_cb(void *opaque, int ret) > +{ > +NvmeRequest *req = opaque; > +int *discards = req->opaque; > + > +trace_pci_nvme_aio_discard_cb(nvme_cid(req)); > + > +if (ret) { > +req->status =

Re: [PATCH v3 1/2] hw/block/nvme: add dulbe support

2020-10-21 Thread Keith Busch
> set. Thus, we always try to deallocate (the BDRV_REQ_MAY_UNMAP flag is > always set). This looks fine. The constraints sound reasonable to me. Reviewed-by: Keith Busch

Re: [PATCH v7 10/11] hw/block/nvme: Separate read and write handlers

2020-10-20 Thread Keith Busch
On Tue, Oct 20, 2020 at 10:28:22AM +0200, Klaus Jensen wrote: > On Oct 19 11:17, Dmitry Fomichev wrote: > > With ZNS support in place, the majority of code in nvme_rw() has > > become read- or write-specific. Move these parts to two separate > > handlers, nvme_read() and nvme_write() to make the co

Re: [PATCH v7 04/11] hw/block/nvme: Support allocated CNS command variants

2020-10-19 Thread Keith Busch
setup. I'm not sure why the spec assumes those two things go together, but it sure enough does! The implementation looks fine. Reviewed-by: Keith Busch

Re: [PATCH v7 03/11] hw/block/nvme: Add support for Namespace Types

2020-10-19 Thread Keith Busch
On Mon, Oct 19, 2020 at 11:17:18AM +0900, Dmitry Fomichev wrote: > +QEMU_BUILD_BUG_ON(sizeof(NvmeIdNsDescr) != 4); ... > QEMU_BUILD_BUG_ON(sizeof(NvmeIdNsDescr) != 4); You've got duplicate sizeof checks for the NvmeIdNsDescr. Otherwise, the patch looks fine.

Re: [PATCH v7 02/11] hw/block/nvme: Generate namespace UUIDs

2020-10-19 Thread Keith Busch
D > explicitly or have a UUID generated automatically every time a > namespace is initialized. > > Suggested-by: Klaus Jansen > Signed-off-by: Dmitry Fomichev > Reviewed-by: Klaus Jansen Looks good to me. Reviewed-by: Keith Busch

Re: [PATCH v7 01/11] hw/block/nvme: Add Commands Supported and Effects log

2020-10-19 Thread Keith Busch
t to > depending on set CC.CSS. > > Signed-off-by: Dmitry Fomichev Looks good to me. Reviewed-by: Keith Busch

Re: [PATCH v2] hw/block/nvme: fix prp mapping status codes

2020-10-19 Thread Keith Busch
hat. > > See NVMe Express v1.3d, Section 4.3 ("Physical Region Page Entry and > List"). Looks good to me. Reviewed-by: Keith Busch

Re: [PATCH] hw/block/nvme: fix aer logic

2020-10-19 Thread Keith Busch
On Mon, Oct 19, 2020 at 08:54:16AM +0200, Klaus Jensen wrote: > @@ -844,6 +838,12 @@ static void nvme_enqueue_event(NvmeCtrl *n, uint8_t > event_type, > return; > } > > +/* ignore if masked (cqe posted, but event not cleared) */ > +if (n->aer_mask & (1 << event_type)) { > +

Re: [PATCH] hw/block/nvme: fix prp mapping status codes

2020-10-19 Thread Keith Busch
On Mon, Oct 19, 2020 at 01:30:39PM +0200, Klaus Jensen wrote: > @@ -328,7 +328,7 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, uint64_t prp1, > uint64_t prp2, > trace_pci_nvme_map_prp(trans_len, len, prp1, prp2, num_prps); > > if (unlikely(!prp1)) { > -trace_pci_nvme_err_invalid

Re: [PATCH] hw/block/nvme: add block utilization tracking

2020-10-14 Thread Keith Busch
On Wed, Oct 14, 2020 at 10:47:21AM +0200, Klaus Jensen wrote: > On Oct 13 14:06, Keith Busch wrote: > > > If we were going to support it here, wouldn't it make more sense to > > tie it to the filesystem's ability to support fallocate hole punch for > > the b

Re: [PATCH v6 01/11] hw/block/nvme: Add Commands Supported and Effects log

2020-10-13 Thread Keith Busch
On Wed, Oct 14, 2020 at 06:42:02AM +0900, Dmitry Fomichev wrote: > +{ > +NvmeEffectsLog log = {}; > +uint32_t *dst_acs = log.acs, *dst_iocs = log.iocs; > +uint32_t trans_len; > +int i; > + > +trace_pci_nvme_cmd_supp_and_effects_log_read(); > + > +if (off >= sizeof(log)) { >

Re: [PATCH] hw/block/nvme: add block utilization tracking

2020-10-13 Thread Keith Busch
On Tue, Oct 13, 2020 at 09:08:46PM +0200, Klaus Jensen wrote: > From: Klaus Jensen > > This adds support for reporting the Deallocated or Unwritten Logical > Block error (DULBE). This requires tracking the allocated/deallocated > status of all logical blocks. > > Introduce a bitmap that does thi

Re: [PATCH 0/9] nvme qemu cleanups and fixes

2020-10-13 Thread Keith Busch
On Tue, Oct 13, 2020 at 11:04:01AM +0200, Klaus Jensen wrote: > On Sep 30 15:04, Keith Busch wrote: > > After going through the zns enabling, I notice the controller enabling > > is not correct. Then I just continued maked more stuff. The series, I > > think, cont

Re: [PATCH 3/9] hw/block/nvme: support per-namespace smart log

2020-10-01 Thread Keith Busch
On Thu, Oct 01, 2020 at 07:18:37PM +0200, Klaus Jensen wrote: > OK, so I agree that it makes sense for it to be supported on a per > namespace basis, but I think the spec is just keeping the door open for > future namespace specific stuff in the log page - currently there is > none. > > Figure 94

Re: [PATCH v5 05/14] hw/block/nvme: Add support for Namespace Types

2020-10-01 Thread Keith Busch
On Thu, Oct 01, 2020 at 04:23:56PM +, Niklas Cassel wrote: > But I see your point, all of this code: > > if (NVME_CC_CSS(data) != NVME_CC_CSS(n->bar.cc)) { > if (NVME_CC_EN(n->bar.cc)) { > NVME_GUEST_ERR(pci_nvme_err_change_css_when_enabled, >

Re: [PATCH v5 05/14] hw/block/nvme: Add support for Namespace Types

2020-10-01 Thread Keith Busch
On Thu, Oct 01, 2020 at 03:50:35PM +, Niklas Cassel wrote: > On Thu, Oct 01, 2020 at 09:29:22AM -0600, Keith Busch wrote: > > On Thu, Oct 01, 2020 at 11:22:46AM +, Niklas Cassel wrote: > > > On Mon, Sep 28, 2020 at 11:35:19AM +0900, Dmitry Fomichev wrote: > >

Re: [PATCH v5 05/14] hw/block/nvme: Add support for Namespace Types

2020-10-01 Thread Keith Busch
On Thu, Oct 01, 2020 at 11:22:46AM +, Niklas Cassel wrote: > On Mon, Sep 28, 2020 at 11:35:19AM +0900, Dmitry Fomichev wrote: > > From: Niklas Cassel > > @@ -,6 +2328,30 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr > > offset, uint64_t data, > > break; > > case 0x14: /

Re: [PATCH 1/9] hw/block/nvme: remove pointless rw indirection

2020-10-01 Thread Keith Busch
On Thu, Oct 01, 2020 at 10:48:03AM +0200, Klaus Jensen wrote: > On Oct 1 06:05, Klaus Jensen wrote: > > On Sep 30 15:04, Keith Busch wrote: > > > The code switches on the opcode to invoke a function specific to that > > > opcode. There's no point in consolidatin

Re: [PATCH 3/9] hw/block/nvme: support per-namespace smart log

2020-10-01 Thread Keith Busch
On Thu, Oct 01, 2020 at 06:10:57AM +0200, Klaus Jensen wrote: > On Sep 30 15:04, Keith Busch wrote: > > Let the user specify a specific namespace if they want to get access > > stats for a specific namespace. > > > > I don't think this makes sense for v1.3+.

[PATCH 9/9] hw/block/nvme: report actual LBA data shift in LBAF

2020-09-30 Thread Keith Busch
add support for multiple LBA formats in the future. Signed-off-by: Dmitry Fomichev Reviewed-by: Klaus Jensen Signed-off-by: Keith Busch --- hw/block/nvme-ns.c | 5 + 1 file changed, 5 insertions(+) diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c index 2ba0263dda..a85e5fdb42 100644

[PATCH 6/9] hw/block/nvme: reject io commands if only admin command set selected

2020-09-30 Thread Keith Busch
-by: Klaus Jensen Signed-off-by: Keith Busch --- hw/block/nvme.c | 4 include/block/nvme.h | 5 + 2 files changed, 9 insertions(+) diff --git a/hw/block/nvme.c b/hw/block/nvme.c index ec7363ea40..80730e1c03 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -1026,6 +1026,10

[PATCH 7/9] hw/block/nvme: add nsid to get/setfeat trace events

2020-09-30 Thread Keith Busch
From: Klaus Jensen Include the namespace id in the pci_nvme_{get,set}feat trace events. Signed-off-by: Klaus Jensen Signed-off-by: Keith Busch --- hw/block/nvme.c | 4 ++-- hw/block/trace-events | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/hw/block/nvme.c b

[PATCH 8/9] hw/block/nvme: add trace event for requests with non-zero status code

2020-09-30 Thread Keith Busch
From: Klaus Jensen If a command results in a non-zero status code, trace it. Signed-off-by: Klaus Jensen Signed-off-by: Keith Busch --- hw/block/nvme.c | 6 ++ hw/block/trace-events | 1 + 2 files changed, 7 insertions(+) diff --git a/hw/block/nvme.c b/hw/block/nvme.c index

[PATCH 2/9] hw/block/nvme: fix log page offset check

2020-09-30 Thread Keith Busch
Return error if the requested offset starts after the size of the log being returned. Also, move the check for earlier in the function so we're not doing unnecessary calculations. Signed-off-by: Keith Busch --- hw/block/nvme.c | 22 ++ 1 file changed, 10 insertions(+

[PATCH 3/9] hw/block/nvme: support per-namespace smart log

2020-09-30 Thread Keith Busch
Let the user specify a specific namespace if they want to get access stats for a specific namespace. Signed-off-by: Keith Busch --- hw/block/nvme.c | 66 +++- include/block/nvme.h | 1 + 2 files changed, 41 insertions(+), 26 deletions(-) diff --git

[PATCH 4/9] hw/block/nvme: validate command set selected

2020-09-30 Thread Keith Busch
Fail to start the controller if the user requests a command set that the controller does not support. Signed-off-by: Keith Busch --- hw/block/nvme.c | 6 +- hw/block/trace-events | 1 + include/block/nvme.h | 4 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/hw

[PATCH 0/9] nvme qemu cleanups and fixes

2020-09-30 Thread Keith Busch
fine, I took the liberty of porting the zns enabling to it and made a public branch for consideration here: http://git.infradead.org/qemu-nvme.git/shortlog/refs/heads/kb-zns Dmitry Fomichev (1): hw/block/nvme: report actual LBA data shift in LBAF Keith Busch (5): hw/block/nvme: remove

[PATCH 5/9] hw/block/nvme: support for admin-only command set

2020-09-30 Thread Keith Busch
Signed-off-by: Keith Busch --- hw/block/nvme.c | 1 + include/block/nvme.h | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/hw/block/nvme.c b/hw/block/nvme.c index 6c582e6874..ec7363ea40 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -2755,6 +2755,7 @@ static

[PATCH 1/9] hw/block/nvme: remove pointless rw indirection

2020-09-30 Thread Keith Busch
tches. Signed-off-by: Keith Busch --- hw/block/nvme.c | 91 - 1 file changed, 29 insertions(+), 62 deletions(-) diff --git a/hw/block/nvme.c b/hw/block/nvme.c index da8344f196..db52ea0db9 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -9

Re: [PATCH v5 03/14] hw/block/nvme: Introduce the Namespace Types definitions

2020-09-30 Thread Keith Busch
On Mon, Sep 28, 2020 at 11:35:17AM +0900, Dmitry Fomichev wrote: > From: Niklas Cassel > > Define the structures and constants required to implement > Namespace Types support. > > Signed-off-by: Niklas Cassel > Signed-off-by: Dmitry Fomichev > --- > hw/block/nvme-ns.h | 2 ++ > hw/block/nv

Re: [PATCH v3 07/18] hw/block/nvme: add support for the get log page command

2020-09-29 Thread Keith Busch
On Wed, Sep 30, 2020 at 12:42:48AM +0200, Klaus Jensen wrote: > On Sep 29 15:34, Keith Busch wrote: > > Yeah, looks safe as-is, but we're missing out on returning the spec > > required 'Invalid Field'. > > I can't see where it says that we should do that

Re: [PATCH v3 07/18] hw/block/nvme: add support for the get log page command

2020-09-29 Thread Keith Busch
On Tue, Sep 29, 2020 at 11:46:00PM +0200, Klaus Jensen wrote: > On Sep 29 14:11, Peter Maydell wrote: > > > +static uint16_t nvme_fw_log_info(NvmeCtrl *n, NvmeCmd *cmd, uint32_t > > > buf_len, > > > + uint64_t off, NvmeRequest *req) > > > +{ > > > +uint32_t tran

<    1   2   3   >