From patchwork Tue Jul 26 06:46:29 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hannes Reinecke X-Patchwork-Id: 1007292 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by demeter1.kernel.org (8.14.4/8.14.4) with ESMTP id p6Q6kY2m023310 for ; Tue, 26 Jul 2011 06:46:34 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751371Ab1GZGqb (ORCPT ); Tue, 26 Jul 2011 02:46:31 -0400 Received: from cantor2.suse.de ([195.135.220.15]:38550 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751000Ab1GZGqb (ORCPT ); Tue, 26 Jul 2011 02:46:31 -0400 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.221.2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx2.suse.de (Postfix) with ESMTP id AC7E68F9AB; Tue, 26 Jul 2011 08:46:29 +0200 (CEST) Message-ID: <4E2E62C5.5080408@suse.de> Date: Tue, 26 Jul 2011 08:46:29 +0200 From: Hannes Reinecke User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.14) Gecko/20110221 SUSE/3.1.8 Thunderbird/3.1.8 MIME-Version: 1.0 To: Markus Armbruster Cc: Kevin Wolf , qemu-devel@nongnu.org, kvm@vger.kernel.org, Alexander Graf Subject: Re: [Qemu-devel] [PATCH 5/6] scsi-disk: Remove 'drive_kind' References: <1311346277-32329-1-git-send-email-hare@suse.de> <1311346277-32329-6-git-send-email-hare@suse.de> <4E2E5CD2.3090102@suse.de> In-Reply-To: Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter1.kernel.org [140.211.167.41]); Tue, 26 Jul 2011 06:46:34 +0000 (UTC) On 07/26/2011 08:38 AM, Markus Armbruster wrote: > Hannes Reinecke writes: > >> On 07/25/2011 05:59 PM, Markus Armbruster wrote: >>> Hannes Reinecke writes: >>> >>>> Instead of using its own definitions scsi-disk should >>>> be using the device type of the parent device. >>>> >>>> Signed-off-by: Hannes Reinecke >>>> --- >>>> 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 From f6e40a484dbcfdd4f8434ae3427c75a56581d659 Mon Sep 17 00:00:00 2001 From: Hannes Reinecke 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 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() \