Re: [Qemu-devel] [PATCH] NVMe: Initial commit to add an NVM Express device

[Andreas Färber]

Hi,

Am 08.12.2012 01:05, schrieb Keith Busch:
> An implementation of a generic NVMe Controller PCI device, developed
> from the open standard available at nvmexpress.org.
> 
> Cc: Michael S. Tsirkin <address@hidden>
> Cc: Keith Busch <address@hidden>
> Signed-off-by: Keith Busch <address@hidden>
> ---
> I've developed for QEMU for a little while, but this is my first patch, so
> I wouldn't be the least bit surprised if I broke some qemu dev rules.
> 
> I wasn't sure if I should submit this as really I think only driver
> developers would be interested. Since it is a fairly large patch, maybe
> no one would review it if they don't already have an interest in NVM
> Express, but we found emulation quite helpful in driver development as
> real hardware is only recently starting to be released, and hope others
> others might find this helpful too. So if this is accepted, us driver
> developers can get all the benefits of the latest QEMU without managing
> our own branch like we're doing today! :)

Generally we encourage people to upstream their devices, given they are
sufficiently isolated and/or maintainable. Your submission is however
lacking an entry to the MAINTAINERS file to assign someone (you?) who
knows and cares about this device, in case some refactoring needs to
touch it (get_maintainer.pl) or questions arise.

IIUC from the website above, NVMe is to be used with SSDs? It would be
good to add to the commit message how to actually use the device
command-line-wise beyond the obvious -device nvme: I did not spot on
brief sight where you expose a bus to add drives (nor a special IF_*
interface type to assign to a drive), so others might wonder as well.
Also explaining the acronym in the commit message (in addition to your
device description far down) is always nice.
Did you do any benchmarks how your NVMe emulation compares to our
existing storage support?

The patch is overly large - might there be a way to split it into a
series by adding some feature in a second step?
If at some point NVMe could become part of a chipset, it might be good
to place the device state struct in an nvme.h header.

I notice that your device is lacking VMState (i.e., migration support).
There's some formal issues that I'll comment on in a second pass.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg