Re: [Qemu-block] [Qemu-devel] [PATCH 8/8] nvme: add an OpenChannel 2.0 NVMe device (ocssd)

[Klaus Birkelund]

On Mon, May 20, 2019 at 11:45:00AM -0500, Eric Blake wrote:
> On 5/17/19 3:42 AM, Klaus Birkelund Jensen wrote:
> > This adds a new 'ocssd' block device that emulates an OpenChannel 2.0
> > device. The device is backed by a new 'ocssd' block backend that is
> > based on the raw format driver but includes a header that holds the
> > device geometry and write data requirements. This new block backend is
> > special in that the size is not specified explicitly but in terms of
> > sector size, number of chunks, number of parallel units, etc. This
> > called for the addition of the `no_size_required` field in `struct
> > BlockDriver` to not fail image creation when the size parameter is
> > missing.
> > 
> > The ocssd device is an individual device but shares a lot of code with
> > the nvme device. Thus, some core functionality of nvme/nvme.c has been
> > exported for use by nvme/ocssd.c.
> > 
> > Thank you to the following people for their contributions to the
> > original qemu-nvme (github.com/OpenChannelSSD/qemu-nvme) implementation.
> > 
> >   Matias Bjørling <address@hidden>
> >   Javier González <address@hidden>
> >   Simon Andreas Frimann Lund <address@hidden>
> >   Hans Holmberg <address@hidden>
> >   Jesper Devantier <address@hidden>
> >   Young Tack Jin <address@hidden>
> > 
> > Signed-off-by: Klaus Birkelund Jensen <address@hidden>
> > ---
> >  MAINTAINERS                     |   14 +-
> >  Makefile.objs                   |    1 +
> >  block.c                         |    2 +-
> >  block/Makefile.objs             |    2 +-
> >  block/nvme.c                    |    2 +-
> >  block/ocssd.c                   |  690 ++++++++
> >  hw/block/Makefile.objs          |    2 +-
> >  hw/block/{ => nvme}/nvme.c      |  192 ++-
> >  hw/block/nvme/ocssd.c           | 2647 +++++++++++++++++++++++++++++++
> >  hw/block/nvme/ocssd.h           |  140 ++
> >  hw/block/nvme/trace-events      |  136 ++
> >  hw/block/trace-events           |  109 --
> >  include/block/block_int.h       |    3 +
> >  include/block/nvme.h            |   12 +-
> >  include/block/ocssd.h           |  231 +++
> >  {hw => include/hw}/block/nvme.h |   61 +
> >  include/hw/pci/pci_ids.h        |    2 +
> >  qapi/block-core.json            |   47 +-
> >  18 files changed, 4121 insertions(+), 172 deletions(-)
> >  create mode 100644 block/ocssd.c
> >  rename hw/block/{ => nvme}/nvme.c (94%)
> >  create mode 100644 hw/block/nvme/ocssd.c
> >  create mode 100644 hw/block/nvme/ocssd.h
> >  create mode 100644 hw/block/nvme/trace-events
> >  create mode 100644 include/block/ocssd.h
> >  rename {hw => include/hw}/block/nvme.h (63%)
> 
> Feels big; are you sure this can't be split into smaller pieces to ease
> review?
> 

I know, but I'm not sure how to meaningfully split it up. Would you
prefer that I move files in one commit? Changed stuff to nvme.{c,h} is
mostly removing static from functions and creating a prototype in the
header files to allow the ocssd device to use the functions. The commit
should be restricted to just adding the ocssd device. Any features and
additions required in the nvme device are added in previous commits.

> I'm focusing just on the qapi portions:
> 

Thank you for the review of that, but it looks like this will all be
dropped from a v2 (see mail from Kevin), because it's simply bad
design to have the driver and device depend so closely on each other.


Thanks,
Klaus