Re: [Qemu-devel] [PATCH 5/6] scsi-disk: Remove 'drive_kind'

[Hannes Reinecke <hare@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 4020 bytes --]

On 07/26/2011 08:38 AM, Markus Armbruster wrote:
> Hannes Reinecke<hare@suse.de>  writes:
>
>> On 07/25/2011 05:59 PM, Markus Armbruster wrote:
>>> Hannes Reinecke<hare@suse.de>   writes:
>>>
>>>> Instead of using its own definitions scsi-disk should
>>>> be using the device type of the parent device.
>>>>
>>>> Signed-off-by: Hannes Reinecke<hare@suse.de>
>>>> ---
>>>>    hw/scsi-defs.h |    6 +++++-
>>>>    hw/scsi-disk.c |   48 ++++++++++++++++++++++++------------------------
>>>>    2 files changed, 29 insertions(+), 25 deletions(-)
>>>>
>> [ .. ]
>>>> @@ -1217,44 +1214,47 @@ static int scsi_initfn(SCSIDevice *dev, SCSIDriveKind kind)
>>>>            return -1;
>>>>        }
>>>>
>>>> -    if (kind == SCSI_CD) {
>>>> +    if (scsi_type == TYPE_ROM) {
>>>>            s->qdev.blocksize = 2048;
>>>> -    } else {
>>>> +    } else if (scsi_type == TYPE_DISK) {
>>>>            s->qdev.blocksize = s->qdev.conf.logical_block_size;
>>>> +    } else {
>>>> +        error_report("scsi-disk: Unhandled SCSI type %02x", scsi_type);
>>>> +        return -1;
>>>>        }
>>>>        s->cluster_size = s->qdev.blocksize / 512;
>>>>        s->bs->buffer_alignment = s->qdev.blocksize;
>>>>
>>>> -    s->qdev.type = TYPE_DISK;
>>>> +    s->qdev.type = scsi_type;
>>>
>>> Is this a bug fix?
>>>
>> No, proper initialisation.
>> s->qdev.type is the SCSI type we're told to emulate. So we have to set
>> it to the correct value otherwise the emulation will return wrong
>> values.
>
> Well, it changes .type from TYPE_DISK to TYPE_ROM for scsi-cd.  I
> understand how that is required for your patch to work.  I wonder
> whether it has an impact beyond that, given that the old value is
> plainly wrong.
>
>>>>        qemu_add_vm_change_state_handler(scsi_dma_restart_cb, s);
>>>> -    bdrv_set_removable(s->bs, kind == SCSI_CD);
>>>> +    bdrv_set_removable(s->bs, scsi_type == TYPE_ROM);
>>>>        add_boot_device_path(s->qdev.conf.bootindex,&dev->qdev, ",0");
>>>>        return 0;
>>>>    }
>>>>
>>>>    static int scsi_hd_initfn(SCSIDevice *dev)
>>>>    {
>>>> -    return scsi_initfn(dev, SCSI_HD);
>>>> +    return scsi_initfn(dev, TYPE_DISK);
>>>>    }
>>>>
>>>>    static int scsi_cd_initfn(SCSIDevice *dev)
>>>>    {
>>>> -    return scsi_initfn(dev, SCSI_CD);
>>>> +    return scsi_initfn(dev, TYPE_ROM);
>>>>    }
>>>>
>>>>    static int scsi_disk_initfn(SCSIDevice *dev)
>>>>    {
>>>> -    SCSIDriveKind kind;
>>>>        DriveInfo *dinfo;
>>>> +    uint8_t scsi_type = TYPE_DISK;
>>>>
>>>> -    if (!dev->conf.bs) {
>>>> -        kind = SCSI_HD;         /* will die in scsi_initfn() */
>>>
>>> The comment explains why we don't explicitly fail when !dev->conf.bs,
>>> like all the other block device models.  I'd rather keep it.
>>>
>> Ah. The magic of block devices. By all means, keep it.
>
> Like this?
>
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index f42a5d1..0925944 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -1251,17 +1251,17 @@ static int scsi_cd_initfn(SCSIDevice *dev)
>
>   static int scsi_disk_initfn(SCSIDevice *dev)
>   {
> -    SCSIDriveKind kind;
> +    uint8_t scsi_type;
>       DriveInfo *dinfo;
>
>       if (!dev->conf.bs) {
> -        kind = SCSI_HD;         /* will die in scsi_initfn() */
> +        scsi_type = TYPE_DISK;  /* will die in scsi_initfn() */
>       } else {
>           dinfo = drive_get_by_blockdev(dev->conf.bs);
> -        kind = dinfo->media_cd ? SCSI_CD : SCSI_HD;
> +        scsi_type = dinfo->media_cd ? TYPE_ROM : TYPE_DISK;
>       }
>
> -    return scsi_initfn(dev, kind);
> +    return scsi_initfn(dev, scsi_type);
>   }
>
>   #define DEFINE_SCSI_DISK_PROPERTIES()                           \
>
> By the way, I'm afraid you forgot to remove typedef SCSIDriveKind.  Care
> to respin this one?
Here you go.

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)

[-- Attachment #2: scsi-disk-Remove-drive_kind.patch --]
[-- Type: text/x-patch, Size: 6660 bytes --]

>From f6e40a484dbcfdd4f8434ae3427c75a56581d659 Mon Sep 17 00:00:00 2001
From: Hannes Reinecke <hare@suse.de>
Date: Fri, 22 Jul 2011 16:44:46 +0200
Subject: [PATCH] scsi-disk: Remove 'drive_kind'

Instead of using its own definitions scsi-disk should
be using the device type of the parent device.

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

diff --git a/hw/scsi-defs.h b/hw/scsi-defs.h
index f644860..27010b7 100644
--- a/hw/scsi-defs.h
+++ b/hw/scsi-defs.h
@@ -164,6 +164,7 @@
 
 #define TYPE_DISK           0x00
 #define TYPE_TAPE           0x01
+#define TYPE_PRINTER        0x02
 #define TYPE_PROCESSOR      0x03    /* HP scanners use this */
 #define TYPE_WORM           0x04    /* Treated as ROM by our system */
 #define TYPE_ROM            0x05
@@ -171,6 +172,9 @@
 #define TYPE_MOD            0x07    /* Magneto-optical disk -
 				     * - treated as TYPE_DISK */
 #define TYPE_MEDIUM_CHANGER 0x08
-#define TYPE_ENCLOSURE	    0x0d    /* Enclosure Services Device */
+#define TYPE_STORAGE_ARRAY  0x0c    /* Storage array device */
+#define TYPE_ENCLOSURE      0x0d    /* Enclosure Services Device */
+#define TYPE_RBC            0x0e    /* Simplified Direct-Access Device */
+#define TYPE_OSD            0x11    /* Object-storage Device */
 #define TYPE_NO_LUN         0x7f
 
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 535fa11..f2511d0 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -59,8 +59,6 @@ typedef struct SCSIDiskReq {
     uint32_t status;
 } SCSIDiskReq;
 
-typedef enum { SCSI_HD, SCSI_CD } SCSIDriveKind;
-
 struct SCSIDiskState
 {
     SCSIDevice qdev;
@@ -74,7 +72,6 @@ struct SCSIDiskState
     char *version;
     char *serial;
     SCSISense sense;
-    SCSIDriveKind drive_kind;
 };
 
 static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type);
@@ -382,7 +379,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             return -1;
         }
 
-        if (s->drive_kind == SCSI_CD) {
+        if (s->qdev.type == TYPE_ROM) {
             outbuf[buflen++] = 5;
         } else {
             outbuf[buflen++] = 0;
@@ -401,7 +398,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             if (s->serial)
                 outbuf[buflen++] = 0x80; // unit serial number
             outbuf[buflen++] = 0x83; // device identification
-            if (s->drive_kind == SCSI_HD) {
+            if (s->qdev.type == TYPE_DISK) {
                 outbuf[buflen++] = 0xb0; // block limits
                 outbuf[buflen++] = 0xb2; // thin provisioning
             }
@@ -460,7 +457,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             unsigned int opt_io_size =
                     s->qdev.conf.opt_io_size / s->qdev.blocksize;
 
-            if (s->drive_kind == SCSI_CD) {
+            if (s->qdev.type == TYPE_ROM) {
                 DPRINTF("Inquiry (EVPD[%02X] not supported for CDROM\n",
                         page_code);
                 return -1;
@@ -530,12 +527,11 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
         return buflen;
     }
 
-    if (s->drive_kind == SCSI_CD) {
-        outbuf[0] = 5;
+    outbuf[0] = s->qdev.type & 0x1f;
+    if (s->qdev.type == TYPE_ROM) {
         outbuf[1] = 0x80;
         memcpy(&outbuf[16], "QEMU CD-ROM     ", 16);
     } else {
-        outbuf[0] = 0;
         outbuf[1] = s->removable ? 0x80 : 0;
         memcpy(&outbuf[16], "QEMU HARDDISK   ", 16);
     }
@@ -661,7 +657,7 @@ static int mode_sense_page(SCSIRequest *req, int page, uint8_t *p,
         return p[1] + 2;
 
     case 0x2a: /* CD Capabilities and Mechanical Status page. */
-        if (s->drive_kind != SCSI_CD)
+        if (s->qdev.type != TYPE_ROM)
             return 0;
         p[0] = 0x2a;
         p[1] = 0x14;
@@ -877,7 +873,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
             goto illegal_request;
         break;
     case START_STOP:
-        if (s->drive_kind == SCSI_CD && (req->cmd.buf[4] & 2)) {
+        if (s->qdev.type == TYPE_ROM && (req->cmd.buf[4] & 2)) {
             /* load/eject medium */
             bdrv_eject(s->bs, !(req->cmd.buf[4] & 1));
         }
@@ -1183,7 +1179,7 @@ static void scsi_destroy(SCSIDevice *dev)
     blockdev_mark_auto_del(s->qdev.conf.bs);
 }
 
-static int scsi_initfn(SCSIDevice *dev, SCSIDriveKind kind)
+static int scsi_initfn(SCSIDevice *dev, uint8_t scsi_type)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
     DriveInfo *dinfo;
@@ -1193,9 +1189,8 @@ static int scsi_initfn(SCSIDevice *dev, SCSIDriveKind kind)
         return -1;
     }
     s->bs = s->qdev.conf.bs;
-    s->drive_kind = kind;
 
-    if (kind == SCSI_HD && !bdrv_is_inserted(s->bs)) {
+    if (scsi_type == TYPE_DISK && !bdrv_is_inserted(s->bs)) {
         error_report("Device needs media, but drive is empty");
         return -1;
     }
@@ -1217,44 +1212,47 @@ static int scsi_initfn(SCSIDevice *dev, SCSIDriveKind kind)
         return -1;
     }
 
-    if (kind == SCSI_CD) {
+    if (scsi_type == TYPE_ROM) {
         s->qdev.blocksize = 2048;
-    } else {
+    } else if (scsi_type == TYPE_DISK) {
         s->qdev.blocksize = s->qdev.conf.logical_block_size;
+    } else {
+        error_report("scsi-disk: Unhandled SCSI type %02x", scsi_type);
+        return -1;
     }
     s->cluster_size = s->qdev.blocksize / 512;
     s->bs->buffer_alignment = s->qdev.blocksize;
 
-    s->qdev.type = TYPE_DISK;
+    s->qdev.type = scsi_type;
     qemu_add_vm_change_state_handler(scsi_dma_restart_cb, s);
-    bdrv_set_removable(s->bs, kind == SCSI_CD);
+    bdrv_set_removable(s->bs, scsi_type == TYPE_ROM);
     add_boot_device_path(s->qdev.conf.bootindex, &dev->qdev, ",0");
     return 0;
 }
 
 static int scsi_hd_initfn(SCSIDevice *dev)
 {
-    return scsi_initfn(dev, SCSI_HD);
+    return scsi_initfn(dev, TYPE_DISK);
 }
 
 static int scsi_cd_initfn(SCSIDevice *dev)
 {
-    return scsi_initfn(dev, SCSI_CD);
+    return scsi_initfn(dev, TYPE_ROM);
 }
 
 static int scsi_disk_initfn(SCSIDevice *dev)
 {
-    SCSIDriveKind kind;
     DriveInfo *dinfo;
+    uint8_t scsi_type;
 
     if (!dev->conf.bs) {
-        kind = SCSI_HD;         /* will die in scsi_initfn() */
+        scsi_type = TYPE_DISK;  /* will die in scsi_initfn() */
     } else {
         dinfo = drive_get_by_blockdev(dev->conf.bs);
-        kind = dinfo->media_cd ? SCSI_CD : SCSI_HD;
+        scsi_type = dinfo->media_cd ? TYPE_ROM : TYPE_DISK;
     }
 
-    return scsi_initfn(dev, kind);
+    return scsi_initfn(dev, scsi_type);
 }
 
 #define DEFINE_SCSI_DISK_PROPERTIES()                           \