[v2,00/20] nvme: support NVMe v1.3d, SGLs and multiple namespaces
mbox series

Message ID 20191015103900.313928-1-its@irrelevant.dk
Headers show
Series
  • nvme: support NVMe v1.3d, SGLs and multiple namespaces
Related show

Message

Klaus Birkelund Jensen Oct. 15, 2019, 10:38 a.m. UTC
Hi,

(Quick note to Fam): most of this series is irrelevant to you as the
maintainer of the nvme block driver, but patch "nvme: add support for
scatter gather lists" touches block/nvme.c due to changes in the shared
NvmeCmd struct.

Anyway, v2 comes with a good bunch of changes. Compared to v1[1], I have
squashed some commits in the beginning of the series and heavily
refactored "nvme: support multiple block requests per request" into the
new commit "nvme: allow multiple aios per command".

I have also removed the original implementation of the Abort command
(commit "nvme: add support for the abort command") as it is currently
too tricky to test reliably. It has been replaced by a stub that,
besides a trivial sanity check, just fails to abort the given command.
*Some* implementation of the Abort command is mandatory, but given the
"best effort" nature of the command this is acceptable for now. When the
device gains support for arbitration it should be less tricky to test.

The support for multiple namespaces is now backwards compatible. The
nvme device still accepts a 'drive' parameter, but for multiple
namespaces the use of 'nvme-ns' devices are required. I also integrated
some feedback from Paul so the device supports non-consecutive namespace
ids.

I have also added some new commits at the end:

  - "nvme: bump controller pci device id" makes sure the Linux kernel
    doesn't apply any quirks to the controller that it no longer has.
  - "nvme: handle dma errors" won't actually do anything before this[2]
    fix to include/hw/pci/pci.h is merged. With these two patches added,
    the device reliably passes some additional nasty tests from blktests
    (block/011 "disable PCI device while doing I/O" and block/019 "break
    PCI link device while doing I/O"). Before this patch, block/011
    would pass from time to time if you were lucky, but would at least
    mess up the controller pretty badly, causing a reset in the best
    case.


  [1]: https://patchwork.kernel.org/project/qemu-devel/list/?series=142383
  [2]: https://patchwork.kernel.org/patch/11184911/


Klaus Jensen (20):
  nvme: remove superfluous breaks
  nvme: move device parameters to separate struct
  nvme: add missing fields in the identify controller data structure
  nvme: populate the mandatory subnqn and ver fields
  nvme: allow completion queues in the cmb
  nvme: add support for the abort command
  nvme: refactor device realization
  nvme: add support for the get log page command
  nvme: add support for the asynchronous event request command
  nvme: add logging to error information log page
  nvme: add missing mandatory features
  nvme: bump supported specification version to 1.3
  nvme: refactor prp mapping
  nvme: allow multiple aios per command
  nvme: add support for scatter gather lists
  nvme: support multiple namespaces
  nvme: bump controller pci device id
  nvme: remove redundant NvmeCmd pointer parameter
  nvme: make lba data size configurable
  nvme: handle dma errors

 block/nvme.c           |   18 +-
 hw/block/Makefile.objs |    2 +-
 hw/block/nvme-ns.c     |  139 +++
 hw/block/nvme-ns.h     |   60 ++
 hw/block/nvme.c        | 1863 +++++++++++++++++++++++++++++++++-------
 hw/block/nvme.h        |  219 ++++-
 hw/block/trace-events  |   37 +-
 include/block/nvme.h   |  132 ++-
 8 files changed, 2094 insertions(+), 376 deletions(-)
 create mode 100644 hw/block/nvme-ns.c
 create mode 100644 hw/block/nvme-ns.h

Comments

no-reply@patchew.org Oct. 15, 2019, 5:19 p.m. UTC | #1
Patchew URL: https://patchew.org/QEMU/20191015103900.313928-1-its@irrelevant.dk/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      hw/misc/imx7_gpr.o
  CC      hw/misc/mst_fpga.o
/tmp/qemu-test/src/hw/block/nvme.c: In function 'nvme_map_prp':
/tmp/qemu-test/src/hw/block/nvme.c:232:42: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
                 trace_nvme_err_addr_read((void *) prp2);
                                          ^
/tmp/qemu-test/src/hw/block/nvme.c:258:50: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
                         trace_nvme_err_addr_read((void *) prp_ent);
                                                  ^
/tmp/qemu-test/src/hw/block/nvme.c: In function 'nvme_map_sgl':
/tmp/qemu-test/src/hw/block/nvme.c:414:42: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
                 trace_nvme_err_addr_read((void *) addr);
                                          ^
/tmp/qemu-test/src/hw/block/nvme.c:429:38: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
             trace_nvme_err_addr_read((void *) addr);
                                      ^
/tmp/qemu-test/src/hw/block/nvme.c:478:38: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
             trace_nvme_err_addr_read((void *) addr);
                                      ^
/tmp/qemu-test/src/hw/block/nvme.c:493:34: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
         trace_nvme_err_addr_read((void *) addr);
                                  ^
/tmp/qemu-test/src/hw/block/nvme.c: In function 'nvme_post_cqes':
/tmp/qemu-test/src/hw/block/nvme.c:847:39: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
             trace_nvme_err_addr_write((void *) addr);
                                       ^
/tmp/qemu-test/src/hw/block/nvme.c: In function 'nvme_process_sq':
/tmp/qemu-test/src/hw/block/nvme.c:1971:38: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
             trace_nvme_err_addr_read((void *) addr);
                                      ^
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: hw/block/nvme.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 662, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=8aa0a85fff1f457c9dc7c826d7b3189d', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-2g1bl41s/src/docker-src.2019-10-15-13.13.48.993:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=8aa0a85fff1f457c9dc7c826d7b3189d
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-2g1bl41s/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    5m56.522s
user    0m7.913s


The full log is available at
http://patchew.org/logs/20191015103900.313928-1-its@irrelevant.dk/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
no-reply@patchew.org Oct. 15, 2019, 5:26 p.m. UTC | #2
Patchew URL: https://patchew.org/QEMU/20191015103900.313928-1-its@irrelevant.dk/



Hi,

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

Subject: [PATCH v2 00/20] nvme: support NVMe v1.3d, SGLs and multiple namespaces
Type: series
Message-id: 20191015103900.313928-1-its@irrelevant.dk

=== 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 ===

Switched to a new branch 'test'
c68f7e0 nvme: handle dma errors
855f2b8 nvme: make lba data size configurable
68fc575 nvme: remove redundant NvmeCmd pointer parameter
eb585d1 nvme: bump controller pci device id
227280c nvme: support multiple namespaces
ccc877b nvme: add support for scatter gather lists
76d6fe6 nvme: allow multiple aios per command
73227cb nvme: refactor prp mapping
df5fd9f nvme: bump supported specification version to 1.3
c85c0ff nvme: add missing mandatory features
1188552 nvme: add logging to error information log page
714808c nvme: add support for the asynchronous event request command
88bdfce nvme: add support for the get log page command
7716649 nvme: refactor device realization
7d2d51e nvme: add support for the abort command
4ec0e81 nvme: allow completion queues in the cmb
68f00db nvme: populate the mandatory subnqn and ver fields
f08d66a nvme: add missing fields in the identify controller data structure
315a6eb nvme: move device parameters to separate struct
b94cf4a nvme: remove superfluous breaks

=== OUTPUT BEGIN ===
1/20 Checking commit b94cf4aea07b (nvme: remove superfluous breaks)
2/20 Checking commit 315a6eb1f09f (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 2/20 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/20 Checking commit f08d66aa761b (nvme: add missing fields in the identify controller data structure)
4/20 Checking commit 68f00db57e87 (nvme: populate the mandatory subnqn and ver fields)
5/20 Checking commit 4ec0e81a8ca5 (nvme: allow completion queues in the cmb)
6/20 Checking commit 7d2d51e5da89 (nvme: add support for the abort command)
7/20 Checking commit 7716649c3d6d (nvme: refactor device realization)
8/20 Checking commit 88bdfce1a599 (nvme: add support for the get log page command)
9/20 Checking commit 714808cd3ef8 (nvme: add support for the asynchronous event request command)
10/20 Checking commit 11885522fa87 (nvme: add logging to error information log page)
11/20 Checking commit c85c0ff5ea35 (nvme: add missing mandatory features)
12/20 Checking commit df5fd9f283a4 (nvme: bump supported specification version to 1.3)
13/20 Checking commit 73227cb3c83c (nvme: refactor prp mapping)
14/20 Checking commit 76d6fe6ea1cf (nvme: allow multiple aios per command)
15/20 Checking commit ccc877b6f72b (nvme: add support for scatter gather lists)
16/20 Checking commit 227280c8d08c (nvme: support multiple namespaces)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#42: 
new file mode 100644

total: 0 errors, 1 warnings, 801 lines checked

Patch 16/20 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
17/20 Checking commit eb585d1231e3 (nvme: bump controller pci device id)
18/20 Checking commit 68fc575b3fc7 (nvme: remove redundant NvmeCmd pointer parameter)
19/20 Checking commit 855f2b86dd6c (nvme: make lba data size configurable)
20/20 Checking commit c68f7e0d0c55 (nvme: handle dma errors)
WARNING: line over 80 characters
#77: FILE: hw/block/nvme.c:257:
+                    if (nvme_addr_read(n, prp_ent, (void *) prp_list, prp_trans)) {

WARNING: line over 80 characters
#103: FILE: hw/block/nvme.c:428:
+        if (nvme_addr_read(n, addr, segment, nsgld * sizeof(NvmeSglDescriptor))) {

total: 0 errors, 2 warnings, 148 lines checked

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

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20191015103900.313928-1-its@irrelevant.dk/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Fam Zheng Oct. 16, 2019, 6:29 a.m. UTC | #3
On Tue, 10/15 12:38, Klaus Jensen wrote:
> Hi,
> 
> (Quick note to Fam): most of this series is irrelevant to you as the
> maintainer of the nvme block driver, but patch "nvme: add support for
> scatter gather lists" touches block/nvme.c due to changes in the shared
> NvmeCmd struct.

Yeah, that part looks sane to me. For the block/nvme.c bit:

Acked-by: Fam Zheng <fam@euphon.net>
Klaus Birkelund Jensen Oct. 28, 2019, 6:09 a.m. UTC | #4
On Tue, Oct 15, 2019 at 12:38:40PM +0200, Klaus Jensen wrote:
> Hi,
> 
> (Quick note to Fam): most of this series is irrelevant to you as the
> maintainer of the nvme block driver, but patch "nvme: add support for
> scatter gather lists" touches block/nvme.c due to changes in the shared
> NvmeCmd struct.
> 
> Anyway, v2 comes with a good bunch of changes. Compared to v1[1], I have
> squashed some commits in the beginning of the series and heavily
> refactored "nvme: support multiple block requests per request" into the
> new commit "nvme: allow multiple aios per command".
> 
> I have also removed the original implementation of the Abort command
> (commit "nvme: add support for the abort command") as it is currently
> too tricky to test reliably. It has been replaced by a stub that,
> besides a trivial sanity check, just fails to abort the given command.
> *Some* implementation of the Abort command is mandatory, but given the
> "best effort" nature of the command this is acceptable for now. When the
> device gains support for arbitration it should be less tricky to test.
> 
> The support for multiple namespaces is now backwards compatible. The
> nvme device still accepts a 'drive' parameter, but for multiple
> namespaces the use of 'nvme-ns' devices are required. I also integrated
> some feedback from Paul so the device supports non-consecutive namespace
> ids.
> 
> I have also added some new commits at the end:
> 
>   - "nvme: bump controller pci device id" makes sure the Linux kernel
>     doesn't apply any quirks to the controller that it no longer has.
>   - "nvme: handle dma errors" won't actually do anything before this[2]
>     fix to include/hw/pci/pci.h is merged. With these two patches added,
>     the device reliably passes some additional nasty tests from blktests
>     (block/011 "disable PCI device while doing I/O" and block/019 "break
>     PCI link device while doing I/O"). Before this patch, block/011
>     would pass from time to time if you were lucky, but would at least
>     mess up the controller pretty badly, causing a reset in the best
>     case.
> 
> 
>   [1]: https://patchwork.kernel.org/project/qemu-devel/list/?series=142383
>   [2]: https://patchwork.kernel.org/patch/11184911/
> 
> 
> Klaus Jensen (20):
>   nvme: remove superfluous breaks
>   nvme: move device parameters to separate struct
>   nvme: add missing fields in the identify controller data structure
>   nvme: populate the mandatory subnqn and ver fields
>   nvme: allow completion queues in the cmb
>   nvme: add support for the abort command
>   nvme: refactor device realization
>   nvme: add support for the get log page command
>   nvme: add support for the asynchronous event request command
>   nvme: add logging to error information log page
>   nvme: add missing mandatory features
>   nvme: bump supported specification version to 1.3
>   nvme: refactor prp mapping
>   nvme: allow multiple aios per command
>   nvme: add support for scatter gather lists
>   nvme: support multiple namespaces
>   nvme: bump controller pci device id
>   nvme: remove redundant NvmeCmd pointer parameter
>   nvme: make lba data size configurable
>   nvme: handle dma errors
> 
>  block/nvme.c           |   18 +-
>  hw/block/Makefile.objs |    2 +-
>  hw/block/nvme-ns.c     |  139 +++
>  hw/block/nvme-ns.h     |   60 ++
>  hw/block/nvme.c        | 1863 +++++++++++++++++++++++++++++++++-------
>  hw/block/nvme.h        |  219 ++++-
>  hw/block/trace-events  |   37 +-
>  include/block/nvme.h   |  132 ++-
>  8 files changed, 2094 insertions(+), 376 deletions(-)
>  create mode 100644 hw/block/nvme-ns.c
>  create mode 100644 hw/block/nvme-ns.h
> 
> -- 
> 2.23.0
> 

Gentle ping on this.

I'm aware that this is a lot to go through, but I would like to know if
anyone has had a chance to look at it?


https://patchwork.kernel.org/project/qemu-devel/list/?series=187637