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

From: Klaus Jensen <its@irrelevant.dk>
To: Dmitry Fomichev <dmitry.fomichev@wdc.com>
Cc: "Fam Zheng" <fam@euphon.net>, "Kevin Wolf" <kwolf@redhat.com>,
	"Damien Le Moal" <damien.lemoal@wdc.com>,
	qemu-block@nongnu.org, "Niklas Cassel" <niklas.cassel@wdc.com>,
	"Klaus Jensen" <k.jensen@samsung.com>,
	qemu-devel@nongnu.org, "Maxim Levitsky" <mlevitsk@redhat.com>,
	"Alistair Francis" <alistair.francis@wdc.com>,
	"Keith Busch" <kbusch@kernel.org>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"Matias Bjorling" <matias.bjorling@wdc.com>
Subject: Re: [PATCH v7 04/11] hw/block/nvme: Support allocated CNS command variants
Date: Tue, 20 Oct 2020 10:21:18 +0200	[thread overview]
Message-ID: <20201020082118.GA178548@apples.localdomain> (raw)
In-Reply-To: <20201019021726.12048-5-dmitry.fomichev@wdc.com>

[-- Attachment #1: Type: text/plain, Size: 2301 bytes --]

On Oct 19 11:17, Dmitry Fomichev wrote:

(snip)

> CAP.CSS (together with the I/O Command Set data structure) defines
> what command sets are supported by the controller.
> 
> CC.CSS (together with Set Profile) can be set to enable a subset of
> the available command sets.
> 
> Even if a user configures CC.CSS to e.g. Admin only, NVM namespaces
> will still be attached (and thus marked as active).
> Similarly, if a user configures CC.CSS to e.g. NVM, ZNS namespaces
> will still be attached (and thus marked as active).
> 
> However, any operation from a disabled command set will result in a
> Invalid Command Opcode.
> 

This part of the commit message seems irrelevant to the patch.

> Add a new Boolean namespace property, "attached", to provide the most
> basic namespace attachment support. The default value for this new
> property is true. Also, implement the logic in the new CNS values to
> include/exclude namespaces based on this new property. The only thing
> missing is hooking up the actual Namespace Attachment command opcode,
> which will allow a user to toggle the "attached" flag per namespace.
> 

Without Namespace Attachment support, the sole purpose of this parameter
is to allow unusable namespace IDs to be reported. I have no problems
with adding support for the additional CNS values. They will return
identical responses, but I think that is good enough for now.

When it is not really needed, we should be wary of adding a parameter
that is really hard to get rid of again.

> The reason for not hooking up this command completely is because the
> NVMe specification requires the namespace management command to be
> supported if the namespace attachment command is supported.
> 

There are many ways to support Namespace Management, and there are a lot
of quirks with each of them. Do we use a big blockdev and carve out
namespaces? Then, what are the semantics of an image resize operation?

Do we dynamically create blockdev devices - thats sounds pretty nice,
but might have other quirks and the attachment is not really persistent.

I think at least the "attached" parameter should be x-prefixed, but
better, leave it out for now until we know how we want Namespace
Attachment and Management to be implemented.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]