[Qemu-devel] [PATCH v3 0/6] block: Add VFIO based driver for NVMe device

Fam Zheng posted 6 patches 6 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170705133635.11850-1-famz@redhat.com
Test FreeBSD passed
Test checkpatch failed
Test docker passed
Test s390x passed
There is a newer version of this series
MAINTAINERS                    |    6 +
block/Makefile.objs            |    1 +
block/block-backend.c          |   10 +
block/io.c                     |   24 +
block/nvme-vfio.c              |  703 +++++++++++++++++++++++++
block/nvme-vfio.h              |   30 ++
block/nvme.c                   | 1103 ++++++++++++++++++++++++++++++++++++++++
block/nvme.h                   |  700 +++++++++++++++++++++++++
block/trace-events             |   32 ++
hw/block/nvme.h                |  698 +------------------------
include/block/block.h          |    2 +
include/block/block_int.h      |    4 +
include/sysemu/block-backend.h |    3 +
qemu-img.c                     |    9 +-
stubs/Makefile.objs            |    1 +
stubs/ram-block.c              |   16 +
16 files changed, 2644 insertions(+), 698 deletions(-)
create mode 100644 block/nvme-vfio.c
create mode 100644 block/nvme-vfio.h
create mode 100644 block/nvme.c
create mode 100644 block/nvme.h
create mode 100644 stubs/ram-block.c
[Qemu-devel] [PATCH v3 0/6] block: Add VFIO based driver for NVMe device
Posted by Fam Zheng 6 years, 9 months ago
v3: Rebase, small tweaks/fixes and add locks to provide basic thread safety
    (basic because it is not really tested).

v2:
    - Implement "split vfio addr space" appraoch. [Paolo]
    - Add back 'device reset' in nvme_close(). [Paolo]
    - Better variable namings. [Stefan]
    - "Reuse" macro definitions from NVMe emulation code.
    - Rebase onto current master which has polling by default and update
      performance results accordingly.
    - Update MAINTAINERS.
    - Specify namespace in URI.
    - The sporadical I/O error from v1 "disappeared" in this version.
    - Tests one: qemu-img bench, fio, bonnie++ and installation of
      ubuntu/fedora/rhel on QEMU emulated nvme and a Intel P3700 card.

Fam Zheng (6):
  stubs: Add stubs for ram block API
  block: Add VFIO based NVMe driver
  block: Introduce bdrv_dma_map and bdrv_dma_unmap
  block/nvme: Implement .bdrv_dma_map and .bdrv_dma_unmap
  qemu-img: Map bench buffer
  block: Move NVMe spec definitions to a separate header

 MAINTAINERS                    |    6 +
 block/Makefile.objs            |    1 +
 block/block-backend.c          |   10 +
 block/io.c                     |   24 +
 block/nvme-vfio.c              |  703 +++++++++++++++++++++++++
 block/nvme-vfio.h              |   30 ++
 block/nvme.c                   | 1103 ++++++++++++++++++++++++++++++++++++++++
 block/nvme.h                   |  700 +++++++++++++++++++++++++
 block/trace-events             |   32 ++
 hw/block/nvme.h                |  698 +------------------------
 include/block/block.h          |    2 +
 include/block/block_int.h      |    4 +
 include/sysemu/block-backend.h |    3 +
 qemu-img.c                     |    9 +-
 stubs/Makefile.objs            |    1 +
 stubs/ram-block.c              |   16 +
 16 files changed, 2644 insertions(+), 698 deletions(-)
 create mode 100644 block/nvme-vfio.c
 create mode 100644 block/nvme-vfio.h
 create mode 100644 block/nvme.c
 create mode 100644 block/nvme.h
 create mode 100644 stubs/ram-block.c

-- 
2.9.4


Re: [Qemu-devel] [PATCH v3 0/6] block: Add VFIO based driver for NVMe device
Posted by Paolo Bonzini 6 years, 9 months ago

On 05/07/2017 15:36, Fam Zheng wrote:
> v3: Rebase, small tweaks/fixes and add locks to provide basic thread safety
>     (basic because it is not really tested).

Sounds good, it can be converted to CoMutex later.

Paolo

Re: [Qemu-devel] [PATCH v3 0/6] block: Add VFIO based driver for NVMe device
Posted by no-reply@patchew.org 6 years, 9 months ago
Hi,

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

Message-id: 20170705133635.11850-1-famz@redhat.com
Type: series
Subject: [Qemu-devel] [PATCH v3 0/6] block: Add VFIO based driver for NVMe device

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
079ea59 block: Move NVMe spec definitions to a separate header
28b0ea5 qemu-img: Map bench buffer
0c5fe62 block/nvme: Implement .bdrv_dma_map and .bdrv_dma_unmap
bb59e7c block: Introduce bdrv_dma_map and bdrv_dma_unmap
be4dab1 block: Add VFIO based NVMe driver
74c0158 stubs: Add stubs for ram block API

=== OUTPUT BEGIN ===
Checking PATCH 1/6: stubs: Add stubs for ram block API...
Checking PATCH 2/6: block: Add VFIO based NVMe driver...
WARNING: line over 80 characters
#191: FILE: block/nvme-vfio.c:133:
+    if (ioctl(s->device, VFIO_DEVICE_GET_REGION_INFO, &s->bar_region_info[index])) {

WARNING: line over 80 characters
#279: FILE: block/nvme-vfio.c:221:
+static int nvme_vfio_pci_write_config(NVMeVFIOState *s, void *buf, int size, int ofs)

ERROR: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
#843: FILE: block/nvme.c:40:
+    volatile uint32_t *doorbell;

ERROR: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
#869: FILE: block/nvme.c:66:
+typedef volatile struct {

WARNING: line over 80 characters
#1381: FILE: block/nvme.c:578:
+            error_setg(errp, "Timeout while waiting for device to reset (%ld ms)",

WARNING: line over 80 characters
#1410: FILE: block/nvme.c:607:
+            error_setg(errp, "Timeout while waiting for device to start (%ld ms)",

total: 2 errors, 4 warnings, 1878 lines checked

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

Checking PATCH 3/6: block: Introduce bdrv_dma_map and bdrv_dma_unmap...
Checking PATCH 4/6: block/nvme: Implement .bdrv_dma_map and .bdrv_dma_unmap...
Checking PATCH 5/6: qemu-img: Map bench buffer...
Checking PATCH 6/6: block: Move NVMe spec definitions to a separate header...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
Re: [Qemu-devel] [PATCH v3 0/6] block: Add VFIO based driver for NVMe device
Posted by Paolo Bonzini 6 years, 9 months ago

On 06/07/2017 16:06, no-reply@patchew.org wrote:
> ERROR: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
> #843: FILE: block/nvme.c:40:
> +    volatile uint32_t *doorbell;
> 
> ERROR: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
> #869: FILE: block/nvme.c:66:
> +typedef volatile struct {

Indeed volatile should not be necessary, since we use memory barriers
appropriately.  But these are hardware registers (like, host hardware)
so I guess it's okay for this special case.

Paolo

Re: [Qemu-devel] [PATCH v3 0/6] block: Add VFIO based driver for NVMe device
Posted by Fam Zheng 6 years, 9 months ago
On Thu, 07/06 16:22, Paolo Bonzini wrote:
> 
> 
> On 06/07/2017 16:06, no-reply@patchew.org wrote:
> > ERROR: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
> > #843: FILE: block/nvme.c:40:
> > +    volatile uint32_t *doorbell;
> > 
> > ERROR: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
> > #869: FILE: block/nvme.c:66:
> > +typedef volatile struct {
> 
> Indeed volatile should not be necessary, since we use memory barriers
> appropriately.  But these are hardware registers (like, host hardware)
> so I guess it's okay for this special case.

I think I used it because we don't have ACCESS_ONCE (maybe we should?).

Fam

Re: [Qemu-devel] [PATCH v3 0/6] block: Add VFIO based driver for NVMe device
Posted by Paolo Bonzini 6 years, 9 months ago

On 06/07/2017 16:36, Fam Zheng wrote:
> On Thu, 07/06 16:22, Paolo Bonzini wrote:
>>
>>
>> On 06/07/2017 16:06, no-reply@patchew.org wrote:
>>> ERROR: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
>>> #843: FILE: block/nvme.c:40:
>>> +    volatile uint32_t *doorbell;
>>>
>>> ERROR: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
>>> #869: FILE: block/nvme.c:66:
>>> +typedef volatile struct {
>>
>> Indeed volatile should not be necessary, since we use memory barriers
>> appropriately.  But these are hardware registers (like, host hardware)
>> so I guess it's okay for this special case.
> 
> I think I used it because we don't have ACCESS_ONCE (maybe we should?).

We have atomic_read and atomic_set (and Linux in fact tries not to use
ACCESS_ONCE anymore, it's been replaced by READ_ONCE and WRITE_ONCE so
it's really 1:1 with QEMU).

Paolo