[v5,00/26] nvme: support NVMe v1.3d, SGLs and multiple namespaces

Message

Klaus Jensen Feb. 4, 2020, 9:51 a.m. UTC

Hi,


Changes since v4
 - Changed vendor and device id to use a Red Hat allocated one. For
   backwards compatibility add the 'x-use-intel-id' nvme device
   parameter. This is off by default but is added as a machine compat
   property to be true for machine types <= 4.2.

 - SGL mapping code has been refactored.


Comments specific to Beata's review:
 - [PATCH v4 19/24] nvme: handle dma errors
   I ended up not including any specific code for resetting the device
   when dma transfers fail for too long. If running I/O and then
   disabling bus master, the OS will (should) eventually reset the
   device and reenable bus mastering (this is the behavior in Linux at
   least). The device can maybe set CFS ("Controller Fail Status") in
   the BAR, but I have not explored that for now.

 - [PATCH v4 17/24] nvme: allow multiple aios per command
   I forgot to give an answer for your comment on the correctness of:

     if (unlikely((slba + nlb) > nsze)) {

   `slba` *is* the "address" of the first logical block, but it is in
   terms of logical blocks, so the condition should be correct. (and at
   this point `nlb` is no longer a 0's based value)


Klaus Jensen (26):
  nvme: rename trace events to nvme_dev
  nvme: remove superfluous breaks
  nvme: move device parameters to separate struct
  nvme: add missing fields in the identify data structures
  nvme: populate the mandatory subnqn and ver fields
  nvme: refactor nvme_addr_read
  nvme: add support for the abort command
  nvme: refactor device realization
  nvme: add temperature threshold feature
  nvme: add support for the get log page command
  nvme: add support for the asynchronous event request command
  nvme: add missing mandatory features
  nvme: additional tracing
  nvme: make sure ncqr and nsqr is valid
  nvme: bump supported specification to 1.3
  nvme: refactor prp mapping
  nvme: allow multiple aios per command
  nvme: use preallocated qsg/iov in nvme_dma_prp
  pci: pass along the return value of dma_memory_rw
  nvme: handle dma errors
  nvme: add support for scatter gather lists
  nvme: support multiple namespaces
  pci: allocate pci id for nvme
  nvme: change controller pci id
  nvme: remove redundant NvmeCmd pointer parameter
  nvme: make lba data size configurable

 MAINTAINERS            |    1 +
 block/nvme.c           |   18 +-
 docs/specs/nvme.txt    |   10 +
 docs/specs/pci-ids.txt |    1 +
 hw/block/Makefile.objs |    2 +-
 hw/block/nvme-ns.c     |  158 ++++
 hw/block/nvme-ns.h     |   62 ++
 hw/block/nvme.c        | 2012 +++++++++++++++++++++++++++++++---------
 hw/block/nvme.h        |  201 +++-
 hw/block/trace-events  |  204 ++--
 hw/core/machine.c      |    1 +
 include/block/nvme.h   |  143 ++-
 include/hw/pci/pci.h   |    4 +-
 13 files changed, 2266 insertions(+), 551 deletions(-)
 create mode 100644 docs/specs/nvme.txt
 create mode 100644 hw/block/nvme-ns.c
 create mode 100644 hw/block/nvme-ns.h

Comments

[e-mail] Feb. 4, 2020, 10:34 a.m. UTC | #1

Patchew URL: https://patchew.org/QEMU/[e-mail]/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH v5 00/26] nvme: support NVMe v1.3d, SGLs and multiple namespaces
Message-id: [e-mail]
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

From https://github.com/patchew-project/qemu
 * [new tag]         patchew/[e-mail] -> patchew/[e-mail]
Switched to a new branch 'test'
a7128db nvme: make lba data size configurable
a0eb0b5 nvme: remove redundant NvmeCmd pointer parameter
0e1bd8c nvme: change controller pci id
f046db4 pci: allocate pci id for nvme
a59d563 nvme: support multiple namespaces
f6b57ba nvme: add support for scatter gather lists
4f78005 nvme: handle dma errors
4e75b11 pci: pass along the return value of dma_memory_rw
c08065d nvme: use preallocated qsg/iov in nvme_dma_prp
3ec0d9f nvme: allow multiple aios per command
540b98f nvme: refactor prp mapping
8fd4e4c nvme: bump supported specification to 1.3
13fceab nvme: make sure ncqr and nsqr is valid
66bf321 nvme: additional tracing
bbb3c58 nvme: add missing mandatory features
77b9455 nvme: add support for the asynchronous event request command
8cdc15c nvme: add support for the get log page command
e612e83 nvme: add temperature threshold feature
ffc039c nvme: refactor device realization
0623024 nvme: add support for the abort command
11b89df nvme: refactor nvme_addr_read
d9f7bf0 nvme: populate the mandatory subnqn and ver fields
f8716d6 nvme: add missing fields in the identify data structures
67d91b0 nvme: move device parameters to separate struct
5f71397 nvme: remove superfluous breaks
f83d65c nvme: rename trace events to nvme_dev

=== OUTPUT BEGIN ===
1/26 Checking commit f83d65c36a14 (nvme: rename trace events to nvme_dev)
2/26 Checking commit 5f71397a1057 (nvme: remove superfluous breaks)
3/26 Checking commit 67d91b03edce (nvme: move device parameters to separate struct)
ERROR: Macros with complex values should be enclosed in parenthesis
#177: FILE: hw/block/nvme.h:6:
+#define DEFINE_NVME_PROPERTIES(_state, _props) \
+    DEFINE_PROP_STRING("serial", _state, _props.serial), \
+    DEFINE_PROP_UINT32("cmb_size_mb", _state, _props.cmb_size_mb, 0), \
+    DEFINE_PROP_UINT32("num_queues", _state, _props.num_queues, 64)

total: 1 errors, 0 warnings, 181 lines checked

Patch 3/26 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

4/26 Checking commit f8716d6d577c (nvme: add missing fields in the identify data structures)
5/26 Checking commit d9f7bf0bea10 (nvme: populate the mandatory subnqn and ver fields)
6/26 Checking commit 11b89df75991 (nvme: refactor nvme_addr_read)
7/26 Checking commit 06230241911a (nvme: add support for the abort command)
8/26 Checking commit ffc039c6a990 (nvme: refactor device realization)
9/26 Checking commit e612e83d5189 (nvme: add temperature threshold feature)
10/26 Checking commit 8cdc15c88d53 (nvme: add support for the get log page command)
11/26 Checking commit 77b945573421 (nvme: add support for the asynchronous event request command)
12/26 Checking commit bbb3c586241a (nvme: add missing mandatory features)
13/26 Checking commit 66bf3218e7f3 (nvme: additional tracing)
14/26 Checking commit 13fceab275cc (nvme: make sure ncqr and nsqr is valid)
15/26 Checking commit 8fd4e4c6fb73 (nvme: bump supported specification to 1.3)
16/26 Checking commit 540b98f3c98d (nvme: refactor prp mapping)
17/26 Checking commit 3ec0d9f718ea (nvme: allow multiple aios per command)
18/26 Checking commit c08065deefa3 (nvme: use preallocated qsg/iov in nvme_dma_prp)
19/26 Checking commit 4e75b1170a2f (pci: pass along the return value of dma_memory_rw)
20/26 Checking commit 4f78005aa73f (nvme: handle dma errors)
21/26 Checking commit f6b57ba3f3f8 (nvme: add support for scatter gather lists)
22/26 Checking commit a59d5630a44f (nvme: support multiple namespaces)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#42: 
new file mode 100644

ERROR: Macros with complex values should be enclosed in parenthesis
#218: FILE: hw/block/nvme-ns.h:8:
+#define DEFINE_NVME_NS_PROPERTIES(_state, _props) \
+    DEFINE_PROP_DRIVE("drive", _state, blk), \
+    DEFINE_PROP_UINT32("nsid", _state, _props.nsid, 0)

total: 1 errors, 1 warnings, 816 lines checked

Patch 22/26 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

23/26 Checking commit f046db41f34b (pci: allocate pci id for nvme)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#29: 
new file mode 100644

total: 0 errors, 1 warnings, 31 lines checked

Patch 23/26 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
24/26 Checking commit 0e1bd8c8281e (nvme: change controller pci id)
25/26 Checking commit a0eb0b55ad5d (nvme: remove redundant NvmeCmd pointer parameter)
26/26 Checking commit a7128db3f7bf (nvme: make lba data size configurable)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/[e-mail]/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to [e-mail]

Keith Busch Feb. 4, 2020, 4:47 p.m. UTC | #2

On Tue, Feb 04, 2020 at 10:51:42AM +0100, Klaus Jensen wrote:
> Hi,
> 
> 
> Changes since v4
>  - Changed vendor and device id to use a Red Hat allocated one. For
>    backwards compatibility add the 'x-use-intel-id' nvme device
>    parameter. This is off by default but is added as a machine compat
>    property to be true for machine types <= 4.2.
> 
>  - SGL mapping code has been refactored.

Looking pretty good to me. For the series beyond the individually
reviewed patches:

Acked-by: Keith Busch <[e-mail]>

If you need to send a v5, you may add my tag to the patches that are not
substaintially modified if you like.

Klaus Jensen Feb. 6, 2020, 7:29 a.m. UTC | #3

On Feb  5 01:47, Keith Busch wrote:
> On Tue, Feb 04, 2020 at 10:51:42AM +0100, Klaus Jensen wrote:
> > Hi,
> > 
> > 
> > Changes since v4
> >  - Changed vendor and device id to use a Red Hat allocated one. For
> >    backwards compatibility add the 'x-use-intel-id' nvme device
> >    parameter. This is off by default but is added as a machine compat
> >    property to be true for machine types <= 4.2.
> > 
> >  - SGL mapping code has been refactored.
> 
> Looking pretty good to me. For the series beyond the individually
> reviewed patches:
> 
> Acked-by: Keith Busch <[e-mail]>
> 
> If you need to send a v5, you may add my tag to the patches that are not
> substaintially modified if you like.

I'll send a v6 with the changes to "nvme: make lba data size
configurable". It won't be substantially changed, I will just only
accept 9 and 12 as valid values for lbads.

Thanks for the Ack's and Reviews Keith!


Klaus