From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60122) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVfMV-0007AF-K0 for qemu-devel@nongnu.org; Wed, 11 Mar 2015 08:03:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YVfMP-0003yc-Ow for qemu-devel@nongnu.org; Wed, 11 Mar 2015 08:03:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55784) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVfMP-0003yM-Fu for qemu-devel@nongnu.org; Wed, 11 Mar 2015 08:03:37 -0400 Date: Wed, 11 Mar 2015 13:03:21 +0100 From: "Michael S. Tsirkin" Message-ID: <20150311130254-mutt-send-email-mst@redhat.com> References: <1424122283-12521-1-git-send-email-mst@redhat.com> <1424122283-12521-11-git-send-email-mst@redhat.com> <87pp8fdcsg.fsf@abhimanyu.in.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87pp8fdcsg.fsf@abhimanyu.in.ibm.com> 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: Nikunj A Dadhania Cc: Peter Maydell , Thomas Huth , Alexander Graf , qemu-devel@nongnu.org, Stefan Hajnoczi , "Chen, Tiejun" , Cornelia Huck , Paolo Bonzini , Anthony Liguori On Wed, Mar 11, 2015 at 04:24:55PM +0530, Nikunj A Dadhania wrote: > > Hi Michael, > > "Michael S. Tsirkin" writes: > > Drop duplicated code. > > > > Signed-off-by: Michael S. Tsirkin > > This patch is breaking SLOF. Reason below: Can you please try out this patch: mid.gmane.org/20150311101858-mutt-send-email-mst@redhat.com Thanks! > > --- > > 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