Hi Michael,
"Michael S. Tsirkin" <m...@redhat.com> writes:
> Drop duplicated code.
>
> Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
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
