Re: [PATCH 0/4] hw/nvme: add support for TP4084

[Klaus Jensen <its@irrelevant.dk>]

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

On Jun 16 07:57, Keith Busch wrote:
> On Thu, Jun 16, 2022 at 12:42:49PM +0200, Klaus Jensen wrote:
> > On Jun  8 03:28, Niklas Cassel via wrote:
> > > Hello there,
> > > 
> > > considering that Linux v5.19-rc1 is out which includes support for
> > > NVMe TP4084:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/nvme/host/core.c?id=354201c53e61e493017b15327294b0c8ab522d69
> > > 
> > > I thought that it might be nice to have QEMU support for the same.
> > > 
> > > TP4084 adds a new mode, CC.CRIME, that can be used to mark a namespace
> > > as ready independently from the controller.
> > > 
> > > When CC.CRIME is 0 (default), things behave as before, all namespaces
> > > are ready when CSTS.RDY gets set to 1.
> > > 
> > > Add a new "ready_delay" namespace device parameter, in order to emulate
> > > different ready latencies for namespaces when CC.CRIME is 1.
> > > 
> > > The patch series also adds a "crwmt" controller parameter, in order to
> > > be able to expose the worst case timeout that the host should wait for
> > > all namespaces to become ready.
> > > 
> > > 
> > > Example qemu cmd line for the new options:
> > > 
> > > # delay in s (20s)
> > > NS1_DELAY_S=20
> > > # convert to units of 500ms
> > > NS1_DELAY=$((NS1_DELAY_S*2))
> > > 
> > > # delay in s (60s)
> > > NS2_DELAY_S=60
> > > # convert to units of 500ms
> > > NS2_DELAY=$((NS2_DELAY_S*2))
> > > 
> > > # timeout in s (120s)
> > > CRWMT_S=120
> > > # convert to units of 500ms
> > > CRWMT=$((CRWMT_S*2))
> > > 
> > >              -device nvme,serial=deadbeef,crwmt=$CRWMT \
> > >              -drive file=$NS1_DATA,id=nvm-1,format=raw,if=none \
> > >              -device nvme-ns,drive=nvm-1,ready_delay=$NS1_DELAY \
> > >              -drive file=$NS2_DATA,id=nvm-2,format=raw,if=none \
> > >              -device nvme-ns,drive=nvm-2,ready_delay=$NS2_DELAY \
> > > 
> > > 
> > > Niklas Cassel (4):
> > >   hw/nvme: claim NVMe 2.0 compliance
> > >   hw/nvme: store a pointer to the NvmeSubsystem in the NvmeNamespace
> > >   hw/nvme: add support for ratified TP4084
> > >   hw/nvme: add new never_ready parameter to test the DNR bit
> > > 
> > >  hw/nvme/ctrl.c       | 151 +++++++++++++++++++++++++++++++++++++++++--
> > >  hw/nvme/ns.c         |  17 +++++
> > >  hw/nvme/nvme.h       |   9 +++
> > >  hw/nvme/trace-events |   1 +
> > >  include/block/nvme.h |  60 ++++++++++++++++-
> > >  5 files changed, 233 insertions(+), 5 deletions(-)
> > > 
> > > -- 
> > > 2.36.1
> > > 
> > > 
> > 
> > Hi Niklas,
> > 
> > I've been going back and forth on my position on this.
> > 
> > I'm not straight up against it, but this only seems useful as a one-off
> > patch to test the kernel support for this. Considering the limitations
> > you state and the limited use case, I fear this is a little bloaty to
> > carry upstream.
> > 
> > But I totally acknowledge that this is a horrible complicated behavior
> > to implement on the driver side, so I guess we might all benefit from
> > this.
> > 
> > Keith, do you have an opinion on this?
> 
> There are drivers utilizing the capability, so I think supporting it is fine
> despite this environment not having any inherent time-to-ready delays.
> 
> This will probably be another knob that gets lots of use for initial driver
> validation, then largely forgotten. But maybe it will be useful for driver unit
> and regression testing in the future.

Alright, sounds good. I'll give it a proper review :)

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