From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41872) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVeJ6-0005va-AT for qemu-devel@nongnu.org; Wed, 11 Mar 2015 06:56:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YVeJ2-0006Fk-Rb for qemu-devel@nongnu.org; Wed, 11 Mar 2015 06:56:08 -0400 Received: from e23smtp04.au.ibm.com ([202.81.31.146]:37948) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVeJ2-0006F5-77 for qemu-devel@nongnu.org; Wed, 11 Mar 2015 06:56:04 -0400 Received: from /spool/local by e23smtp04.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 11 Mar 2015 20:55:59 +1000 Received: from d23relay06.au.ibm.com (d23relay06.au.ibm.com [9.185.63.219]) by d23dlp01.au.ibm.com (Postfix) with ESMTP id BA0FE2CE8050 for ; Wed, 11 Mar 2015 21:55:56 +1100 (EST) Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay06.au.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t2BAtmwn41353462 for ; Wed, 11 Mar 2015 21:55:56 +1100 Received: from d23av03.au.ibm.com (localhost [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t2BAtMl8001948 for ; Wed, 11 Mar 2015 21:55:22 +1100 From: Nikunj A Dadhania In-Reply-To: <1424122283-12521-11-git-send-email-mst@redhat.com> References: <1424122283-12521-1-git-send-email-mst@redhat.com> <1424122283-12521-11-git-send-email-mst@redhat.com> Date: Wed, 11 Mar 2015 16:24:55 +0530 Message-ID: <87pp8fdcsg.fsf@abhimanyu.in.ibm.com> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v3 10/17] virtio-scsi: use standard-headers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" , qemu-devel@nongnu.org Cc: Peter Maydell , Thomas Huth , Alexander Graf , Stefan Hajnoczi , "Chen, Tiejun" , Cornelia Huck , Paolo Bonzini , Anthony Liguori Hi Michael, "Michael S. Tsirkin" writes: > Drop duplicated code. > > Signed-off-by: Michael S. Tsirkin This patch is breaking SLOF. Reason below: > --- > include/hw/virtio/virtio-scsi.h | 120 +++------------------------------------- > hw/scsi/virtio-scsi.c | 1 + > 2 files changed, 10 insertions(+), 111 deletions(-) > > diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h > index bf17cc9..864070d 100644 > --- a/include/hw/virtio/virtio-scsi.h > +++ b/include/hw/virtio/virtio-scsi.h For instance: > - > -/* SCSI command request, followed by CDB and data-out */ > -typedef struct { > - uint8_t lun[8]; /* Logical Unit Number */ > - uint64_t tag; /* Command identifier */ > - uint8_t task_attr; /* Task attribute */ > - uint8_t prio; > - uint8_t crn; > -} QEMU_PACKED VirtIOSCSICmdReq; in include/standard-headers/linux/virtio_scsi.h #define VIRTIO_SCSI_CDB_SIZE 32 #define VIRTIO_SCSI_SENSE_SIZE 96 /* SCSI command request, followed by data-out */ struct virtio_scsi_cmd_req { uint8_t lun[8]; /* Logical Unit Number */ __virtio64 tag; /* Command identifier */ uint8_t task_attr; /* Task attribute */ uint8_t prio; /* SAM command priority field */ uint8_t crn; uint8_t cdb[VIRTIO_SCSI_CDB_SIZE]; } QEMU_PACKED; Here the structure is changed having cdb as extra member. Moreover, we have checks like below in hw/scsi/virtio-scsi.c rc = virtio_scsi_parse_req(req, sizeof(VirtIOSCSICmdReq) + vs->cdb_size, sizeof(VirtIOSCSICmdResp) + vs->sense_size); Now that the size of CmdReq structure is not the same as earlier. Similarly for CmdResp structure. SLOF has these headers replicated, IMHO, we should be changing the structure size. > - > -/* Response, followed by sense data and data-in */ > -typedef struct { > - uint32_t sense_len; /* Sense data length */ > - uint32_t resid; /* Residual bytes in data buffer */ > - uint16_t status_qualifier; /* Status qualifier */ > - uint8_t status; /* Command completion status */ > - uint8_t response; /* Response values */ > -} QEMU_PACKED VirtIOSCSICmdResp; > - > -/* Task Management Request */ > -typedef struct { > - uint32_t type; > - uint32_t subtype; > - uint8_t lun[8]; > - uint64_t tag; > -} QEMU_PACKED VirtIOSCSICtrlTMFReq; > - > -typedef struct { > - uint8_t response; > -} QEMU_PACKED VirtIOSCSICtrlTMFResp; > - > -/* Asynchronous notification query/subscription */ > -typedef struct { > - uint32_t type; > - uint8_t lun[8]; > - uint32_t event_requested; > -} QEMU_PACKED VirtIOSCSICtrlANReq; > - > -typedef struct { > - uint32_t event_actual; > - uint8_t response; > -} QEMU_PACKED VirtIOSCSICtrlANResp; > - > -typedef struct { > - uint32_t event; > - uint8_t lun[8]; > - uint32_t reason; > -} QEMU_PACKED VirtIOSCSIEvent; > - > -typedef struct { > - uint32_t num_queues; > - uint32_t seg_max; > - uint32_t max_sectors; > - uint32_t cmd_per_lun; > - uint32_t event_info_size; > - uint32_t sense_size; > - uint32_t cdb_size; > - uint16_t max_channel; > - uint16_t max_target; > - uint32_t max_lun; > -} QEMU_PACKED VirtIOSCSIConfig; > +typedef struct virtio_scsi_cmd_req VirtIOSCSICmdReq; > +typedef struct virtio_scsi_cmd_resp VirtIOSCSICmdResp; > +typedef struct virtio_scsi_ctrl_tmf_req VirtIOSCSICtrlTMFReq; > +typedef struct virtio_scsi_ctrl_tmf_resp VirtIOSCSICtrlTMFResp; > +typedef struct virtio_scsi_ctrl_an_req VirtIOSCSICtrlANReq; > +typedef struct virtio_scsi_ctrl_an_resp VirtIOSCSICtrlANResp; > +typedef struct virtio_scsi_event VirtIOSCSIEvent; > +typedef struct virtio_scsi_config VirtIOSCSIConfig; > > struct VirtIOSCSIConf { > uint32_t num_queues; > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c > index 9e2c718..d18654e 100644 > --- a/hw/scsi/virtio-scsi.c > +++ b/hw/scsi/virtio-scsi.c > @@ -13,6 +13,7 @@ > * > */ > > +#include "standard-headers/linux/virtio_ids.h" > #include "hw/virtio/virtio-scsi.h" > #include "qemu/error-report.h" > #include "qemu/iov.h" > -- > MST Thanks Nikunj