Re: [Qemu-devel] [PATCH 5/5] [scsi-bsg]: Add initial support for BSG based SCSIDeviceInfo

[Nicholas A. Bellinger]

On Mon, 2010-06-14 at 18:11 +0000, Blue Swirl wrote:
> On Mon, Jun 14, 2010 at 9:44 AM, Nicholas A. Bellinger
> <address@hidden> wrote:
> > From: Nicholas Bellinger <address@hidden>
> >
> > This patch adds initial support for using the Linux BSG interface with 
> > write/read vectored
> > AIO as a QEMU backstore (SCSIDeviceInfo) with hw/scsi-bus.c compatible HBA 
> > emulation.
> 
> Did I miss the docs?

I assume you mean the docs for CLI usage, yes..?  This ops are the same
as scsi-generic, and using megasas HBA emulation look like:

qemu-system-x86_64 -m 512 -boot c ~/lenny-32bit.img -drive
if=none,id=mydisk1,file=/dev/4\:0\:1\:0 -device megasas,id=raid -device
scsi-bsg,bus=raid.0,scsi-id=1,drive=mydisk1

> 
> >
> > So far it has been tested with x86_64 host and guest using hw/megasas.c and 
> > TCM_Loop LLD
> > Port LUNs.  Because this path uses struct iovec for struct 
> > sg_io_v4->d[out,in]_xferp payloads,
> > which currently requires a patch to linux/block/bsg.c:bsg_map_hdr() in 
> > order to setup the
> > user -> kernel iovecs.  This also will only currently work with paired 
> > user/kernel
> > (eg: 64bit user / 64bit kernel) because of different pointer sizes in 
> > struct iovec->iov_base.
> >
> > There are also two FIXMEs in hw/scsi-bsg.c:bsg_generic_initfn() related to 
> > extraction of
> > SCSI LUN and device type values using BSG and required by QEMU-KVM.
> >
> > Signed-off-by: Nicholas A. Bellinger <address@hidden>
> > ---
> >  Makefile.objs |    2 +-
> >  hw/scsi-bsg.c |  588 
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 589 insertions(+), 1 deletions(-)
> >  create mode 100644 hw/scsi-bsg.c
> >
> > diff --git a/Makefile.objs b/Makefile.objs
> > index 188d617..c4fcb72 100644
> > --- a/Makefile.objs
> > +++ b/Makefile.objs
> > @@ -197,7 +197,7 @@ hw-obj-$(CONFIG_IDE_CMD646) += ide/cmd646.o
> >  hw-obj-$(CONFIG_IDE_MACIO) += ide/macio.o
> >
> >  # SCSI layer
> > -hw-obj-y += scsi-disk.o scsi-generic.o
> > +hw-obj-y += scsi-disk.o scsi-generic.o scsi-bsg.o
> 
> Instead of '#ifdef __linux__' (which should be '#ifdef CONFIG_LINUX'),
> please compile the object only if CONFIG_LINUX is set, something like:
> hw-obj-$(CONFIG_LINUX) += scsi-bsg.o
> 
> Please also check if this could be compiled in common-obj set.
> 

Ok, I added the 'hw-obj-$(CONFIG_LINUX) += scsi-bsg.o' mentioned above..

> >  hw-obj-y += lsi53c895a.o megasas.o
> >  hw-obj-$(CONFIG_ESP) += esp.o
> >
> > diff --git a/hw/scsi-bsg.c b/hw/scsi-bsg.c
> > new file mode 100644
> > index 0000000..fc76b76
> > --- /dev/null
> > +++ b/hw/scsi-bsg.c
> > @@ -0,0 +1,588 @@
> > +/*
> > + * block layer implementation of the sg v4 interface for Linux hosts
> > + *
> > + * Copyright (c) 2010 Rising Tide Systems
> > + * Written by Nicholas A. Bellinger <address@hidden>
> > + *
> > + * Based on hw/scsi-generic code by Laurent Vivier, Paul Brook, and 
> > Fabrice Bellard
> > + *
> > + * This code is licenced under the LGPL.
> > + */
> > +
> > +#include "qemu-common.h"
> > +#include "qemu-error.h"
> > +#include "block.h"
> > +#include "scsi.h"
> > +#include "dma.h"
> > +#include "block/raw-posix-aio.h"
> > +
> > +#ifdef __linux__
> > +
> > +#define DEBUG_BSG
> > +#undef DEBUG_BSG_IO
> > +#undef DEBUG_BSG_MAP
> 
> This should be
> //#define DEBUG_BSG
> //#define DEBUG_BSG_IO
> //#define DEBUG_BSG_MAP
> 

Fixed

> > +
> > +#ifdef DEBUG_BSG
> > +#define DPRINTF(fmt, ...) \
> > +do { printf("scsi-bsg: " fmt , ## __VA_ARGS__); } while (0)
> > +#else
> > +#define DPRINTF(fmt, ...) do {} while(0)
> > +#endif
> > +
> > +#define BADF(fmt, ...) \
> > +do { fprintf(stderr, "scsi-bsg: " fmt , ## __VA_ARGS__); } while (0)
> > +
> > +#include <stdio.h>
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <sys/epoll.h>
> > +#include <unistd.h>
> > +#include <scsi/sg.h>
> > +#include <linux/bsg.h>
> > +#include "scsi-defs.h"
> > +
> > +#define SCSI_SENSE_BUF_SIZE 96
> > +
> > +#define SG_ERR_DRIVER_TIMEOUT 0x06
> > +#define SG_ERR_DRIVER_SENSE 0x08
> > +
> > +#ifndef MAX_UINT
> > +#define MAX_UINT ((unsigned int)-1)
> 
> The standard macro is UINT_MAX.
> 

Fixed (This was originally from hw/scsi-generic.c)

> > +#endif
> > +
> > +typedef struct SCSIBSGState SCSIBSGState;
> > +
> > +typedef struct SCSIBSGReq {
> > +    SCSIRequest req;
> > +    uint8_t *buf;
> > +    int buflen;
> > +    QEMUIOVector iov;
> > +    QEMUIOVector aio_iov;
> > +    struct sg_io_v4 bsg_hdr;
> > +} SCSIBSGReq;
> > +
> > +struct SCSIBSGState {
> > +    SCSIDevice qdev;
> > +    BlockDriverState *bs;
> > +    int lun;
> > +    int driver_status;
> > +    uint8_t sensebuf[SCSI_SENSE_BUF_SIZE];
> > +    uint8_t senselen;
> > +};
> > +
> > +static int bsg_read(int fd, void *p_read, int to_read)
> > +{
> > +    int err;
> > +
> > +    while (to_read > 0) {
> > +        err = read(fd, p_read, to_read);
> > +        if (err >= 0) {
> > +            to_read -= err;
> > +            p_read += err;
> > +        } else if (errno == EINTR)
> > +            continue;
> > +        else {
> > +            printf("bsg device %d read failed, errno: %d\n",
> > +                    fd, errno);
> 
> DPRINTF?

I made this into a error_report()

> 
> > +            return errno;
> > +        }
> > +    }
> > +    return 0;
> > +}
> > +
> > +static SCSIBSGReq *bsg_new_request(SCSIDevice *d, uint32_t tag, uint32_t 
> > lun)
> > +{
> > +    SCSIRequest *req;
> > +    SCSIBSGReq *r;
> > +
> > +    req = scsi_req_alloc(sizeof(SCSIBSGReq), d, tag, lun);
> > +    r = DO_UPCAST(SCSIBSGReq, req, req);
> > +    qemu_iovec_init(&r->iov, 1);
> > +    qemu_iovec_init(&r->aio_iov, 1);
> > +    return r;
> > +}
> > +
> > +static void bsg_remove_request(SCSIBSGReq *r)
> > +{
> > +    qemu_free(r->buf);
> > +    qemu_iovec_destroy(&r->iov);
> > +    qemu_iovec_destroy(&r->aio_iov);
> > +    scsi_req_free(&r->req);
> > +}
> > +
> > +static void bsg_command_complete(void *opaque, int ret)
> > +{
> > +    SCSIBSGReq *r = (SCSIBSGReq *)opaque;
> 
> Useless cast in C.
> 

Fixed

> > +    SCSIBSGState *s = DO_UPCAST(SCSIBSGState, qdev, r->req.dev);
> > +
> > +    s->driver_status = r->bsg_hdr.driver_status;
> > +    if (s->driver_status)
> > +        s->senselen = SCSI_SENSE_BUF_SIZE;
> > +
> > +    if (ret != 0) {
> > +        scsi_req_print(&r->req);
> > +        fprintf(stderr, "%s: ret %d (%s)\n", __FUNCTION__,
> > +                ret, strerror(-ret));
> 
> error_report()?
> 

Ok, using error_report() instead of fprintf(stderr) in
bsg_command_complete()

> > +        s->senselen = scsi_build_sense(SENSE_CODE(INVALID_FIELD),
> > +                    s->sensebuf, SCSI_SENSE_BUF_SIZE, 0);
> > +        s->driver_status = SG_ERR_DRIVER_SENSE;
> > +        r->req.status = CHECK_CONDITION;
> > +    } else {
> > +        if (s->driver_status & SG_ERR_DRIVER_TIMEOUT) {
> > +            scsi_req_print(&r->req);
> > +            fprintf(stderr, "%s: timeout\n", __FUNCTION__);
> > +            r->req.status = BUSY << 1;
> > +        } else if (r->bsg_hdr.device_status) {
> > +            r->req.status = r->bsg_hdr.device_status;
> > +        } else if (s->driver_status & SG_ERR_DRIVER_SENSE) {
> > +            scsi_req_print(&r->req);
> > +            fprintf(stderr, "%s: driver sense\n", __FUNCTION__);
> > +            r->req.status = CHECK_CONDITION << 1;
> > +        } else {
> > +            r->req.status = GOOD << 1;
> > +        }
> > +    }
> > +#ifdef DEBUG_BSG_IO
> > +    DPRINTF("Command complete 0x%p tag=0x%x status=%d\n",
> > +            r, r->req.tag, r->req.status);
> 
> Please introduce DPRINTF_BSG_IO and remove #ifdef/#endif.
> 

Done

> > +#endif
> > +    scsi_req_complete(&r->req);
> > +}
> > +
> > +static int bsg_execute_command_run(SCSIBSGReq *r,
> > +                                   BlockDriverCompletionFunc *complete)
> > +{
> > +    BlockDriverState *bdrv = r->req.dev->conf.dinfo->bdrv;
> > +    SCSIBSGState *s = DO_UPCAST(SCSIBSGState, qdev, r->req.dev);
> > +    /*
> > +     * Following linux/include/linux/bsg.h
> > +     */
> > +    /* [i] 'Q' to differentiate from v3 */
> > +    r->bsg_hdr.guard = 'Q';
> > +    r->bsg_hdr.protocol = BSG_PROTOCOL_SCSI;
> > +    r->bsg_hdr.subprotocol = BSG_SUB_PROTOCOL_SCSI_CMD;
> > +    r->bsg_hdr.request_len = r->req.cmd.len;
> > +    r->bsg_hdr.request = (unsigned long)r->req.cmd.buf;
> > +    r->bsg_hdr.max_response_len = sizeof(s->sensebuf);
> > +    /* SCSI: (auto)sense data */
> > +    r->bsg_hdr.response = (unsigned long)s->sensebuf;
> > +    /* Unlimited timeout */
> > +    r->bsg_hdr.timeout = MAX_UINT;
> > +    /* [i->o] unused internally */
> > +    r->bsg_hdr.usr_ptr = (unsigned long)r;
> > +    /* Bsg does Q_AT_HEAD by default */
> > +    r->bsg_hdr.flags |= BSG_FLAG_Q_AT_TAIL;
> 
> Does something initialize r->bsg_hdr.flags before?
> 

Nope, changed to an assignment..

> > +
> > +    qemu_iovec_reset(&r->aio_iov);
> > +    qemu_iovec_add(&r->aio_iov, &r->bsg_hdr, sizeof(r->bsg_hdr));
> > +
> > +    r->req.aiocb = paio_submit_len(bdrv, bdrv->fd, 0, &r->aio_iov,
> > +                sizeof(r->bsg_hdr), complete, r, QEMU_AIO_WRITE);
> > +    if (r->req.aiocb == NULL) {
> > +        BADF("execute_command: paio_submit_len() failed\n");
> > +        return -1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static int bsg_execute_command_buf(SCSIBSGReq *r,
> > +                                   BlockDriverCompletionFunc *complete,
> > +                                   uint8_t *buf, uint32_t buflen)
> > +{
> > +    if (r->req.cmd.mode == SCSI_XFER_TO_DEV) {
> > +        r->bsg_hdr.dout_xferp = (unsigned long)buf;
> > +        r->bsg_hdr.dout_xfer_len = buflen;
> > +    } else if (r->req.cmd.mode == SCSI_XFER_FROM_DEV) {
> > +        r->bsg_hdr.din_xferp = (unsigned long)buf;
> > +        r->bsg_hdr.din_xfer_len = buflen;
> > +    }
> > +#ifdef DEBUG_BSG_IO
> > +    DPRINTF("execute BUF: %p, dxfer_len %u\n", buf, buflen);
> > +#endif
> > +    return bsg_execute_command_run(r, complete);
> > +}
> > +
> > +static int bsg_execute_command_iov(SCSIBSGReq *r,
> > +                                   BlockDriverCompletionFunc *complete,
> > +                                   QEMUIOVector *iov)
> > +{
> > +    if (r->req.cmd.mode == SCSI_XFER_TO_DEV) {
> > +        r->bsg_hdr.dout_iovec_count = iov->niov;
> > +        r->bsg_hdr.dout_xferp = (unsigned long)iov->iov;
> > +        r->bsg_hdr.dout_xfer_len = iov->size;
> > +    } else if (r->req.cmd.mode == SCSI_XFER_FROM_DEV) {
> > +        r->bsg_hdr.din_iovec_count = iov->niov;
> > +        r->bsg_hdr.din_xferp = (unsigned long)iov->iov;
> > +        r->bsg_hdr.din_xfer_len = iov->size;
> > +    }
> > +#ifdef DEBUG_BSG_IO
> > +    DPRINTF("execute IOV: iovec_count: %u, iov: %p, size: %u\n",
> > +            iov->niov, iov->iov, (unsigned int)iov->size);
> 
> You can remove the cast by using '%zu'.
> 

Fixed

> > +static int bsg_generic_initfn(SCSIDevice *dev)
> > +{
> > +    SCSIBSGState *s = DO_UPCAST(SCSIBSGState, qdev, dev);
> > +
> > +    if (!s->qdev.conf.dinfo || !s->qdev.conf.dinfo->bdrv) {
> > +        error_report("scsi-bsg: drive property not set");
> > +        return -1;
> > +    }
> > +    s->bs = s->qdev.conf.dinfo->bdrv;
> > +
> > +    /* check we are really using a /dev/bsg/ * file */
> > +    if (!bdrv_is_bsg(s->bs)) {
> > +        error_report("scsi-bsg: not BSG*");
> > +        return -1;
> > +    }
> > +#if 0
> > +    /* get LUN of the BSG */
> > +    if (bdrv_ioctl(s->bs, SG_GET_SCSI_ID, &scsiid)) {
> > +        error_report("scsi-bsg: SG_GET_SCSI_ID ioctl failed");
> > +        return -1;
> > +    }
> > +#endif
> 
> Dead code shouldn't be committed.
> 

Understood.  The reason this was included in the original commit is
because this is what hw/scsi-generic.c with the SG_IO ioctl supports to
determine HCTL, and adding it to linux/block/bsg.c IOCTL is going to
make sense as it already supports certain existing SG_IO IOCTL ops.

> > +// FIXME: Get SCSI lun from BSG
> > +    s->lun = 0;
> > +// FIXME: Get SCSI device type from BSG INQUIRY
> > +    s->qdev.type = TYPE_DISK;
> > +    DPRINTF("LUN %d\n", s->lun);
> > +    DPRINTF("device type %d\n", s->qdev.type);
> > +
> > +    if (s->qdev.type == TYPE_TAPE) {
> > +        s->qdev.blocksize = bsg_get_stream_blocksize(s->bs);
> > +        if (s->qdev.blocksize == -1)
> > +            s->qdev.blocksize = 0;
> > +    } else {
> > +        s->qdev.blocksize = bsg_get_blocksize(s->bs);
> > +        /* removable media returns 0 if not present */
> > +        if (s->qdev.blocksize <= 0) {
> > +            if (s->qdev.type == TYPE_ROM || s->qdev.type  == TYPE_WORM)
> > +                s->qdev.blocksize = 2048;
> > +            else
> > +                s->qdev.blocksize = 512;
> > +        }
> > +    }
> > +    DPRINTF("block size %d\n", s->qdev.blocksize);
> > +    s->driver_status = 0;
> > +    memset(s->sensebuf, 0, sizeof(s->sensebuf));
> > +    return 0;
> > +}
> > +
> > +static void bsg_generic_unmap(SCSIBSGReq *r)
> > +{
> > +    int is_write = !scsi_req_is_write(&r->req);
> > +    int i;
> > +
> > +    for (i = 0; i < r->iov.niov; i++) {
> > +        cpu_physical_memory_unmap(r->iov.iov[i].iov_base,
> > +                                  r->iov.iov[i].iov_len, is_write,
> > +                                  r->iov.iov[i].iov_len);
> > +    }
> > +    qemu_iovec_reset(&r->iov);
> > +}
> > +
> > +static int bsg_generic_map(SCSIBSGReq *r, QEMUSGList *sg)
> > +{
> > +    int is_write = !scsi_req_is_write(&r->req);
> > +    target_phys_addr_t cur_addr, cur_len, cur_offset = 0;
> > +    void *mem;
> > +    int i;
> > +
> > +    qemu_iovec_reset(&r->iov);
> > +    for (i = 0; i < sg->nsg;) {
> > +        cur_addr = sg->sg[i].base + cur_offset;
> > +        cur_len = sg->sg[i].len - cur_offset;
> > +#ifdef DEBUG_BSG_MAP
> > +        DPRINTF("Using cur_addr: 0x%016lx cur_len: 0x%016lx\n",
> > +            (long unsigned int)cur_addr, (long unsigned int)cur_len);
> 
> Please use TARGET_FMT_plx for cur_addr and cur_len.
> 

Fixed

> > +#endif
> > +        mem = cpu_physical_memory_map(cur_addr, &cur_len, is_write);
> > +        if (!mem)
> > +            goto err;
> > +#ifdef DEBUG_BSG_MAP
> > +        DPRINTF("Adding iovec for mem: %p len: 0x%016lx\n", mem,
> > +            (long unsigned int)cur_len);
> 
> Same here.

Fixed

Thank you for your comments!

--nab