On Mon, 2010-06-14 at 18:11 +0000, Blue Swirl wrote:
> On Mon, Jun 14, 2010 at 9:44 AM, Nicholas A. Bellinger
> <nab@xxxxxxxxxxxxxxx> wrote:
> > From: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
> >
> > 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 <nab@xxxxxxxxxxxxxxx>
> > ---
> > 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 <nab@xxxxxxxxxxxxxxx>
> > + *
> > + * 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
