Re: [Qemu-devel] [PATCH 01/11] NVMe: Initial commit for NVM Express device

[Andreas Färber]

Am 27.02.2013 01:47, schrieb Keith Busch:
> NVM Express is an open standard for PCI-e attached Non-Volatile Memory
> storage. This commit adds an emulated device that supports the register
> interface and command set defined by this standard. The standard can
> be viewed at nvmexpress.org. This initial commit implements the minimum
> amount required for an nvme device to work with existing block drivers.
> 
> Cc: Keith Busch <address@hidden>
> Signed-off-by: Keith Busch <address@hidden>
> ---

Sorry, I never got around to finishing review of v1, some more comments
inline:

[...]
> diff --git a/hw/nvme.c b/hw/nvme.c
> new file mode 100644
> index 0000000..dfc93ee
> --- /dev/null
> +++ b/hw/nvme.c
[...]
> +static int nvme_init(PCIDevice *pci_dev)
> +{
> +    NvmeCtrl *n = DO_UPCAST(NvmeCtrl, dev, pci_dev);

Please don't use DO_UPCAST() for QOM objects, introduce a QOM cast macro
NVME(obj) instead. We've collected such review comments here:

http://wiki.qemu.org/QOMConventions

Not mentioned there but in hw/qdev-core.h: We've prepared some new
infrastructure since your original submission. Could you please split
your nvme_init into a non-failing trivial .instance_init
void nvme_init(Object *obj)
and a second-stage
int nvme_initfn(PCIDevice *pci_dev)
that we can later easily change to
void nvme_realize(DeviceState *dev, Error **errp)
?

> +    NvmeIdCtrl *id = &n->id_ctrl;
> +    uint8_t *pci_conf;
> +    int64_t bs_size;
> +    int i, j;
> +
> +    if (!n->conf.bs) {
> +        return -1;
> +    }
> +
> +    bs_size =  bdrv_getlength(n->conf.bs);

Accidental extra space?

> +    if (bs_size <= 0) {
> +        return -1;
> +    }
> +
> +    pci_conf = pci_dev->config;
> +    pci_conf[PCI_INTERRUPT_PIN] = 1;
> +    pci_config_set_prog_interface(pci_dev->config, 0x2);
> +    pci_config_set_class(pci_dev->config, PCI_CLASS_STORAGE_EXPRESS);
> +
> +    n->num_namespaces = 1;
> +    n->num_queues = 64;
> +    n->max_q_ents = 0x7ff;
> +    n->reg_size = 1 << qemu_fls(0x1004 + 2 * (n->num_queues + 1) * 4);
> +    n->ns_size = bs_size / n->num_namespaces;
> +
> +    n->instance = instance++;
> +    n->namespaces = g_malloc0(sizeof(*n->namespaces)*n->num_namespaces);
> +    n->sq = g_malloc0(sizeof(*n->sq)*n->num_queues);
> +    n->cq = g_malloc0(sizeof(*n->cq)*n->num_queues);
> +
> +    id->vid     = PCI_VENDOR_ID_INTEL;
> +    id->ssvid   = 0x0111;
> +    id->rab     = 6;
> +    id->ieee[0] = 0x00;
> +    id->ieee[1] = 0x02;
> +    id->ieee[2] = 0xb3;
> +    id->sqes    = 0xf << 4 | 0x6;
> +    id->cqes    = 0xf << 4 | 0x4;

I assume you've verified operator precedence does what you want, but
adding parentheses around the shift would make that clear to everyone.

> +    id->nn      = n->num_namespaces;
> +    id->vwc     = 1;
> +    snprintf((char *)id->mn, sizeof(id->mn), "QEMU NVMe Ctrl");
> +    snprintf((char *)id->fr, sizeof(id->fr), "1.0");
> +    snprintf((char *)id->sn, sizeof(id->sn), "NVMeQx10%02x", n->instance);

Does this linear instance counting cope with hot-plug and hot-unplug as
expected?

> +    id->psd[0].mp    = 0x9c4;
> +    id->psd[0].enlat = 0x10;
> +    id->psd[0].exlat = 0x4;
> +
> +    n->bar.cap  = (uint64_t)(n->max_q_ents & CAP_MQES_MASK) << 
> CAP_MQES_SHIFT;
> +    n->bar.cap |= (uint64_t)(1   & CAP_CQR_MASK)    << CAP_CQR_SHIFT;
> +    n->bar.cap |= (uint64_t)(1   & CAP_AMS_MASK)    << CAP_AMS_SHIFT;
> +    n->bar.cap |= (uint64_t)(0xf & CAP_TO_MASK)     << CAP_TO_SHIFT;
> +    n->bar.cap |= (uint64_t)(1   & CAP_CSS_MASK)    << CAP_CSS_SHIFT;
> +    n->bar.cap |= (uint64_t)(0xf & CAP_MPSMAX_MASK) << CAP_MPSMAX_SHIFT;
> +    n->bar.vs = 0x00010001;
> +    n->bar.intmc = n->bar.intms = 0;
> +
> +    memory_region_init_io(&n->iomem, &nvme_mmio_ops, n, "nvme-mmio",
> +        n->reg_size);
> +    pci_register_bar(&n->dev, 0,
> +        PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64,
> +        &n->iomem);
> +    msix_init_exclusive_bar(&n->dev, n->num_queues, 4);
> +
> +    for (i = 0; i < n->num_namespaces; i++) {
> +        NvmeNamespace *ns = &n->namespaces[i];
> +        NvmeIdNs *id_ns = &ns->id_ns;
> +        id_ns->ncap  = id_ns->nsze = (n->ns_size) >> 9;
> +        id_ns->nlbaf = 0x4;
> +
> +        for (j = 0; j <= id_ns->nlbaf; j++) {
> +            id_ns->lbaf[j].ds = 9 + j;
> +        }
> +        ns->id = i + 1;
> +        ns->ctrl = n;
> +        ns->start_block = id_ns->nsze * i;
> +    }
> +
> +    return 0;
> +}
> +
> +static void nvme_exit(PCIDevice *pci_dev)
> +{
> +    NvmeCtrl *n = DO_UPCAST(NvmeCtrl, dev, pci_dev);

NVME(pci_dev)

> +    nvme_clear_ctrl(n);
> +    g_free(n->namespaces);
> +    g_free(n->cq);
> +    g_free(n->sq);
> +    msix_uninit_exclusive_bar(pci_dev);
> +    memory_region_destroy(&n->iomem);
> +}

Split off .instance_finalize function matching .instance_init / pc->init
split above if applicable?

> +
> +static void nvme_reset(DeviceState *dev)
> +{
> +    NvmeCtrl *n = DO_UPCAST(NvmeCtrl, dev.qdev, dev);
> +    (void)n;

Add NvmeCtrl *n = NVME(dev) once needed only?

BTW please name NvmeCtrl's dev field parent_obj, then you easily see
where it is still used outside VMState.

> +}
> +
> +static Property nvme_props[] = {
> +    DEFINE_BLOCK_PROPERTIES(NvmeCtrl, conf),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static const VMStateDescription nvme_vmstate = {
> +    .name = "nvme",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_PCI_DEVICE(dev, NvmeCtrl),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void nvme_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    PCIDeviceClass *pc = PCI_DEVICE_CLASS(oc);
> +
> +    pc->init = nvme_init;
> +    pc->exit = nvme_exit;
> +    pc->class_id = PCI_CLASS_STORAGE_EXPRESS;
> +    pc->vendor_id = PCI_VENDOR_ID_INTEL;
> +    pc->device_id = 0x0111;
> +    pc->revision = 1;
> +
> +    dc->desc = "Non-Volatile Memory Express";
> +    dc->reset = nvme_reset;
> +    dc->props = nvme_props;
> +    dc->vmsd = &nvme_vmstate;
> +}
> +
> +static TypeInfo nvme_info = {

static const please, all in-tree devices have been updated recently.

> +    .name          = "nvme",
> +    .parent        = TYPE_PCI_DEVICE,
> +    .instance_size = sizeof(NvmeCtrl),
> +    .class_init    = nvme_class_init,
> +};
> +
> +static void nvme_register_devices(void)
> +{
> +    type_register_static(&nvme_info);
> +}
> +type_init(nvme_register_devices);

Please drop semicolon, it's not a statement, and rename to
nvme_register_types (had been cleaned up early 2012). Also please add a
white line before type_init() and whenever there's more than one line of
statements in a function to separate the variable declaration block.

> diff --git a/hw/nvme.h b/hw/nvme.h
> new file mode 100644
> index 0000000..5716f51
> --- /dev/null
> +++ b/hw/nvme.h
> @@ -0,0 +1,666 @@
> +#ifndef _NVME_H
> +#define _NVME_H

C99 is said to reserve leading underscores, suggest HW_NVME_H if you
want to distinguish from potential system/library header.

[...]
> diff --git a/hw/pci/pci-hotplug.c b/hw/pci/pci-hotplug.c
> index f38df30..992db47 100644
> --- a/hw/pci/pci-hotplug.c
> +++ b/hw/pci/pci-hotplug.c
> @@ -84,8 +84,8 @@ static int scsi_hot_add(Monitor *mon, DeviceState *adapter,
>          object_dynamic_cast(OBJECT(QLIST_FIRST(&adapter->child_bus)),
>                              TYPE_SCSI_BUS);
>      if (!scsibus) {
> -     error_report("Device is not a SCSI adapter");
> -     return -1;
> +        error_report("Device is not a SCSI adapter");
> +        return -1;
>      }
>  
>      /*
[snip]

Unrelated whitespace cleanup hunk?

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