Re: [Qemu-devel] [PATCH v3 10/17] virtio-scsi: use standard-headers

[Michael S. Tsirkin]

On Wed, Mar 11, 2015 at 04:24:55PM +0530, Nikunj A Dadhania wrote:
> 
> Hi Michael,
> 
> "Michael S. Tsirkin" <address@hidden> writes:
> > Drop duplicated code.
> >
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> 
> This patch is breaking SLOF. Reason below:

Can you please try out this patch:
mid.gmane.org/address@hidden

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