[Qemu-devel] [PATCH RFC 00/11] QEMU ATAPI-SCSI bridge GSoC project

[Qemu-devel] [PATCH RFC 00/11] QEMU ATAPI-SCSI bridge GSoC project

[Alexander Bezzubikov]

This is my QEMU Google Summer of Code project.
Here I introduce new device - ATAPI-SCSI bridge.
Its purpose is to unify IDE ATAPI CD-ROM emulation
with SCSI CD-ROM emulation to reduce code duplication and squash bugs.
It's purpose is simple - it just forwards ATAPI commands
to SCSI side for parsing and execution.

Example of using:
qemu-system-x86_64 \
	-drive if=none,file=<filename>,id=cdrom \
	-drive if=none,id=fake \
	-device ide-bridge,id=bridge,drive=fake \
	-device scsi-cd,drive=cdrom,bus=bridge.0 \
	-hda diskimage \
	-m 1024 

Alexander Bezzubikov (11):
  ide: ATAPI-SCSI bridge TypeInfo and init function created
  ide: necessary fields for ATAPI-SCSI bridge added
  ide: necessary checks corrected to treat ATAPI-SCSI bridge as CDROM
  scsi: Added function to enable ATAPI-SCSI bridge send SCSI requests
  ide: bridge 'transfer' and 'complete' functions added
  ide: ATAPI-SCSI bridge can now forward requests to SCSI
  ide: Makefile corrected due to bridge creation
  scsi: SCSIDiskReq declaration moved to header
  ide: ide_bridge_do_transfer is treated as PIO transfer
  ide: corrected ATAPI checks to be ignored by ATAPI-SCSI bridge
  ide: bridge functions assigned to SCSIBusInfo, bridge is OK now

 hw/ide/Makefile.objs   |   2 +-
 hw/ide/atapi.c         |  22 +++++++++-
 hw/ide/bridge.c        | 114 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/ide/bridge.h        |  10 +++++
 hw/ide/core.c          |  28 +++++++-----
 hw/ide/internal.h      |   6 ++-
 hw/ide/qdev.c          |  42 +++++++++++++++++-
 hw/scsi/scsi-disk.c    |  55 ++++++++++++++++++------
 include/hw/scsi/scsi.h |  16 +++++++
 9 files changed, 267 insertions(+), 28 deletions(-)
 create mode 100644 hw/ide/bridge.c
 create mode 100644 hw/ide/bridge.h

-- 
2.1.4

[Qemu-devel] [PATCH RFC 01/11] ide: ATAPI-SCSI bridge TypeInfo and init function created

[Alexander Bezzubikov]

Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
---
 hw/ide/bridge.h   |  9 +++++++++
 hw/ide/internal.h |  3 ++-
 hw/ide/qdev.c     | 40 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 1 deletion(-)
 create mode 100644 hw/ide/bridge.h

diff --git a/hw/ide/bridge.h b/hw/ide/bridge.h
new file mode 100644
index 0000000..dca5d73
--- /dev/null
+++ b/hw/ide/bridge.h
@@ -0,0 +1,9 @@
+#ifndef HW_IDE_BRIDGE_H
+#define HW_IDE_BRIDGE_H
+
+#include "hw/ide/internal.h"
+
+void ide_bridge_start_transfer(SCSIRequest *req, uint32_t len);
+void ide_bridge_complete(SCSIRequest *req, uint32_t status, size_t resid);
+
+#endif
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 30fdcbc..f2999ce 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -12,6 +12,7 @@
 #include "sysemu/sysemu.h"
 #include "hw/block/block.h"
 #include "block/scsi.h"
+#include "hw/scsi/scsi.h"
 
 /* debug IDE devices */
 //#define DEBUG_IDE
@@ -317,7 +318,7 @@ typedef struct IDEDMAOps IDEDMAOps;
 #define SMART_DISABLE         0xd9
 #define SMART_STATUS          0xda
 
-typedef enum { IDE_HD, IDE_CD, IDE_CFATA } IDEDriveKind;
+typedef enum { IDE_HD, IDE_CD, IDE_CFATA, IDE_BRIDGE } IDEDriveKind;
 
 typedef void EndTransferFunc(IDEState *);
 
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 788b361..e96a6e9 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -25,6 +25,7 @@
 #include "hw/block/block.h"
 #include "sysemu/sysemu.h"
 #include "qapi/visitor.h"
+#include "hw/ide/bridge.h"
 
 /* --------------------------------- */
 
@@ -143,6 +144,17 @@ int ide_get_bios_chs_trans(BusState *bus, int unit)
     return DO_UPCAST(IDEBus, qbus, bus)->ifs[unit].chs_trans;
 }
 
+/* BusInfo structure for ATAPI-SCSI bridge */
+static const struct SCSIBusInfo atapi_scsi_info = {
+    .tcq = true,
+    .max_target = 0,
+    .max_lun = 0,
+
+    .transfer_data = NULL,
+    .complete = NULL,
+    .cancel = NULL
+};
+
 /* --------------------------------- */
 
 typedef struct IDEDrive {
@@ -184,6 +196,11 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
                        dev->chs_trans) < 0) {
         return -1;
     }
+    if (kind == IDE_BRIDGE) {
+        scsi_bus_new(&dev->scsi_bus, sizeof(dev->scsi_bus), &dev->qdev,
+                     &atapi_scsi_info, NULL);
+        scsi_bus_legacy_handle_cmdline(&dev->scsi_bus, NULL);
+    }
 
     if (!dev->version) {
         dev->version = g_strdup(s->version);
@@ -253,6 +270,11 @@ static int ide_cd_initfn(IDEDevice *dev)
     return ide_dev_initfn(dev, IDE_CD);
 }
 
+static int ide_bridge_initfn(IDEDevice *dev)
+{
+    return ide_dev_initfn(dev, IDE_BRIDGE);
+}
+
 static int ide_drive_initfn(IDEDevice *dev)
 {
     DriveInfo *dinfo = blk_legacy_dinfo(dev->conf.blk);
@@ -314,6 +336,23 @@ static const TypeInfo ide_cd_info = {
     .class_init    = ide_cd_class_init,
 };
 
+static void ide_bridge_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
+    k->init = ide_bridge_initfn;
+    dc->fw_name = "drive";
+    dc->desc = "virtual ATAPI-SCSI bridge";
+    dc->props = ide_cd_properties;
+}
+
+static const TypeInfo ide_bridge_info = {
+    .name          = "ide-bridge",
+    .parent        = TYPE_IDE_DEVICE,
+    .instance_size = sizeof(IDEDrive),
+    .class_init    = ide_bridge_class_init,
+};
+
 static Property ide_drive_properties[] = {
     DEFINE_IDE_DEV_PROPERTIES(),
     DEFINE_PROP_END_OF_LIST(),
@@ -360,6 +399,7 @@ static void ide_register_types(void)
     type_register_static(&ide_bus_info);
     type_register_static(&ide_hd_info);
     type_register_static(&ide_cd_info);
+    type_register_static(&ide_bridge_info);
     type_register_static(&ide_drive_info);
     type_register_static(&ide_device_type_info);
 }
-- 
2.1.4

[Qemu-devel] [PATCH RFC 02/11] ide: necessary fields for ATAPI-SCSI bridge added

[Alexander Bezzubikov]

Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
---
 hw/ide/internal.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index f2999ce..3ebc63c 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -429,6 +429,8 @@ struct IDEState {
     uint8_t *smart_selftest_data;
     /* AHCI */
     int ncq_queues;
+    /* ATAPI-SCSI bridge */
+    SCSIRequest *cur_req;
 };
 
 struct IDEDMAOps {
@@ -493,6 +495,7 @@ struct IDEDevice {
     char *serial;
     char *model;
     uint64_t wwn;
+    SCSIBus *scsi_bus;
 };
 
 /* These are used for the error_status field of IDEBus */
-- 
2.1.4

[Qemu-devel] [PATCH RFC 03/11] ide: necessary checks corrected to treat ATAPI-SCSI bridge as CDROM

[Alexander Bezzubikov]

Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
---
 hw/ide/core.c | 24 ++++++++++++++----------
 hw/ide/qdev.c |  2 +-
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 50449ca..d017d4e 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -348,7 +348,7 @@ static void ide_set_signature(IDEState *s)
     /* put signature */
     s->nsector = 1;
     s->sector = 1;
-    if (s->drive_kind == IDE_CD) {
+    if (s->drive_kind == IDE_CD || s->drive_kind == IDE_BRIDGE) {
         s->lcyl = 0x14;
         s->hcyl = 0xeb;
     } else if (s->blk) {
@@ -1144,7 +1144,7 @@ static bool cmd_data_set_management(IDEState *s, uint8_t cmd)
 
 static bool cmd_identify(IDEState *s, uint8_t cmd)
 {
-    if (s->blk && s->drive_kind != IDE_CD) {
+    if (s->blk && s->drive_kind != IDE_CD && s->drive_kind != IDE_BRIDGE) {
         if (s->drive_kind != IDE_CFATA) {
             ide_identify(s);
         } else {
@@ -1155,7 +1155,7 @@ static bool cmd_identify(IDEState *s, uint8_t cmd)
         ide_set_irq(s->bus);
         return false;
     } else {
-        if (s->drive_kind == IDE_CD) {
+        if (s->drive_kind == IDE_CD || s->drive_kind == IDE_BRIDGE) {
             ide_set_signature(s);
         }
         ide_abort_command(s);
@@ -1232,7 +1232,7 @@ static bool cmd_read_pio(IDEState *s, uint8_t cmd)
 {
     bool lba48 = (cmd == WIN_READ_EXT);
 
-    if (s->drive_kind == IDE_CD) {
+    if (s->drive_kind == IDE_CD || s->drive_kind == IDE_BRIDGE) {
         ide_set_signature(s); /* odd, but ATA4 8.27.5.2 requires it */
         ide_abort_command(s);
         return true;
@@ -1426,7 +1426,7 @@ static bool cmd_exec_dev_diagnostic(IDEState *s, uint8_t cmd)
 {
     ide_set_signature(s);
 
-    if (s->drive_kind == IDE_CD) {
+    if (s->drive_kind == IDE_CD || s->drive_kind == IDE_BRIDGE) {
         s->status = 0; /* ATAPI spec (v6) section 9.10 defines packet
                         * devices to return a clear status register
                         * with READY_STAT *not* set. */
@@ -1731,7 +1731,7 @@ abort_cmd:
 }
 
 #define HD_OK (1u << IDE_HD)
-#define CD_OK (1u << IDE_CD)
+#define CD_OK ((1u << IDE_CD) | (1u << IDE_BRIDGE))
 #define CFA_OK (1u << IDE_CFATA)
 #define HD_CFA_OK (HD_OK | CFA_OK)
 #define ALL_OK (HD_OK | CD_OK | CFA_OK)
@@ -1978,10 +1978,11 @@ void ide_cmd_write(void *opaque, uint32_t addr, uint32_t val)
         /* high to low */
         for(i = 0;i < 2; i++) {
             s = &bus->ifs[i];
-            if (s->drive_kind == IDE_CD)
+            if (s->drive_kind == IDE_CD || s->drive_kind == IDE_BRIDGE) {
                 s->status = 0x00; /* NOTE: READY is _not_ set */
-            else
+            } else {
                 s->status = READY_STAT | SEEK_STAT;
+            }
             ide_set_signature(s);
         }
     }
@@ -2234,7 +2235,7 @@ static void ide_resize_cb(void *opaque)
         ide_cfata_identify_size(s);
     } else {
         /* IDE_CD uses a different set of callbacks entirely. */
-        assert(s->drive_kind != IDE_CD);
+        assert(s->drive_kind != IDE_CD && s->drive_kind != IDE_BRIDGE);
         ide_identify_size(s);
     }
 }
@@ -2274,7 +2275,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
     s->smart_autosave = 1;
     s->smart_errors = 0;
     s->smart_selftest_count = 0;
-    if (kind == IDE_CD) {
+    if (kind == IDE_CD || kind == IDE_BRIDGE) {
         blk_set_dev_ops(blk, &ide_cd_block_ops, s);
         blk_set_guest_block_size(blk, 2048);
     } else {
@@ -2304,6 +2305,9 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
         case IDE_CFATA:
             strcpy(s->drive_model_str, "QEMU MICRODRIVE");
             break;
+        case IDE_BRIDGE:
+            strcpy(s->drive_model_str, "QEMU VIRTUAL ATAPI-SCSI BRIDGE");
+            break;
         default:
             strcpy(s->drive_model_str, "QEMU HARDDISK");
             break;
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index e96a6e9..63090cc 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -182,7 +182,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
     }
 
     blkconf_serial(&dev->conf, &dev->serial);
-    if (kind != IDE_CD) {
+    if (kind != IDE_CD && kind != IDE_BRIDGE) {
         blkconf_geometry(&dev->conf, &dev->chs_trans, 65536, 16, 255, &err);
         if (err) {
             error_report_err(err);
-- 
2.1.4

[Qemu-devel] [PATCH RFC 04/11] scsi: Added function to enable ATAPI-SCSI bridge send SCSI requests

[Alexander Bezzubikov]

Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
---
 hw/ide/internal.h      |  2 +-
 hw/scsi/scsi-disk.c    | 43 +++++++++++++++++++++++++++++++++++++++++++
 include/hw/scsi/scsi.h |  3 +++
 3 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 3ebc63c..fd385bc 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -495,7 +495,7 @@ struct IDEDevice {
     char *serial;
     char *model;
     uint64_t wwn;
-    SCSIBus *scsi_bus;
+    SCSIBus scsi_bus;
 };
 
 /* These are used for the error_status field of IDEBus */
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 64f0694..f67d816 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2440,6 +2440,49 @@ static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun,
     return req;
 }
 
+SCSIRequest *scsi_new_request_from_bridge(SCSIDevice *d, uint32_t tag,
+                                          uint32_t lun, uint8_t *buf,
+                                          void *hba_private)
+{
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
+    SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(d);
+    SCSIRequest *req;
+    const SCSIReqOps *ops;
+    uint8_t command;
+
+    command = buf[0];
+    ops = scsi_disk_reqops_dispatch[command];
+    if (!ops) {
+        ops = &scsi_disk_emulate_reqops;
+    }
+    req = scsi_req_alloc(ops, &s->qdev, tag, lun, hba_private);
+    memcpy(req->cmd.buf, buf, 16);
+
+    SCSICommand cmd = { .len = 0 };
+
+    if (ops != NULL || !sc->parse_cdb) {
+        scsi_req_parse_cdb(d, &cmd, buf);
+    } else {
+        sc->parse_cdb(d, &cmd, buf, hba_private);
+    }
+
+    req->cmd = cmd;
+    req->resid = req->cmd.xfer;
+
+    #ifdef DEBUG_SCSI
+    DPRINTF("Command: lun=%d tag=0x%x data=0x%02x", lun, tag, buf[0]);
+    {
+        int i;
+        for (i = 1; i < scsi_cdb_length(buf); i++) {
+            printf(" 0x%02x", buf[i]);
+        }
+        printf("\n");
+    }
+    #endif
+
+    return req;
+}
+
 #ifdef __linux__
 static int get_device_type(SCSIDiskState *s)
 {
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index cdaf0f8..881ed23 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -249,6 +249,9 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
                             uint32_t tag, uint32_t lun, void *hba_private);
 SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
                           uint8_t *buf, void *hba_private);
+SCSIRequest *scsi_new_request_from_bridge(SCSIDevice *d, uint32_t tag,
+                                          uint32_t lun, uint8_t *buf,
+                                          void *hba_private);
 int32_t scsi_req_enqueue(SCSIRequest *req);
 void scsi_req_free(SCSIRequest *req);
 SCSIRequest *scsi_req_ref(SCSIRequest *req);
-- 
2.1.4

[Qemu-devel] [PATCH RFC 05/11] ide: bridge 'transfer' and 'complete' functions added

[Alexander Bezzubikov]

Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
---
 hw/ide/bridge.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 114 insertions(+)
 create mode 100644 hw/ide/bridge.c

diff --git a/hw/ide/bridge.c b/hw/ide/bridge.c
new file mode 100644
index 0000000..3a534de
--- /dev/null
+++ b/hw/ide/bridge.c
@@ -0,0 +1,114 @@
+#include "hw/ide/bridge.h"
+
+static void ide_bridge_do_transfer(IDEState *s)
+{
+    SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, s->cur_req);
+
+    if (r->buflen > 0) {
+        int size = r->buflen;
+
+        int byte_count_limit = s->lcyl | (s->hcyl << 8);
+        if (byte_count_limit == 0xffff) {
+            byte_count_limit--;
+        }
+        if (size > byte_count_limit) {
+            /* byte count limit must be even if this case */
+            if (byte_count_limit & 1) {
+                byte_count_limit--;
+            }
+            size = byte_count_limit;
+        }
+        s->lcyl = size;
+        s->hcyl = size >> 8;
+        s->nsector = (s->nsector & ~7) | ATAPI_INT_REASON_IO;
+
+        int offset = (r->buflen == r->qiov.size) ? 0 : r->qiov.size - r->buflen;
+        r->buflen -= size;
+
+        ide_transfer_start(s, s->io_buffer + offset, size,
+                           &ide_bridge_do_transfer);
+        ide_set_irq(s->bus);
+    } else {
+        scsi_req_complete(s->cur_req, GOOD);
+    }
+}
+
+static void ide_bridge_dma_complete(void *opaque, int ret)
+{
+    IDEState *s = opaque;
+
+    s->io_buffer_size = s->bus->dma->iov.iov_len;
+    s->nsector = (s->nsector & ~7) | ATAPI_INT_REASON_IO | ATAPI_INT_REASON_CD;
+    s->bus->dma->ops->rw_buf(s->bus->dma, 1);
+    scsi_req_complete(s->cur_req, GOOD);
+
+    s->status = READY_STAT | SEEK_STAT;
+
+    ide_set_irq(s->bus);
+    ide_set_inactive(s, false);
+}
+
+void ide_bridge_start_transfer(SCSIRequest *req, uint32_t len)
+{
+    IDEDevice *dev = IDE_DEVICE(req->bus->qbus.parent);
+    IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
+    IDEState *s = bus->ifs;
+    SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
+
+    int cmd = req->cmd.buf[0];
+    if (cmd == READ_10) {
+        if (s->feature & 1) {
+            s->status = READY_STAT | SEEK_STAT | DRQ_STAT | BUSY_STAT;
+            qemu_iovec_clone(&s->bus->dma->qiov, &r->qiov, NULL);
+            qemu_iovec_to_buf(&r->qiov, 0, s->io_buffer, r->qiov.size);
+        } else {
+            qemu_iovec_to_buf(&r->qiov, 0, s->io_buffer, r->qiov.size);
+        }
+    } else {
+        if (cmd == INQUIRY) {
+            len = 36;
+        }
+        r->iov.iov_len = len;
+        qemu_iovec_concat_iov(&r->qiov, &r->iov, len, 0, len);
+        qemu_iovec_to_buf(&r->qiov, 0, s->io_buffer, r->qiov.size);
+    }
+
+    s->io_buffer_index = 0;
+    s->status = READY_STAT | SEEK_STAT;
+
+    if (cmd != TEST_UNIT_READY && cmd != ALLOW_MEDIUM_REMOVAL) {
+        if (s->feature & 1) {
+            s->io_buffer_index = 0;
+            s->bus->retry_unit = s->unit;
+            s->bus->retry_sector_num = ide_get_sector(s);
+            s->bus->retry_nsector = s->nsector;
+
+            s->bus->dma->iov.iov_base = (void *)(s->io_buffer);
+            s->bus->dma->iov.iov_len = r->qiov.size;
+
+            if (cmd != READ_10) {
+                s->status = READY_STAT | SEEK_STAT | DRQ_STAT;
+            }
+
+            if (s->bus->dma->ops->start_dma) {
+                s->bus->dma->ops->start_dma(s->bus->dma, s,
+                                            ide_bridge_dma_complete);
+            }
+        } else {
+            r->buflen = r->qiov.size;
+            ide_bridge_do_transfer(s);
+        }
+    } else {
+        scsi_req_complete(req, GOOD);
+    }
+}
+
+void ide_bridge_complete(SCSIRequest *req, uint32_t status, size_t resid)
+{
+    IDEDevice *dev = IDE_DEVICE(req->bus->qbus.parent);
+    IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
+    IDEState *s = bus->ifs;
+
+    ide_atapi_cmd_ok(s);
+    ide_set_irq(s->bus);
+}
-- 
2.1.4

[Qemu-devel] [PATCH RFC 06/11] ide: ATAPI-SCSI bridge can now forward requests to SCSI

[Alexander Bezzubikov]

Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
---
 hw/ide/atapi.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 79dd167..6e73910 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -1257,5 +1257,21 @@ void ide_atapi_cmd(IDEState *s)
         return;
     }
 
+    if (s->drive_kind == IDE_BRIDGE) {
+        IDEDevice *dev = s->bus->master;
+        SCSIDevice *scsi_dev = scsi_device_find(&dev->scsi_bus, 0, 0, 0);
+        s->cur_req = scsi_new_request_from_bridge(scsi_dev, 0, 0, buf, NULL);
+
+        /* Looks like a hack, but it's better than placing it at scsi_do_read */
+        if (buf[0] == READ_10) {
+            s->status |= BUSY_STAT;
+        }
+
+        if (scsi_req_enqueue(s->cur_req)) {
+            scsi_req_continue(s->cur_req);
+        }
+        return;
+    }
+
     ide_atapi_cmd_error(s, ILLEGAL_REQUEST, ASC_ILLEGAL_OPCODE);
 }
-- 
2.1.4

[Qemu-devel] [PATCH RFC 07/11] ide: Makefile corrected due to bridge creation

[Alexander Bezzubikov]

Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
---
 hw/ide/Makefile.objs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/Makefile.objs b/hw/ide/Makefile.objs
index 729e9bd..f54f275 100644
--- a/hw/ide/Makefile.objs
+++ b/hw/ide/Makefile.objs
@@ -1,4 +1,4 @@
-common-obj-$(CONFIG_IDE_CORE) += core.o atapi.o
+common-obj-$(CONFIG_IDE_CORE) += core.o atapi.o bridge.o
 common-obj-$(CONFIG_IDE_QDEV) += qdev.o
 common-obj-$(CONFIG_IDE_PCI) += pci.o
 common-obj-$(CONFIG_IDE_ISA) += isa.o
-- 
2.1.4

[Qemu-devel] [PATCH RFC 08/11] scsi: SCSIDiskReq declaration moved to header

[Alexander Bezzubikov]

Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
---
 hw/scsi/scsi-disk.c    | 12 ------------
 include/hw/scsi/scsi.h | 13 +++++++++++++
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index f67d816..9d5f0a4 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -53,18 +53,6 @@ do { printf("scsi-disk: " fmt , ## __VA_ARGS__); } while (0)
 
 typedef struct SCSIDiskState SCSIDiskState;
 
-typedef struct SCSIDiskReq {
-    SCSIRequest req;
-    /* Both sector and sector_count are in terms of qemu 512 byte blocks.  */
-    uint64_t sector;
-    uint32_t sector_count;
-    uint32_t buflen;
-    bool started;
-    struct iovec iov;
-    QEMUIOVector qiov;
-    BlockAcctCookie acct;
-} SCSIDiskReq;
-
 #define SCSI_DISK_F_REMOVABLE             0
 #define SCSI_DISK_F_DPOFUA                1
 #define SCSI_DISK_F_NO_REMOVABLE_DEVOPS   2
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 881ed23..de0546e 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -6,6 +6,7 @@
 #include "hw/block/block.h"
 #include "sysemu/sysemu.h"
 #include "qemu/notify.h"
+#include "block/accounting.h"
 
 #define MAX_SCSI_DEVS	255
 
@@ -120,6 +121,18 @@ extern const VMStateDescription vmstate_scsi_device;
     .offset     = vmstate_offset_value(_state, _field, SCSIDevice),  \
 }
 
+typedef struct SCSIDiskReq {
+    SCSIRequest req;
+    /* Both sector and sector_count are in terms of qemu 512 byte blocks.  */
+    uint64_t sector;
+    uint32_t sector_count;
+    uint32_t buflen;
+    bool started;
+    struct iovec iov;
+    QEMUIOVector qiov;
+    BlockAcctCookie acct;
+} SCSIDiskReq;
+
 /* cdrom.c */
 int cdrom_read_toc(int nb_sectors, uint8_t *buf, int msf, int start_track);
 int cdrom_read_toc_raw(int nb_sectors, uint8_t *buf, int msf, int session_num);
-- 
2.1.4

[Qemu-devel] [PATCH RFC 09/11] ide: ide_bridge_do_transfer is treated as PIO transfer

[Alexander Bezzubikov]

Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
---
 hw/ide/bridge.c | 2 +-
 hw/ide/bridge.h | 1 +
 hw/ide/core.c   | 4 +++-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/ide/bridge.c b/hw/ide/bridge.c
index 3a534de..4a5251b 100644
--- a/hw/ide/bridge.c
+++ b/hw/ide/bridge.c
@@ -1,6 +1,6 @@
 #include "hw/ide/bridge.h"
 
-static void ide_bridge_do_transfer(IDEState *s)
+void ide_bridge_do_transfer(IDEState *s)
 {
     SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, s->cur_req);
 
diff --git a/hw/ide/bridge.h b/hw/ide/bridge.h
index dca5d73..59f2f25 100644
--- a/hw/ide/bridge.h
+++ b/hw/ide/bridge.h
@@ -5,5 +5,6 @@
 
 void ide_bridge_start_transfer(SCSIRequest *req, uint32_t len);
 void ide_bridge_complete(SCSIRequest *req, uint32_t status, size_t resid);
+void ide_bridge_do_transfer(IDEState *s);
 
 #endif
diff --git a/hw/ide/core.c b/hw/ide/core.c
index d017d4e..99b955b 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -34,6 +34,7 @@
 #include "sysemu/block-backend.h"
 
 #include <hw/ide/internal.h>
+#include "hw/ide/bridge.h"
 
 /* These values were based on a Seagate ST3500418AS but have been modified
    to make more sense in QEMU */
@@ -2002,7 +2003,8 @@ static bool ide_is_pio_out(IDEState *s)
     } else if (s->end_transfer_func == ide_sector_read ||
                s->end_transfer_func == ide_transfer_stop ||
                s->end_transfer_func == ide_atapi_cmd_reply_end ||
-               s->end_transfer_func == ide_dummy_transfer_stop) {
+               s->end_transfer_func == ide_dummy_transfer_stop ||
+               s->end_transfer_func == ide_bridge_do_transfer) {
         return true;
     }
 
-- 
2.1.4

[Qemu-devel] [PATCH RFC 10/11] ide: corrected ATAPI checks to be ignored by ATAPI-SCSI bridge

[Alexander Bezzubikov]

Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
---
 hw/ide/atapi.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 6e73910..896f595 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -1230,7 +1230,8 @@ void ide_atapi_cmd(IDEState *s)
      * states rely on this behavior.
      */
     if (!(atapi_cmd_table[s->io_buffer[0]].flags & ALLOW_UA) &&
-        !s->tray_open && blk_is_inserted(s->blk) && s->cdrom_changed) {
+        (s->drive_kind != IDE_BRIDGE && !s->tray_open &&
+        blk_is_inserted(s->blk) && s->cdrom_changed)) {
 
         if (s->cdrom_changed == 1) {
             ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT);
@@ -1245,18 +1246,13 @@ void ide_atapi_cmd(IDEState *s)
 
     /* Report a Not Ready condition if appropriate for the command */
     if ((atapi_cmd_table[s->io_buffer[0]].flags & CHECK_READY) &&
-        (!media_present(s) || !blk_is_inserted(s->blk)))
+        (s->drive_kind != IDE_BRIDGE &&
+            (!media_present(s) || !blk_is_inserted(s->blk))))
     {
         ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT);
         return;
     }
 
-    /* Execute the command */
-    if (atapi_cmd_table[s->io_buffer[0]].handler) {
-        atapi_cmd_table[s->io_buffer[0]].handler(s, buf);
-        return;
-    }
-
     if (s->drive_kind == IDE_BRIDGE) {
         IDEDevice *dev = s->bus->master;
         SCSIDevice *scsi_dev = scsi_device_find(&dev->scsi_bus, 0, 0, 0);
@@ -1273,5 +1269,11 @@ void ide_atapi_cmd(IDEState *s)
         return;
     }
 
+    /* Execute the command */
+    if (atapi_cmd_table[s->io_buffer[0]].handler) {
+        atapi_cmd_table[s->io_buffer[0]].handler(s, buf);
+        return;
+    }
+
     ide_atapi_cmd_error(s, ILLEGAL_REQUEST, ASC_ILLEGAL_OPCODE);
 }
-- 
2.1.4

[Qemu-devel] [PATCH RFC 11/11] ide: bridge functions assigned to SCSIBusInfo, bridge is OK now

[Alexander Bezzubikov]

Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
---
 hw/ide/qdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 63090cc..773a587 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -150,8 +150,8 @@ static const struct SCSIBusInfo atapi_scsi_info = {
     .max_target = 0,
     .max_lun = 0,
 
-    .transfer_data = NULL,
-    .complete = NULL,
+    .transfer_data = ide_bridge_start_transfer,
+    .complete = ide_bridge_complete,
     .cancel = NULL
 };
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH RFC 01/11] ide: ATAPI-SCSI bridge TypeInfo and init function created

[Hannes Reinecke]

On 08/18/2015 01:42 AM, Alexander Bezzubikov wrote:
> Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
> ---
>  hw/ide/bridge.h   |  9 +++++++++
>  hw/ide/internal.h |  3 ++-
>  hw/ide/qdev.c     | 40 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 51 insertions(+), 1 deletion(-)
>  create mode 100644 hw/ide/bridge.h
> 
> diff --git a/hw/ide/bridge.h b/hw/ide/bridge.h
> new file mode 100644
> index 0000000..dca5d73
> --- /dev/null
> +++ b/hw/ide/bridge.h
> @@ -0,0 +1,9 @@
> +#ifndef HW_IDE_BRIDGE_H
> +#define HW_IDE_BRIDGE_H
> +
> +#include "hw/ide/internal.h"
> +
> +void ide_bridge_start_transfer(SCSIRequest *req, uint32_t len);
> +void ide_bridge_complete(SCSIRequest *req, uint32_t status, size_t resid);
> +
> +#endif
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 30fdcbc..f2999ce 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -12,6 +12,7 @@
>  #include "sysemu/sysemu.h"
>  #include "hw/block/block.h"
>  #include "block/scsi.h"
> +#include "hw/scsi/scsi.h"
>  
>  /* debug IDE devices */
>  //#define DEBUG_IDE
> @@ -317,7 +318,7 @@ typedef struct IDEDMAOps IDEDMAOps;
>  #define SMART_DISABLE         0xd9
>  #define SMART_STATUS          0xda
>  
> -typedef enum { IDE_HD, IDE_CD, IDE_CFATA } IDEDriveKind;
> +typedef enum { IDE_HD, IDE_CD, IDE_CFATA, IDE_BRIDGE } IDEDriveKind;
>  
>  typedef void EndTransferFunc(IDEState *);
>  
This change doesn't need the scsi.h include; please move it to the
patch where the SCSI prototypes are used.

> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index 788b361..e96a6e9 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -25,6 +25,7 @@
>  #include "hw/block/block.h"
>  #include "sysemu/sysemu.h"
>  #include "qapi/visitor.h"
> +#include "hw/ide/bridge.h"
>  
>  /* --------------------------------- */
>  
> @@ -143,6 +144,17 @@ int ide_get_bios_chs_trans(BusState *bus, int unit)
>      return DO_UPCAST(IDEBus, qbus, bus)->ifs[unit].chs_trans;
>  }
>  
> +/* BusInfo structure for ATAPI-SCSI bridge */
> +static const struct SCSIBusInfo atapi_scsi_info = {
> +    .tcq = true,
> +    .max_target = 0,
> +    .max_lun = 0,
> +
> +    .transfer_data = NULL,
> +    .complete = NULL,
> +    .cancel = NULL
> +};
> +
>  /* --------------------------------- */
>  
>  typedef struct IDEDrive {
> @@ -184,6 +196,11 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>                         dev->chs_trans) < 0) {
>          return -1;
>      }
> +    if (kind == IDE_BRIDGE) {
> +        scsi_bus_new(&dev->scsi_bus, sizeof(dev->scsi_bus), &dev->qdev,
> +                     &atapi_scsi_info, NULL);
> +        scsi_bus_legacy_handle_cmdline(&dev->scsi_bus, NULL);
> +    }
>  
>      if (!dev->version) {
>          dev->version = g_strdup(s->version);
> @@ -253,6 +270,11 @@ static int ide_cd_initfn(IDEDevice *dev)
>      return ide_dev_initfn(dev, IDE_CD);
>  }
>  
> +static int ide_bridge_initfn(IDEDevice *dev)
> +{
> +    return ide_dev_initfn(dev, IDE_BRIDGE);
> +}
> +
>  static int ide_drive_initfn(IDEDevice *dev)
>  {
>      DriveInfo *dinfo = blk_legacy_dinfo(dev->conf.blk);
> @@ -314,6 +336,23 @@ static const TypeInfo ide_cd_info = {
>      .class_init    = ide_cd_class_init,
>  };
>  
> +static void ide_bridge_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
> +    k->init = ide_bridge_initfn;
> +    dc->fw_name = "drive";
> +    dc->desc = "virtual ATAPI-SCSI bridge";
> +    dc->props = ide_cd_properties;
> +}
> +
> +static const TypeInfo ide_bridge_info = {
> +    .name          = "ide-bridge",
> +    .parent        = TYPE_IDE_DEVICE,
> +    .instance_size = sizeof(IDEDrive),
> +    .class_init    = ide_bridge_class_init,
> +};
> +
>  static Property ide_drive_properties[] = {
>      DEFINE_IDE_DEV_PROPERTIES(),
>      DEFINE_PROP_END_OF_LIST(),
> @@ -360,6 +399,7 @@ static void ide_register_types(void)
>      type_register_static(&ide_bus_info);
>      type_register_static(&ide_hd_info);
>      type_register_static(&ide_cd_info);
> +    type_register_static(&ide_bridge_info);
>      type_register_static(&ide_drive_info);
>      type_register_static(&ide_device_type_info);
>  }
> 
The rest is okay.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [Qemu-devel] [PATCH RFC 02/11] ide: necessary fields for ATAPI-SCSI bridge added

[Hannes Reinecke]

On 08/18/2015 01:42 AM, Alexander Bezzubikov wrote:
> Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
> ---
>  hw/ide/internal.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index f2999ce..3ebc63c 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -429,6 +429,8 @@ struct IDEState {
>      uint8_t *smart_selftest_data;
>      /* AHCI */
>      int ncq_queues;
> +    /* ATAPI-SCSI bridge */
> +    SCSIRequest *cur_req;
>  };
>  
>  struct IDEDMAOps {
> @@ -493,6 +495,7 @@ struct IDEDevice {
>      char *serial;
>      char *model;
>      uint64_t wwn;
> +    SCSIBus *scsi_bus;
>  };
>  
>  /* These are used for the error_status field of IDEBus */
> 
Hmm. I would rather merge this with the patch where the
fields are used; as it stands it looks a bit pointless.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [Qemu-devel] [PATCH RFC 03/11] ide: necessary checks corrected to treat ATAPI-SCSI bridge as CDROM

[Hannes Reinecke]

On 08/18/2015 01:42 AM, Alexander Bezzubikov wrote:
> Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
> ---
>  hw/ide/core.c | 24 ++++++++++++++----------
>  hw/ide/qdev.c |  2 +-
>  2 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 50449ca..d017d4e 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -348,7 +348,7 @@ static void ide_set_signature(IDEState *s)
>      /* put signature */
>      s->nsector = 1;
>      s->sector = 1;
> -    if (s->drive_kind == IDE_CD) {
> +    if (s->drive_kind == IDE_CD || s->drive_kind == IDE_BRIDGE) {
>          s->lcyl = 0x14;
>          s->hcyl = 0xeb;
>      } else if (s->blk) {
> @@ -1144,7 +1144,7 @@ static bool cmd_data_set_management(IDEState *s, uint8_t cmd)
>  
>  static bool cmd_identify(IDEState *s, uint8_t cmd)
>  {
> -    if (s->blk && s->drive_kind != IDE_CD) {
> +    if (s->blk && s->drive_kind != IDE_CD && s->drive_kind != IDE_BRIDGE) {
>          if (s->drive_kind != IDE_CFATA) {
>              ide_identify(s);
>          } else {
> @@ -1155,7 +1155,7 @@ static bool cmd_identify(IDEState *s, uint8_t cmd)
>          ide_set_irq(s->bus);
>          return false;
>      } else {
> -        if (s->drive_kind == IDE_CD) {
> +        if (s->drive_kind == IDE_CD || s->drive_kind == IDE_BRIDGE) {
>              ide_set_signature(s);
>          }
>          ide_abort_command(s);
> @@ -1232,7 +1232,7 @@ static bool cmd_read_pio(IDEState *s, uint8_t cmd)
>  {
>      bool lba48 = (cmd == WIN_READ_EXT);
>  
> -    if (s->drive_kind == IDE_CD) {
> +    if (s->drive_kind == IDE_CD || s->drive_kind == IDE_BRIDGE) {
>          ide_set_signature(s); /* odd, but ATA4 8.27.5.2 requires it */
>          ide_abort_command(s);
>          return true;
> @@ -1426,7 +1426,7 @@ static bool cmd_exec_dev_diagnostic(IDEState *s, uint8_t cmd)
>  {
>      ide_set_signature(s);
>  
> -    if (s->drive_kind == IDE_CD) {
> +    if (s->drive_kind == IDE_CD || s->drive_kind == IDE_BRIDGE) {
>          s->status = 0; /* ATAPI spec (v6) section 9.10 defines packet
>                          * devices to return a clear status register
>                          * with READY_STAT *not* set. */
> @@ -1731,7 +1731,7 @@ abort_cmd:
>  }
>  
>  #define HD_OK (1u << IDE_HD)
> -#define CD_OK (1u << IDE_CD)
> +#define CD_OK ((1u << IDE_CD) | (1u << IDE_BRIDGE))
>  #define CFA_OK (1u << IDE_CFATA)
>  #define HD_CFA_OK (HD_OK | CFA_OK)
>  #define ALL_OK (HD_OK | CD_OK | CFA_OK)
> @@ -1978,10 +1978,11 @@ void ide_cmd_write(void *opaque, uint32_t addr, uint32_t val)
>          /* high to low */
>          for(i = 0;i < 2; i++) {
>              s = &bus->ifs[i];
> -            if (s->drive_kind == IDE_CD)
> +            if (s->drive_kind == IDE_CD || s->drive_kind == IDE_BRIDGE) {
>                  s->status = 0x00; /* NOTE: READY is _not_ set */
> -            else
> +            } else {
>                  s->status = READY_STAT | SEEK_STAT;
> +            }
>              ide_set_signature(s);
>          }
>      }
> @@ -2234,7 +2235,7 @@ static void ide_resize_cb(void *opaque)
>          ide_cfata_identify_size(s);
>      } else {
>          /* IDE_CD uses a different set of callbacks entirely. */
> -        assert(s->drive_kind != IDE_CD);
> +        assert(s->drive_kind != IDE_CD && s->drive_kind != IDE_BRIDGE);
>          ide_identify_size(s);
>      }
>  }
> @@ -2274,7 +2275,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
>      s->smart_autosave = 1;
>      s->smart_errors = 0;
>      s->smart_selftest_count = 0;
> -    if (kind == IDE_CD) {
> +    if (kind == IDE_CD || kind == IDE_BRIDGE) {
>          blk_set_dev_ops(blk, &ide_cd_block_ops, s);
>          blk_set_guest_block_size(blk, 2048);
>      } else {
> @@ -2304,6 +2305,9 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
>          case IDE_CFATA:
>              strcpy(s->drive_model_str, "QEMU MICRODRIVE");
>              break;
> +        case IDE_BRIDGE:
> +            strcpy(s->drive_model_str, "QEMU VIRTUAL ATAPI-SCSI BRIDGE");
> +            break;
>          default:
>              strcpy(s->drive_model_str, "QEMU HARDDISK");
>              break;
> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index e96a6e9..63090cc 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -182,7 +182,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>      }
>  
>      blkconf_serial(&dev->conf, &dev->serial);
> -    if (kind != IDE_CD) {
> +    if (kind != IDE_CD && kind != IDE_BRIDGE) {
>          blkconf_geometry(&dev->conf, &dev->chs_trans, 65536, 16, 255, &err);
>          if (err) {
>              error_report_err(err);
> 
Looks okay.

Acked-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [Qemu-devel] [PATCH RFC 04/11] scsi: Added function to enable ATAPI-SCSI bridge send SCSI requests

[Hannes Reinecke]

On 08/18/2015 01:42 AM, Alexander Bezzubikov wrote:
> Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
> ---
>  hw/ide/internal.h      |  2 +-
>  hw/scsi/scsi-disk.c    | 43 +++++++++++++++++++++++++++++++++++++++++++
>  include/hw/scsi/scsi.h |  3 +++
>  3 files changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 3ebc63c..fd385bc 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -495,7 +495,7 @@ struct IDEDevice {
>      char *serial;
>      char *model;
>      uint64_t wwn;
> -    SCSIBus *scsi_bus;
> +    SCSIBus scsi_bus;
>  };
>  
>  /* These are used for the error_status field of IDEBus */
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 64f0694..f67d816 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -2440,6 +2440,49 @@ static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun,
>      return req;
>  }
>  
> +SCSIRequest *scsi_new_request_from_bridge(SCSIDevice *d, uint32_t tag,
> +                                          uint32_t lun, uint8_t *buf,
> +                                          void *hba_private)
> +{
> +    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
> +    SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(d);
> +    SCSIRequest *req;
> +    const SCSIReqOps *ops;
> +    uint8_t command;
> +
> +    command = buf[0];
> +    ops = scsi_disk_reqops_dispatch[command];
> +    if (!ops) {
> +        ops = &scsi_disk_emulate_reqops;
> +    }
> +    req = scsi_req_alloc(ops, &s->qdev, tag, lun, hba_private);
> +    memcpy(req->cmd.buf, buf, 16);
> +
> +    SCSICommand cmd = { .len = 0 };
> +
> +    if (ops != NULL || !sc->parse_cdb) {
> +        scsi_req_parse_cdb(d, &cmd, buf);
> +    } else {
> +        sc->parse_cdb(d, &cmd, buf, hba_private);
> +    }
> +
> +    req->cmd = cmd;
> +    req->resid = req->cmd.xfer;
> +
> +    #ifdef DEBUG_SCSI
> +    DPRINTF("Command: lun=%d tag=0x%x data=0x%02x", lun, tag, buf[0]);
> +    {
> +        int i;
> +        for (i = 1; i < scsi_cdb_length(buf); i++) {
> +            printf(" 0x%02x", buf[i]);
> +        }
> +        printf("\n");
> +    }
> +    #endif
> +
> +    return req;
> +}
> +
>  #ifdef __linux__
>  static int get_device_type(SCSIDiskState *s)
>  {
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index cdaf0f8..881ed23 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -249,6 +249,9 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
>                              uint32_t tag, uint32_t lun, void *hba_private);
>  SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
>                            uint8_t *buf, void *hba_private);
> +SCSIRequest *scsi_new_request_from_bridge(SCSIDevice *d, uint32_t tag,
> +                                          uint32_t lun, uint8_t *buf,
> +                                          void *hba_private);
>  int32_t scsi_req_enqueue(SCSIRequest *req);
>  void scsi_req_free(SCSIRequest *req);
>  SCSIRequest *scsi_req_ref(SCSIRequest *req);
> 
Again, I would merge it with the latter patch where the function is
actually used.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [Qemu-devel] [PATCH RFC 05/11] ide: bridge 'transfer' and 'complete' functions added

[Hannes Reinecke]

On 08/18/2015 01:42 AM, Alexander Bezzubikov wrote:
> Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
> ---
>  hw/ide/bridge.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 114 insertions(+)
>  create mode 100644 hw/ide/bridge.c
> 
> diff --git a/hw/ide/bridge.c b/hw/ide/bridge.c
> new file mode 100644
> index 0000000..3a534de
> --- /dev/null
> +++ b/hw/ide/bridge.c
> @@ -0,0 +1,114 @@
> +#include "hw/ide/bridge.h"
> +
> +static void ide_bridge_do_transfer(IDEState *s)
> +{
> +    SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, s->cur_req);
> +
> +    if (r->buflen > 0) {
> +        int size = r->buflen;
> +
> +        int byte_count_limit = s->lcyl | (s->hcyl << 8);
> +        if (byte_count_limit == 0xffff) {
> +            byte_count_limit--;
> +        }
> +        if (size > byte_count_limit) {
> +            /* byte count limit must be even if this case */
> +            if (byte_count_limit & 1) {
> +                byte_count_limit--;
> +            }
> +            size = byte_count_limit;
> +        }
> +        s->lcyl = size;
> +        s->hcyl = size >> 8;
> +        s->nsector = (s->nsector & ~7) | ATAPI_INT_REASON_IO;
> +
> +        int offset = (r->buflen == r->qiov.size) ? 0 : r->qiov.size - r->buflen;
> +        r->buflen -= size;
> +
> +        ide_transfer_start(s, s->io_buffer + offset, size,
> +                           &ide_bridge_do_transfer);
> +        ide_set_irq(s->bus);
> +    } else {
> +        scsi_req_complete(s->cur_req, GOOD);
> +    }
> +}
> +
> +static void ide_bridge_dma_complete(void *opaque, int ret)
> +{
> +    IDEState *s = opaque;
> +
> +    s->io_buffer_size = s->bus->dma->iov.iov_len;
> +    s->nsector = (s->nsector & ~7) | ATAPI_INT_REASON_IO | ATAPI_INT_REASON_CD;
> +    s->bus->dma->ops->rw_buf(s->bus->dma, 1);
> +    scsi_req_complete(s->cur_req, GOOD);
> +
> +    s->status = READY_STAT | SEEK_STAT;
> +
> +    ide_set_irq(s->bus);
> +    ide_set_inactive(s, false);
> +}
> +
> +void ide_bridge_start_transfer(SCSIRequest *req, uint32_t len)
> +{
> +    IDEDevice *dev = IDE_DEVICE(req->bus->qbus.parent);
> +    IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
> +    IDEState *s = bus->ifs;
> +    SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
> +
> +    int cmd = req->cmd.buf[0];
> +    if (cmd == READ_10) {
> +        if (s->feature & 1) {
> +            s->status = READY_STAT | SEEK_STAT | DRQ_STAT | BUSY_STAT;
> +            qemu_iovec_clone(&s->bus->dma->qiov, &r->qiov, NULL);
> +            qemu_iovec_to_buf(&r->qiov, 0, s->io_buffer, r->qiov.size);
> +        } else {
> +            qemu_iovec_to_buf(&r->qiov, 0, s->io_buffer, r->qiov.size);
> +        }
> +    } else {
> +        if (cmd == INQUIRY) {
> +            len = 36;
> +        }
> +        r->iov.iov_len = len;
> +        qemu_iovec_concat_iov(&r->qiov, &r->iov, len, 0, len);
> +        qemu_iovec_to_buf(&r->qiov, 0, s->io_buffer, r->qiov.size);
> +    }
> +
> +    s->io_buffer_index = 0;
> +    s->status = READY_STAT | SEEK_STAT;
> +
> +    if (cmd != TEST_UNIT_READY && cmd != ALLOW_MEDIUM_REMOVAL) {
> +        if (s->feature & 1) {
> +            s->io_buffer_index = 0;
> +            s->bus->retry_unit = s->unit;
> +            s->bus->retry_sector_num = ide_get_sector(s);
> +            s->bus->retry_nsector = s->nsector;
> +
> +            s->bus->dma->iov.iov_base = (void *)(s->io_buffer);
> +            s->bus->dma->iov.iov_len = r->qiov.size;
> +
> +            if (cmd != READ_10) {
> +                s->status = READY_STAT | SEEK_STAT | DRQ_STAT;
> +            }
> +
> +            if (s->bus->dma->ops->start_dma) {
> +                s->bus->dma->ops->start_dma(s->bus->dma, s,
> +                                            ide_bridge_dma_complete);
> +            }
> +        } else {
> +            r->buflen = r->qiov.size;
> +            ide_bridge_do_transfer(s);
> +        }
> +    } else {
> +        scsi_req_complete(req, GOOD);
> +    }
> +}
> +
> +void ide_bridge_complete(SCSIRequest *req, uint32_t status, size_t resid)
> +{
> +    IDEDevice *dev = IDE_DEVICE(req->bus->qbus.parent);
> +    IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
> +    IDEState *s = bus->ifs;
> +
> +    ide_atapi_cmd_ok(s);
> +    ide_set_irq(s->bus);
> +}
> 
Same here; best to merge this and the previous patch into the next one.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [Qemu-devel] [PATCH RFC 06/11] ide: ATAPI-SCSI bridge can now forward requests to SCSI

[Hannes Reinecke]

On 08/18/2015 01:42 AM, Alexander Bezzubikov wrote:
> Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
> ---
>  hw/ide/atapi.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 79dd167..6e73910 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -1257,5 +1257,21 @@ void ide_atapi_cmd(IDEState *s)
>          return;
>      }
>  
> +    if (s->drive_kind == IDE_BRIDGE) {
> +        IDEDevice *dev = s->bus->master;
> +        SCSIDevice *scsi_dev = scsi_device_find(&dev->scsi_bus, 0, 0, 0);
> +        s->cur_req = scsi_new_request_from_bridge(scsi_dev, 0, 0, buf, NULL);
> +
> +        /* Looks like a hack, but it's better than placing it at scsi_do_read */

This needs some more explanations on why it's required.

> +        if (buf[0] == READ_10) {
> +            s->status |= BUSY_STAT;
> +        }
> +
> +        if (scsi_req_enqueue(s->cur_req)) {
> +            scsi_req_continue(s->cur_req);
> +        }
> +        return;
> +    }
> +
>      ide_atapi_cmd_error(s, ILLEGAL_REQUEST, ASC_ILLEGAL_OPCODE);
>  }
> 
And this patch should be merged with the two previous ones.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [Qemu-devel] [PATCH RFC 07/11] ide: Makefile corrected due to bridge creation

[Hannes Reinecke]

On 08/18/2015 01:42 AM, Alexander Bezzubikov wrote:
> Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
> ---
>  hw/ide/Makefile.objs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ide/Makefile.objs b/hw/ide/Makefile.objs
> index 729e9bd..f54f275 100644
> --- a/hw/ide/Makefile.objs
> +++ b/hw/ide/Makefile.objs
> @@ -1,4 +1,4 @@
> -common-obj-$(CONFIG_IDE_CORE) += core.o atapi.o
> +common-obj-$(CONFIG_IDE_CORE) += core.o atapi.o bridge.o
>  common-obj-$(CONFIG_IDE_QDEV) += qdev.o
>  common-obj-$(CONFIG_IDE_PCI) += pci.o
>  common-obj-$(CONFIG_IDE_ISA) += isa.o
> 
And this should be merged with the patch which creates 'bridge.c'

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [Qemu-devel] [PATCH RFC 08/11] scsi: SCSIDiskReq declaration moved to header

[Hannes Reinecke]

On 08/18/2015 01:42 AM, Alexander Bezzubikov wrote:
> Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
> ---
>  hw/scsi/scsi-disk.c    | 12 ------------
>  include/hw/scsi/scsi.h | 13 +++++++++++++
>  2 files changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index f67d816..9d5f0a4 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -53,18 +53,6 @@ do { printf("scsi-disk: " fmt , ## __VA_ARGS__); } while (0)
>  
>  typedef struct SCSIDiskState SCSIDiskState;
>  
> -typedef struct SCSIDiskReq {
> -    SCSIRequest req;
> -    /* Both sector and sector_count are in terms of qemu 512 byte blocks.  */
> -    uint64_t sector;
> -    uint32_t sector_count;
> -    uint32_t buflen;
> -    bool started;
> -    struct iovec iov;
> -    QEMUIOVector qiov;
> -    BlockAcctCookie acct;
> -} SCSIDiskReq;
> -
>  #define SCSI_DISK_F_REMOVABLE             0
>  #define SCSI_DISK_F_DPOFUA                1
>  #define SCSI_DISK_F_NO_REMOVABLE_DEVOPS   2
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index 881ed23..de0546e 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -6,6 +6,7 @@
>  #include "hw/block/block.h"
>  #include "sysemu/sysemu.h"
>  #include "qemu/notify.h"
> +#include "block/accounting.h"
>  
>  #define MAX_SCSI_DEVS	255
>  
> @@ -120,6 +121,18 @@ extern const VMStateDescription vmstate_scsi_device;
>      .offset     = vmstate_offset_value(_state, _field, SCSIDevice),  \
>  }
>  
> +typedef struct SCSIDiskReq {
> +    SCSIRequest req;
> +    /* Both sector and sector_count are in terms of qemu 512 byte blocks.  */
> +    uint64_t sector;
> +    uint32_t sector_count;
> +    uint32_t buflen;
> +    bool started;
> +    struct iovec iov;
> +    QEMUIOVector qiov;
> +    BlockAcctCookie acct;
> +} SCSIDiskReq;
> +
>  /* cdrom.c */
>  int cdrom_read_toc(int nb_sectors, uint8_t *buf, int msf, int start_track);
>  int cdrom_read_toc_raw(int nb_sectors, uint8_t *buf, int msf, int session_num);
> 
I'm not sure this is a good idea; the abstraction model seems to be that
scsi-disk.h contains all declarations for the SCSI disk model, and as
such the struct should remain there.

Probably it might be better to create a 'scsi-disk.h' include file, to
contain all necessary things.

But actually I'm not sure.
John? What's your opinion here?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [Qemu-devel] [PATCH RFC 09/11] ide: ide_bridge_do_transfer is treated as PIO transfer

[Hannes Reinecke]

On 08/18/2015 01:42 AM, Alexander Bezzubikov wrote:
> Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
> ---
>  hw/ide/bridge.c | 2 +-
>  hw/ide/bridge.h | 1 +
>  hw/ide/core.c   | 4 +++-
>  3 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/bridge.c b/hw/ide/bridge.c
> index 3a534de..4a5251b 100644
> --- a/hw/ide/bridge.c
> +++ b/hw/ide/bridge.c
> @@ -1,6 +1,6 @@
>  #include "hw/ide/bridge.h"
>  
> -static void ide_bridge_do_transfer(IDEState *s)
> +void ide_bridge_do_transfer(IDEState *s)
>  {
>      SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, s->cur_req);
>  
> diff --git a/hw/ide/bridge.h b/hw/ide/bridge.h
> index dca5d73..59f2f25 100644
> --- a/hw/ide/bridge.h
> +++ b/hw/ide/bridge.h
> @@ -5,5 +5,6 @@
>  
>  void ide_bridge_start_transfer(SCSIRequest *req, uint32_t len);
>  void ide_bridge_complete(SCSIRequest *req, uint32_t status, size_t resid);
> +void ide_bridge_do_transfer(IDEState *s);
>  
>  #endif
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index d017d4e..99b955b 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -34,6 +34,7 @@
>  #include "sysemu/block-backend.h"
>  
>  #include <hw/ide/internal.h>
> +#include "hw/ide/bridge.h"
>  
>  /* These values were based on a Seagate ST3500418AS but have been modified
>     to make more sense in QEMU */
> @@ -2002,7 +2003,8 @@ static bool ide_is_pio_out(IDEState *s)
>      } else if (s->end_transfer_func == ide_sector_read ||
>                 s->end_transfer_func == ide_transfer_stop ||
>                 s->end_transfer_func == ide_atapi_cmd_reply_end ||
> -               s->end_transfer_func == ide_dummy_transfer_stop) {
> +               s->end_transfer_func == ide_dummy_transfer_stop ||
> +               s->end_transfer_func == ide_bridge_do_transfer) {
>          return true;
>      }
>  
> 
Patch is okay, but could benefit from a more detailed description.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [Qemu-devel] [PATCH RFC 10/11] ide: corrected ATAPI checks to be ignored by ATAPI-SCSI bridge

[Hannes Reinecke]

On 08/18/2015 01:42 AM, Alexander Bezzubikov wrote:
> Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
> ---
>  hw/ide/atapi.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 6e73910..896f595 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -1230,7 +1230,8 @@ void ide_atapi_cmd(IDEState *s)
>       * states rely on this behavior.
>       */
>      if (!(atapi_cmd_table[s->io_buffer[0]].flags & ALLOW_UA) &&
> -        !s->tray_open && blk_is_inserted(s->blk) && s->cdrom_changed) {
> +        (s->drive_kind != IDE_BRIDGE && !s->tray_open &&
> +        blk_is_inserted(s->blk) && s->cdrom_changed)) {
>  
>          if (s->cdrom_changed == 1) {
>              ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT);
> @@ -1245,18 +1246,13 @@ void ide_atapi_cmd(IDEState *s)
>  
>      /* Report a Not Ready condition if appropriate for the command */
>      if ((atapi_cmd_table[s->io_buffer[0]].flags & CHECK_READY) &&
> -        (!media_present(s) || !blk_is_inserted(s->blk)))
> +        (s->drive_kind != IDE_BRIDGE &&
> +            (!media_present(s) || !blk_is_inserted(s->blk))))
>      {
>          ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT);
>          return;
>      }
>  
> -    /* Execute the command */
> -    if (atapi_cmd_table[s->io_buffer[0]].handler) {
> -        atapi_cmd_table[s->io_buffer[0]].handler(s, buf);
> -        return;
> -    }
> -
>      if (s->drive_kind == IDE_BRIDGE) {
>          IDEDevice *dev = s->bus->master;
>          SCSIDevice *scsi_dev = scsi_device_find(&dev->scsi_bus, 0, 0, 0);
> @@ -1273,5 +1269,11 @@ void ide_atapi_cmd(IDEState *s)
>          return;
>      }
>  
> +    /* Execute the command */
> +    if (atapi_cmd_table[s->io_buffer[0]].handler) {
> +        atapi_cmd_table[s->io_buffer[0]].handler(s, buf);
> +        return;
> +    }
> +
>      ide_atapi_cmd_error(s, ILLEGAL_REQUEST, ASC_ILLEGAL_OPCODE);
>  }
> 
Please merge it with patch 3.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [Qemu-devel] [PATCH RFC 11/11] ide: bridge functions assigned to SCSIBusInfo, bridge is OK now

[Hannes Reinecke]

On 08/18/2015 01:42 AM, Alexander Bezzubikov wrote:
> Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
> ---
>  hw/ide/qdev.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index 63090cc..773a587 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -150,8 +150,8 @@ static const struct SCSIBusInfo atapi_scsi_info = {
>      .max_target = 0,
>      .max_lun = 0,
>  
> -    .transfer_data = NULL,
> -    .complete = NULL,
> +    .transfer_data = ide_bridge_start_transfer,
> +    .complete = ide_bridge_complete,
>      .cancel = NULL
>  };
>  
> 
And this should be merged with patch 6.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [Qemu-devel] [PATCH RFC 00/11] QEMU ATAPI-SCSI bridge GSoC project

[Hannes Reinecke]

On 08/18/2015 01:42 AM, Alexander Bezzubikov wrote:
> This is my QEMU Google Summer of Code project.
> Here I introduce new device - ATAPI-SCSI bridge.
> Its purpose is to unify IDE ATAPI CD-ROM emulation
> with SCSI CD-ROM emulation to reduce code duplication and squash bugs.
> It's purpose is simple - it just forwards ATAPI commands
> to SCSI side for parsing and execution.
> 
> Example of using:
> qemu-system-x86_64 \
> 	-drive if=none,file=<filename>,id=cdrom \
> 	-drive if=none,id=fake \
> 	-device ide-bridge,id=bridge,drive=fake \
> 	-device scsi-cd,drive=cdrom,bus=bridge.0 \
> 	-hda diskimage \
> 	-m 1024 
> 
> Alexander Bezzubikov (11):
>   ide: ATAPI-SCSI bridge TypeInfo and init function created
>   ide: necessary fields for ATAPI-SCSI bridge added
>   ide: necessary checks corrected to treat ATAPI-SCSI bridge as CDROM
>   scsi: Added function to enable ATAPI-SCSI bridge send SCSI requests
>   ide: bridge 'transfer' and 'complete' functions added
>   ide: ATAPI-SCSI bridge can now forward requests to SCSI
>   ide: Makefile corrected due to bridge creation
>   scsi: SCSIDiskReq declaration moved to header
>   ide: ide_bridge_do_transfer is treated as PIO transfer
>   ide: corrected ATAPI checks to be ignored by ATAPI-SCSI bridge
>   ide: bridge functions assigned to SCSIBusInfo, bridge is OK now
> 
>  hw/ide/Makefile.objs   |   2 +-
>  hw/ide/atapi.c         |  22 +++++++++-
>  hw/ide/bridge.c        | 114 +++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/ide/bridge.h        |  10 +++++
>  hw/ide/core.c          |  28 +++++++-----
>  hw/ide/internal.h      |   6 ++-
>  hw/ide/qdev.c          |  42 +++++++++++++++++-
>  hw/scsi/scsi-disk.c    |  55 ++++++++++++++++++------
>  include/hw/scsi/scsi.h |  16 +++++++
>  9 files changed, 267 insertions(+), 28 deletions(-)
>  create mode 100644 hw/ide/bridge.c
>  create mode 100644 hw/ide/bridge.h
> 
In general a very nice patchset, looking quite closely to what I had
in mind. Congrats.

But I would prefer to reshuffle the patches somewhat; this will make
review easier.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [Qemu-devel] [PATCH RFC 02/11] ide: necessary fields for ATAPI-SCSI bridge added

[Thomas Huth]

On 17/08/15 16:42, Alexander Bezzubikov wrote:
> Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
> ---
>  hw/ide/internal.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index f2999ce..3ebc63c 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -429,6 +429,8 @@ struct IDEState {
>      uint8_t *smart_selftest_data;
>      /* AHCI */
>      int ncq_queues;
> +    /* ATAPI-SCSI bridge */
> +    SCSIRequest *cur_req;

Considering that you're extending struct *IDEState* here, I think the
name "cur_req" might be a little bit misleading? Maybe rather name it
"scsi_req" or "atapi_req"?

 Thomas

Re: [Qemu-devel] [PATCH RFC 02/11] ide: necessary fields for ATAPI-SCSI bridge added

[Hannes Reinecke]

On 08/19/2015 01:50 AM, Thomas Huth wrote:
> On 17/08/15 16:42, Alexander Bezzubikov wrote:
>> Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
>> ---
>>  hw/ide/internal.h | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
>> index f2999ce..3ebc63c 100644
>> --- a/hw/ide/internal.h
>> +++ b/hw/ide/internal.h
>> @@ -429,6 +429,8 @@ struct IDEState {
>>      uint8_t *smart_selftest_data;
>>      /* AHCI */
>>      int ncq_queues;
>> +    /* ATAPI-SCSI bridge */
>> +    SCSIRequest *cur_req;
> 
> Considering that you're extending struct *IDEState* here, I think the
> name "cur_req" might be a little bit misleading? Maybe rather name it
> "scsi_req" or "atapi_req"?
> 
Good point. 'scsi_req' is a far better name.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [Qemu-devel] [PATCH RFC 08/11] scsi: SCSIDiskReq declaration moved to header

[John Snow]

On 08/18/2015 02:17 AM, Hannes Reinecke wrote:
> On 08/18/2015 01:42 AM, Alexander Bezzubikov wrote:
>> Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
>> ---
>>  hw/scsi/scsi-disk.c    | 12 ------------
>>  include/hw/scsi/scsi.h | 13 +++++++++++++
>>  2 files changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>> index f67d816..9d5f0a4 100644
>> --- a/hw/scsi/scsi-disk.c
>> +++ b/hw/scsi/scsi-disk.c
>> @@ -53,18 +53,6 @@ do { printf("scsi-disk: " fmt , ## __VA_ARGS__); } while (0)
>>  
>>  typedef struct SCSIDiskState SCSIDiskState;
>>  
>> -typedef struct SCSIDiskReq {
>> -    SCSIRequest req;
>> -    /* Both sector and sector_count are in terms of qemu 512 byte blocks.  */
>> -    uint64_t sector;
>> -    uint32_t sector_count;
>> -    uint32_t buflen;
>> -    bool started;
>> -    struct iovec iov;
>> -    QEMUIOVector qiov;
>> -    BlockAcctCookie acct;
>> -} SCSIDiskReq;
>> -
>>  #define SCSI_DISK_F_REMOVABLE             0
>>  #define SCSI_DISK_F_DPOFUA                1
>>  #define SCSI_DISK_F_NO_REMOVABLE_DEVOPS   2
>> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
>> index 881ed23..de0546e 100644
>> --- a/include/hw/scsi/scsi.h
>> +++ b/include/hw/scsi/scsi.h
>> @@ -6,6 +6,7 @@
>>  #include "hw/block/block.h"
>>  #include "sysemu/sysemu.h"
>>  #include "qemu/notify.h"
>> +#include "block/accounting.h"
>>  
>>  #define MAX_SCSI_DEVS	255
>>  
>> @@ -120,6 +121,18 @@ extern const VMStateDescription vmstate_scsi_device;
>>      .offset     = vmstate_offset_value(_state, _field, SCSIDevice),  \
>>  }
>>  
>> +typedef struct SCSIDiskReq {
>> +    SCSIRequest req;
>> +    /* Both sector and sector_count are in terms of qemu 512 byte blocks.  */
>> +    uint64_t sector;
>> +    uint32_t sector_count;
>> +    uint32_t buflen;
>> +    bool started;
>> +    struct iovec iov;
>> +    QEMUIOVector qiov;
>> +    BlockAcctCookie acct;
>> +} SCSIDiskReq;
>> +
>>  /* cdrom.c */
>>  int cdrom_read_toc(int nb_sectors, uint8_t *buf, int msf, int start_track);
>>  int cdrom_read_toc_raw(int nb_sectors, uint8_t *buf, int msf, int session_num);
>>
> I'm not sure this is a good idea; the abstraction model seems to be that
> scsi-disk.h contains all declarations for the SCSI disk model, and as
> such the struct should remain there.
> 
> Probably it might be better to create a 'scsi-disk.h' include file, to
> contain all necessary things.
> 
> But actually I'm not sure.
> John? What's your opinion here?
> 
> Cheers,
> 
> Hannes
> 

I know I'm working from outdated copies here, but my guess would be to
attempt to limit exposure of this struct. Putting it in scsi.h probably
exposes it to more files than we anticipate.

I'll see what happens in v2 through v4 :)

--js