Re: [Qemu-devel] [PATCH v3 2/6] block: Add VFIO based NVMe driver

[Fam Zheng <famz@redhat.com>]

On Mon, 07/10 15:55, Stefan Hajnoczi wrote:
> On Wed, Jul 05, 2017 at 09:36:31PM +0800, Fam Zheng wrote:
> > +static bool nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
> > +{
> > +    BDRVNVMeState *s = bs->opaque;
> > +    uint8_t *resp;
> > +    int r;
> > +    uint64_t iova;
> > +    NvmeCmd cmd = {
> > +        .opcode = NVME_ADM_CMD_IDENTIFY,
> > +        .cdw10 = cpu_to_le32(0x1),
> > +    };
> > +
> > +    resp = qemu_try_blockalign0(bs, 4096);
> 
> Is it possible to use struct NvmeIdCtrl to make this code clearer and
> eliminate the hardcoded sizes/offsets?

Yes, will do.

> 
> > +    if (!resp) {
> > +        error_setg(errp, "Cannot allocate buffer for identify response");
> > +        return false;
> > +    }
> > +    r = nvme_vfio_dma_map(s->vfio, resp, 4096, true, &iova);
> > +    if (r) {
> > +        error_setg(errp, "Cannot map buffer for DMA");
> > +        goto fail;
> > +    }
> > +    cmd.prp1 = cpu_to_le64(iova);
> > +
> > +    if (nvme_cmd_sync(bs, s->queues[0], &cmd)) {
> > +        error_setg(errp, "Failed to identify controller");
> > +        goto fail;
> > +    }
> > +
> > +    if (le32_to_cpu(*(uint32_t *)&resp[516]) < namespace) {
> > +        error_setg(errp, "Invalid namespace");
> > +        goto fail;
> > +    }
> > +    s->write_cache = le32_to_cpu(resp[525]) & 0x1;
> > +    s->max_transfer = (resp[77] ? 1 << resp[77] : 0) * s->page_size;
> > +    /* For now the page list buffer per command is one page, to hold at most
> > +     * s->page_size / sizeof(uint64_t) entries. */
> > +    s->max_transfer = MIN_NON_ZERO(s->max_transfer,
> > +                          s->page_size / sizeof(uint64_t) * s->page_size);
> > +
> > +    memset((char *)resp, 0, 4096);
> > +
> > +    cmd.cdw10 = 0;
> > +    cmd.nsid = namespace;
> > +    if (nvme_cmd_sync(bs, s->queues[0], &cmd)) {
> > +        error_setg(errp, "Failed to identify namespace");
> > +        goto fail;
> > +    }
> > +
> > +    s->nsze = le64_to_cpu(*(uint64_t *)&resp[0]);
> > +
> > +    nvme_vfio_dma_unmap(s->vfio, resp);
> > +    qemu_vfree(resp);
> > +    return true;
> > +fail:
> > +    qemu_vfree(resp);
> > +    return false;
> 
> nvme_vfio_dma_unmap() is not called in the error path.

Will fix.

> 
> > +static coroutine_fn int nvme_cmd_map_qiov(BlockDriverState *bs, NvmeCmd *cmd,
> > +                                          NVMeRequest *req, QEMUIOVector *qiov)
> > +{
> > +    BDRVNVMeState *s = bs->opaque;
> > +    uint64_t *pagelist = req->prp_list_page;
> > +    int i, j, r;
> > +    int entries = 0;
> > +
> > +    assert(qiov->size);
> > +    assert(QEMU_IS_ALIGNED(qiov->size, s->page_size));
> > +    assert(qiov->size / s->page_size <= s->page_size / sizeof(uint64_t));
> > +    for (i = 0; i < qiov->niov; ++i) {
> > +        bool retry = true;
> > +        uint64_t iova;
> > +        qemu_co_mutex_lock(&s->dma_map_lock);
> > +try_map:
> > +        r = nvme_vfio_dma_map(s->vfio,
> > +                              qiov->iov[i].iov_base,
> > +                              qiov->iov[i].iov_len,
> > +                              true, &iova);
> > +        if (r == -ENOMEM && retry) {
> > +            retry = false;
> > +            trace_nvme_dma_flush_queue_wait(s);
> > +            if (s->inflight) {
> > +                trace_nvme_dma_map_flush(s);
> > +                qemu_co_queue_wait(&s->dma_flush_queue, &s->dma_map_lock);
> > +            } else {
> > +                r = nvme_vfio_dma_reset_temporary(s->vfio);
> > +                if (r) {
> > +                    return r;
> 
> dma_map_lock is held here!

Oops, will fix.

> 
> > +static int nvme_co_prw(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
> > +                       QEMUIOVector *qiov, bool is_write, int flags)
> > +{
> > +    BDRVNVMeState *s = bs->opaque;
> > +    int r;
> > +    uint8_t *buf = NULL;
> > +    QEMUIOVector local_qiov;
> > +
> > +    assert(QEMU_IS_ALIGNED(offset, s->page_size));
> > +    assert(QEMU_IS_ALIGNED(bytes, s->page_size));
> > +    assert(bytes <= s->max_transfer);
> 
> Who guarantees max_transfer?  I think request alignment is enforced by
> block/io.c but there is no generic max_transfer handling code, so this
> assertion can be triggered by the guest.  Please handle it as a genuine
> request error instead of using an assertion.

There has been one since 04ed95f4843281e292d93018d56d4b14705f9f2c, see the code
around max_transfer in block/io.c:bdrv_aligned_*.

> 
> > +static int nvme_reopen_prepare(BDRVReopenState *reopen_state,
> > +                               BlockReopenQueue *queue, Error **errp)
> > +{
> > +    return 0;
> > +}
> 
> What is the purpose of this dummy .bdrv_reopen_prepare() implementation?

This is necessary for block jobs to work, other drivers provides dummy
implementations as well.

Fam