* [PATCH 0/7] scsi: scsi-disk corrupts data
@ 2020-11-16 18:40 Hannes Reinecke
2020-11-16 18:40 ` [PATCH 1/7] scsi-disk: Add sg_io callback to evaluate status Hannes Reinecke
` (7 more replies)
0 siblings, 8 replies; 20+ messages in thread
From: Hannes Reinecke @ 2020-11-16 18:40 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Hannes Reinecke
Hi all,
a customer of ours reported repeated data corruption in the guest following a command abort.
After lengthy debugging we found that scsi-disk (and scsi-generic, for that matter) ignores
the host_status field from SG_IO once a command is aborted. If the command is aborted, SG_IO
will return with a SCSI status 'GOOD', and host_status 'DID_TIME_OUT'. scsi-disk will now
ignore the DID_TIME_OUT setting, and just report the SCSI status back to the guest.
The guest will then assume everything is okay and not retry the command, leading to the data
corruption.
This patchset moves the (linux only) SG_ERR host_status codes to generic code as SCSI_HOST
values, and adds a host_status field to SCSIRequest. With that some drivers like virtio_scsi
can interpret the host_status code and map it onto it driver-specific status.
This status is then visible to the guest, which then is able to take appropriate action.
As usual, comments and reviews are welcome.
Hannes Reinecke (6):
scsi-disk: Add sg_io callback to evaluate status
scsi: drop 'result' argument from command_complete callback
scsi: Rename linux-specific SG_ERR codes to generic SCSI_HOST error
codes
scsi: Add mapping for generic SCSI_HOST status to sense codes
scsi: split sg_io_sense_from_errno() in two functions
scsi: move host_status handling into SCSI drivers
Paolo Bonzini (1):
scsi-disk: convert more errno values back to SCSI statuses
hw/scsi/esp-pci.c | 5 +--
hw/scsi/esp.c | 17 +++++--
hw/scsi/lsi53c895a.c | 17 +++++--
hw/scsi/megasas.c | 15 +++++--
hw/scsi/mptsas.c | 14 +++++-
hw/scsi/scsi-bus.c | 2 +-
hw/scsi/scsi-disk.c | 75 ++++++++++++++++++++-----------
hw/scsi/scsi-generic.c | 21 ++++++---
hw/scsi/spapr_vscsi.c | 20 ++++++---
hw/scsi/virtio-scsi.c | 44 ++++++++++++++++--
hw/scsi/vmw_pvscsi.c | 29 +++++++++++-
hw/usb/dev-storage.c | 6 +--
hw/usb/dev-uas.c | 7 ++-
include/hw/scsi/esp.h | 2 +-
include/hw/scsi/scsi.h | 5 ++-
include/scsi/utils.h | 29 +++++++-----
scsi/qemu-pr-helper.c | 14 ++++--
scsi/utils.c | 119 ++++++++++++++++++++++++++++++++++++-------------
18 files changed, 328 insertions(+), 113 deletions(-)
--
2.16.4
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/7] scsi-disk: Add sg_io callback to evaluate status
2020-11-16 18:40 [PATCH 0/7] scsi: scsi-disk corrupts data Hannes Reinecke
@ 2020-11-16 18:40 ` Hannes Reinecke
2020-11-16 18:40 ` [PATCH 2/7] scsi: drop 'result' argument from command_complete callback Hannes Reinecke
` (6 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Hannes Reinecke @ 2020-11-16 18:40 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Hannes Reinecke
Add a separate sg_io callback to allow us to evaluate the various
states returned by the SG_IO ioctl.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
hw/scsi/scsi-disk.c | 28 ++++++++++++++++++++++------
1 file changed, 22 insertions(+), 6 deletions(-)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index dd23a38d6a..5d6c892f29 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -76,7 +76,6 @@ typedef struct SCSIDiskReq {
struct iovec iov;
QEMUIOVector qiov;
BlockAcctCookie acct;
- unsigned char *status;
} SCSIDiskReq;
#define SCSI_DISK_F_REMOVABLE 0
@@ -188,7 +187,7 @@ static bool scsi_disk_req_check_error(SCSIDiskReq *r, int ret, bool acct_failed)
return true;
}
- if (ret < 0 || (r->status && *r->status)) {
+ if (ret < 0 || r->req.status) {
return scsi_handle_rw_error(r, -ret, acct_failed);
}
@@ -452,11 +451,11 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
* whether the error has to be handled by the guest or should rather
* pause the host.
*/
- assert(r->status && *r->status);
+ assert(r->req.status);
if (scsi_sense_buf_is_guest_recoverable(r->req.sense, sizeof(r->req.sense))) {
/* These errors are handled by guest. */
sdc->update_sense(&r->req);
- scsi_req_complete(&r->req, *r->status);
+ scsi_req_complete(&r->req, r->req.status);
return true;
}
error = scsi_sense_buf_to_errno(r->req.sense, sizeof(r->req.sense));
@@ -2688,8 +2687,24 @@ typedef struct SCSIBlockReq {
/* CDB passed to SG_IO. */
uint8_t cdb[16];
+ BlockCompletionFunc *cb;
+ void *cb_opaque;
} SCSIBlockReq;
+static void scsi_block_sgio_complete(void *opaque, int ret)
+{
+ SCSIBlockReq *req = (SCSIBlockReq *)opaque;
+ SCSIDiskReq *r = &req->req;
+ SCSISense sense;
+
+ r->req.status = sg_io_sense_from_errno(-ret, &req->io_header, &sense);
+ if (r->req.status == CHECK_CONDITION &&
+ req->io_header.status != CHECK_CONDITION)
+ scsi_req_build_sense(&r->req, sense);
+
+ req->cb(req->cb_opaque, ret);
+}
+
static BlockAIOCB *scsi_block_do_sgio(SCSIBlockReq *req,
int64_t offset, QEMUIOVector *iov,
int direction,
@@ -2768,9 +2783,11 @@ static BlockAIOCB *scsi_block_do_sgio(SCSIBlockReq *req,
io_header->timeout = s->qdev.io_timeout * 1000;
io_header->usr_ptr = r;
io_header->flags |= SG_FLAG_DIRECT_IO;
+ req->cb = cb;
+ req->cb_opaque = opaque;
trace_scsi_disk_aio_sgio_command(r->req.tag, req->cdb[0], lba,
nb_logical_blocks, io_header->timeout);
- aiocb = blk_aio_ioctl(s->qdev.conf.blk, SG_IO, io_header, cb, opaque);
+ aiocb = blk_aio_ioctl(s->qdev.conf.blk, SG_IO, io_header, scsi_block_sgio_complete, req);
assert(aiocb != NULL);
return aiocb;
}
@@ -2884,7 +2901,6 @@ static int32_t scsi_block_dma_command(SCSIRequest *req, uint8_t *buf)
return 0;
}
- r->req.status = &r->io_header.status;
return scsi_disk_dma_command(req, buf);
}
--
2.16.4
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/7] scsi: drop 'result' argument from command_complete callback
2020-11-16 18:40 [PATCH 0/7] scsi: scsi-disk corrupts data Hannes Reinecke
2020-11-16 18:40 ` [PATCH 1/7] scsi-disk: Add sg_io callback to evaluate status Hannes Reinecke
@ 2020-11-16 18:40 ` Hannes Reinecke
2020-11-16 18:40 ` [PATCH 3/7] scsi-disk: convert more errno values back to SCSI statuses Hannes Reinecke
` (5 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Hannes Reinecke @ 2020-11-16 18:40 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Hannes Reinecke
The command complete callback has a SCSIRequest as the first argument,
and the status field of that structure is identical to the 'status'
argument. So drop the argument from the callback.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
hw/scsi/esp-pci.c | 5 ++---
hw/scsi/esp.c | 7 +++----
hw/scsi/lsi53c895a.c | 6 +++---
hw/scsi/megasas.c | 6 ++----
hw/scsi/mptsas.c | 5 +++--
hw/scsi/scsi-bus.c | 2 +-
hw/scsi/spapr_vscsi.c | 10 +++++-----
hw/scsi/virtio-scsi.c | 5 ++---
hw/scsi/vmw_pvscsi.c | 4 ++--
hw/usb/dev-storage.c | 6 +++---
hw/usb/dev-uas.c | 7 +++----
include/hw/scsi/esp.h | 2 +-
include/hw/scsi/scsi.h | 2 +-
13 files changed, 31 insertions(+), 36 deletions(-)
diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
index 2ce96dc56e..4d7c2cab56 100644
--- a/hw/scsi/esp-pci.c
+++ b/hw/scsi/esp-pci.c
@@ -329,13 +329,12 @@ static const VMStateDescription vmstate_esp_pci_scsi = {
}
};
-static void esp_pci_command_complete(SCSIRequest *req, uint32_t status,
- size_t resid)
+static void esp_pci_command_complete(SCSIRequest *req, size_t resid)
{
ESPState *s = req->hba_private;
PCIESPState *pci = container_of(s, PCIESPState, esp);
- esp_command_complete(req, status, resid);
+ esp_command_complete(req, resid);
pci->dma_regs[DMA_WBC] = 0;
pci->dma_regs[DMA_STAT] |= DMA_STAT_DONE;
}
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index b84e0fe33e..93d9c9c7b9 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -485,8 +485,7 @@ static void esp_report_command_complete(ESPState *s, uint32_t status)
}
}
-void esp_command_complete(SCSIRequest *req, uint32_t status,
- size_t resid)
+void esp_command_complete(SCSIRequest *req, size_t resid)
{
ESPState *s = req->hba_private;
@@ -495,11 +494,11 @@ void esp_command_complete(SCSIRequest *req, uint32_t status,
* interrupt has been handled.
*/
trace_esp_command_complete_deferred();
- s->deferred_status = status;
+ s->deferred_status = req->status;
s->deferred_complete = true;
return;
}
- esp_report_command_complete(s, status);
+ esp_report_command_complete(s, req->status);
}
void esp_transfer_data(SCSIRequest *req, uint32_t len)
diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 7d13c7dc1c..a4e58580e4 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -787,14 +787,14 @@ static int lsi_queue_req(LSIState *s, SCSIRequest *req, uint32_t len)
}
/* Callback to indicate that the SCSI layer has completed a command. */
-static void lsi_command_complete(SCSIRequest *req, uint32_t status, size_t resid)
+static void lsi_command_complete(SCSIRequest *req, size_t resid)
{
LSIState *s = LSI53C895A(req->bus->qbus.parent);
int out;
out = (s->sstat1 & PHASE_MASK) == PHASE_DO;
- trace_lsi_command_complete(status);
- s->status = status;
+ trace_lsi_command_complete(req->status);
+ s->status = req->status;
s->command_complete = 2;
if (s->waiting && s->dbc != 0) {
/* Raise phase mismatch for short transfers. */
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index e24c12d7ee..35867dbd40 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1852,13 +1852,12 @@ static void megasas_xfer_complete(SCSIRequest *req, uint32_t len)
}
}
-static void megasas_command_complete(SCSIRequest *req, uint32_t status,
- size_t resid)
+static void megasas_command_complete(SCSIRequest *req, size_t resid)
{
MegasasCmd *cmd = req->hba_private;
uint8_t cmd_status = MFI_STAT_OK;
- trace_megasas_command_complete(cmd->index, status, resid);
+ trace_megasas_command_complete(cmd->index, req->status, resid);
if (req->io_canceled) {
return;
@@ -1873,7 +1872,6 @@ static void megasas_command_complete(SCSIRequest *req, uint32_t status,
return;
}
} else {
- req->status = status;
trace_megasas_scsi_complete(cmd->index, req->status,
cmd->iov_size, req->cmd.xfer);
if (req->status != GOOD) {
diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index 135e7d96e4..d4fbfb2da7 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -1133,7 +1133,7 @@ static QEMUSGList *mptsas_get_sg_list(SCSIRequest *sreq)
}
static void mptsas_command_complete(SCSIRequest *sreq,
- uint32_t status, size_t resid)
+ size_t resid)
{
MPTSASRequest *req = sreq->hba_private;
MPTSASState *s = req->dev;
@@ -1143,7 +1143,8 @@ static void mptsas_command_complete(SCSIRequest *sreq,
hwaddr sense_buffer_addr = req->dev->sense_buffer_high_addr |
req->scsi_io.SenseBufferLowAddr;
- trace_mptsas_command_complete(s, req->scsi_io.MsgContext, status, resid);
+ trace_mptsas_command_complete(s, req->scsi_io.MsgContext,
+ sreq->status, resid);
sense_len = scsi_req_get_sense(sreq, sense_buf, SCSI_SENSE_BUF_SIZE);
if (sense_len > 0) {
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 3284a5d1fb..17ce3238d7 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1415,7 +1415,7 @@ void scsi_req_complete(SCSIRequest *req, int status)
scsi_req_ref(req);
scsi_req_dequeue(req);
- req->bus->info->complete(req, req->status, req->resid);
+ req->bus->info->complete(req, req->resid);
/* Cancelled requests might end up being completed instead of cancelled */
notifier_list_notify(&req->cancel_notifiers, req);
diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index 4aa0224c47..d653b5a6ad 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -551,19 +551,19 @@ static void vscsi_transfer_data(SCSIRequest *sreq, uint32_t len)
}
/* Callback to indicate that the SCSI layer has completed a transfer. */
-static void vscsi_command_complete(SCSIRequest *sreq, uint32_t status, size_t resid)
+static void vscsi_command_complete(SCSIRequest *sreq, size_t resid)
{
VSCSIState *s = VIO_SPAPR_VSCSI_DEVICE(sreq->bus->qbus.parent);
vscsi_req *req = sreq->hba_private;
int32_t res_in = 0, res_out = 0;
- trace_spapr_vscsi_command_complete(sreq->tag, status, req);
+ trace_spapr_vscsi_command_complete(sreq->tag, sreq->status, req);
if (req == NULL) {
fprintf(stderr, "VSCSI: Can't find request for tag 0x%x\n", sreq->tag);
return;
}
- if (status == CHECK_CONDITION) {
+ if (sreq->status == CHECK_CONDITION) {
req->senselen = scsi_req_get_sense(req->sreq, req->sense,
sizeof(req->sense));
trace_spapr_vscsi_command_complete_sense_data1(req->senselen,
@@ -574,8 +574,8 @@ static void vscsi_command_complete(SCSIRequest *sreq, uint32_t status, size_t re
req->sense[12], req->sense[13], req->sense[14], req->sense[15]);
}
- trace_spapr_vscsi_command_complete_status(status);
- if (status == 0) {
+ trace_spapr_vscsi_command_complete_status(sreq->status);
+ if (sreq->status == 0) {
/* We handle overflows, not underflows for normal commands,
* but hopefully nobody cares
*/
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index c9873d5af7..64cd852d82 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -493,8 +493,7 @@ static void virtio_scsi_complete_cmd_req(VirtIOSCSIReq *req)
virtio_scsi_complete_req(req);
}
-static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status,
- size_t resid)
+static void virtio_scsi_command_complete(SCSIRequest *r, size_t resid)
{
VirtIOSCSIReq *req = r->hba_private;
uint8_t sense[SCSI_SENSE_BUF_SIZE];
@@ -506,7 +505,7 @@ static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status,
}
req->resp.cmd.response = VIRTIO_SCSI_S_OK;
- req->resp.cmd.status = status;
+ req->resp.cmd.status = r->status;
if (req->resp.cmd.status == GOOD) {
req->resp.cmd.resid = virtio_tswap32(vdev, resid);
} else {
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index a63d25de48..0da378ed50 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -511,7 +511,7 @@ pvscsi_write_sense(PVSCSIRequest *r, uint8_t *sense, int len)
}
static void
-pvscsi_command_complete(SCSIRequest *req, uint32_t status, size_t resid)
+pvscsi_command_complete(SCSIRequest *req, size_t resid)
{
PVSCSIRequest *pvscsi_req = req->hba_private;
PVSCSIState *s;
@@ -528,7 +528,7 @@ pvscsi_command_complete(SCSIRequest *req, uint32_t status, size_t resid)
pvscsi_req->cmp.hostStatus = BTSTAT_DATARUN;
}
- pvscsi_req->cmp.scsiStatus = status;
+ pvscsi_req->cmp.scsiStatus = req->status;
if (pvscsi_req->cmp.scsiStatus == CHECK_CONDITION) {
uint8_t sense[SCSI_SENSE_BUF_SIZE];
int sense_len =
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 648340323f..d1aeebe2ce 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -284,17 +284,17 @@ static void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
}
}
-static void usb_msd_command_complete(SCSIRequest *req, uint32_t status, size_t resid)
+static void usb_msd_command_complete(SCSIRequest *req, size_t resid)
{
MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
USBPacket *p = s->packet;
- DPRINTF("Command complete %d tag 0x%x\n", status, req->tag);
+ DPRINTF("Command complete %d tag 0x%x\n", req->status, req->tag);
s->csw.sig = cpu_to_le32(0x53425355);
s->csw.tag = cpu_to_le32(req->tag);
s->csw.residue = cpu_to_le32(s->data_len);
- s->csw.status = status != 0;
+ s->csw.status = req->status != 0;
if (s->packet) {
if (s->data_len == 0 && s->mode == USB_MSDM_DATAOUT) {
diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index cec071d96c..202e9cc4b7 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -597,17 +597,16 @@ static void usb_uas_scsi_transfer_data(SCSIRequest *r, uint32_t len)
}
}
-static void usb_uas_scsi_command_complete(SCSIRequest *r,
- uint32_t status, size_t resid)
+static void usb_uas_scsi_command_complete(SCSIRequest *r, size_t resid)
{
UASRequest *req = r->hba_private;
- trace_usb_uas_scsi_complete(req->uas->dev.addr, req->tag, status, resid);
+ trace_usb_uas_scsi_complete(req->uas->dev.addr, req->tag, r->status, resid);
req->complete = true;
if (req->data) {
usb_uas_complete_data_packet(req);
}
- usb_uas_queue_sense(req, status);
+ usb_uas_queue_sense(req, r->status);
scsi_req_unref(req->req);
}
diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
index 60cc3047a5..d8a6263c13 100644
--- a/include/hw/scsi/esp.h
+++ b/include/hw/scsi/esp.h
@@ -151,7 +151,7 @@ struct SysBusESPState {
void esp_dma_enable(ESPState *s, int irq, int level);
void esp_request_cancelled(SCSIRequest *req);
-void esp_command_complete(SCSIRequest *req, uint32_t status, size_t resid);
+void esp_command_complete(SCSIRequest *req, size_t resid);
void esp_transfer_data(SCSIRequest *req, uint32_t len);
void esp_hard_reset(ESPState *s);
uint64_t esp_reg_read(ESPState *s, uint32_t saddr);
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 21a6249743..23a9b23e50 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -123,7 +123,7 @@ struct SCSIBusInfo {
int (*parse_cdb)(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf,
void *hba_private);
void (*transfer_data)(SCSIRequest *req, uint32_t arg);
- void (*complete)(SCSIRequest *req, uint32_t arg, size_t resid);
+ void (*complete)(SCSIRequest *req, size_t resid);
void (*cancel)(SCSIRequest *req);
void (*change)(SCSIBus *bus, SCSIDevice *dev, SCSISense sense);
QEMUSGList *(*get_sg_list)(SCSIRequest *req);
--
2.16.4
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/7] scsi-disk: convert more errno values back to SCSI statuses
2020-11-16 18:40 [PATCH 0/7] scsi: scsi-disk corrupts data Hannes Reinecke
2020-11-16 18:40 ` [PATCH 1/7] scsi-disk: Add sg_io callback to evaluate status Hannes Reinecke
2020-11-16 18:40 ` [PATCH 2/7] scsi: drop 'result' argument from command_complete callback Hannes Reinecke
@ 2020-11-16 18:40 ` Hannes Reinecke
2020-11-16 18:40 ` [PATCH 4/7] scsi: Rename linux-specific SG_ERR codes to generic SCSI_HOST error codes Hannes Reinecke
` (4 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Hannes Reinecke @ 2020-11-16 18:40 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Hannes Reinecke
From: Paolo Bonzini <pbonzini@redhat.com>
Linux has some OS-specific (and sometimes weird) mappings for various SCSI
statuses and sense codes. The most important is probably RESERVATION
CONFLICT. Add them so that they can be reported back to the guest
kernel.
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi/scsi-disk.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 5d6c892f29..797779afd6 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -460,6 +460,25 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
}
error = scsi_sense_buf_to_errno(r->req.sense, sizeof(r->req.sense));
break;
+#ifdef CONFIG_LINUX
+ /* These errno mapping are specific to Linux. For more information:
+ * - scsi_decide_disposition in drivers/scsi/scsi_error.c
+ * - scsi_result_to_blk_status in drivers/scsi/scsi_lib.c
+ * - blk_errors[] in block/blk-core.c
+ */
+ case EBADE:
+ /* DID_NEXUS_FAILURE -> BLK_STS_NEXUS. */
+ scsi_req_complete(&r->req, RESERVATION_CONFLICT);
+ break;
+ case ENODATA:
+ /* DID_MEDIUM_ERROR -> BLK_STS_MEDIUM. */
+ scsi_check_condition(r, SENSE_CODE(READ_ERROR));
+ break;
+ case EREMOTEIO:
+ /* DID_TARGET_FAILURE -> BLK_STS_TARGET. */
+ scsi_req_complete(&r->req, HARDWARE_ERROR);
+ break;
+#endif
case ENOMEDIUM:
scsi_check_condition(r, SENSE_CODE(NO_MEDIUM));
break;
--
2.16.4
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 4/7] scsi: Rename linux-specific SG_ERR codes to generic SCSI_HOST error codes
2020-11-16 18:40 [PATCH 0/7] scsi: scsi-disk corrupts data Hannes Reinecke
` (2 preceding siblings ...)
2020-11-16 18:40 ` [PATCH 3/7] scsi-disk: convert more errno values back to SCSI statuses Hannes Reinecke
@ 2020-11-16 18:40 ` Hannes Reinecke
2020-11-16 18:40 ` [PATCH 5/7] scsi: Add mapping for generic SCSI_HOST status to sense codes Hannes Reinecke
` (3 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Hannes Reinecke @ 2020-11-16 18:40 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Hannes Reinecke
We really should make a distinction between legitimate sense codes
(ie if one is running against an emulated block device or for
pass-through sense codes), and the intermediate errors generated
during processing of the command, which really are not sense codes
but refer to some specific internal status. And this internal
state is not necessarily linux-specific, but rather can refer to
the qemu implementation itself.
So rename the linux-only SG_ERR codes to SCSI_HOST codes and make
them available generally.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
include/scsi/utils.h | 23 ++++++++++++++++-------
scsi/utils.c | 6 +++---
2 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/include/scsi/utils.h b/include/scsi/utils.h
index fbc5588279..a55ba2c1ea 100644
--- a/include/scsi/utils.h
+++ b/include/scsi/utils.h
@@ -16,6 +16,22 @@ enum SCSIXferMode {
SCSI_XFER_TO_DEV, /* WRITE, MODE_SELECT, ... */
};
+enum SCSIHostStatus {
+ SCSI_HOST_OK,
+ SCSI_HOST_NO_LUN,
+ SCSI_HOST_BUSY,
+ SCSI_HOST_TIME_OUT,
+ SCSI_HOST_BAD_RESPONSE,
+ SCSI_HOST_ABORTED,
+ SCSI_HOST_ERROR = 0x07,
+ SCSI_HOST_RESET = 0x08,
+ SCSI_HOST_TRANSPORT_DISRUPTED = 0xe,
+ SCSI_HOST_TARGET_FAILURE = 0x10,
+ SCSI_HOST_RESERVATION_ERROR = 0x11,
+ SCSI_HOST_ALLOCATION_FAILURE = 0x12,
+ SCSI_HOST_MEDIUM_ERROR = 0x13,
+};
+
typedef struct SCSICommand {
uint8_t buf[SCSI_CMD_BUF_SIZE];
int len;
@@ -122,13 +138,6 @@ int scsi_cdb_length(uint8_t *buf);
#define SG_ERR_DRIVER_TIMEOUT 0x06
#define SG_ERR_DRIVER_SENSE 0x08
-#define SG_ERR_DID_OK 0x00
-#define SG_ERR_DID_NO_CONNECT 0x01
-#define SG_ERR_DID_BUS_BUSY 0x02
-#define SG_ERR_DID_TIME_OUT 0x03
-
-#define SG_ERR_DRIVER_SENSE 0x08
-
int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
SCSISense *sense);
#endif
diff --git a/scsi/utils.c b/scsi/utils.c
index b37c283014..262ef1c3ea 100644
--- a/scsi/utils.c
+++ b/scsi/utils.c
@@ -576,9 +576,9 @@ int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
return CHECK_CONDITION;
}
} else {
- if (io_hdr->host_status == SG_ERR_DID_NO_CONNECT ||
- io_hdr->host_status == SG_ERR_DID_BUS_BUSY ||
- io_hdr->host_status == SG_ERR_DID_TIME_OUT ||
+ if (io_hdr->host_status == SCSI_HOST_NO_LUN ||
+ io_hdr->host_status == SCSI_HOST_BUSY ||
+ io_hdr->host_status == SCSI_HOST_TIME_OUT ||
(io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT)) {
return BUSY;
} else if (io_hdr->host_status) {
--
2.16.4
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 5/7] scsi: Add mapping for generic SCSI_HOST status to sense codes
2020-11-16 18:40 [PATCH 0/7] scsi: scsi-disk corrupts data Hannes Reinecke
` (3 preceding siblings ...)
2020-11-16 18:40 ` [PATCH 4/7] scsi: Rename linux-specific SG_ERR codes to generic SCSI_HOST error codes Hannes Reinecke
@ 2020-11-16 18:40 ` Hannes Reinecke
2020-11-16 18:57 ` Paolo Bonzini
2020-11-16 18:40 ` [PATCH 6/7] scsi: split sg_io_sense_from_errno() in two functions Hannes Reinecke
` (2 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Hannes Reinecke @ 2020-11-16 18:40 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Hannes Reinecke
As we don't have a driver-specific mapping (yet) we should provide
for a detailed mapping from host_status to SCSI sense codes.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
scsi/utils.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 55 insertions(+), 5 deletions(-)
diff --git a/scsi/utils.c b/scsi/utils.c
index 262ef1c3ea..ae68881184 100644
--- a/scsi/utils.c
+++ b/scsi/utils.c
@@ -252,6 +252,21 @@ const struct SCSISense sense_code_LUN_COMM_FAILURE = {
.key = ABORTED_COMMAND, .asc = 0x08, .ascq = 0x00
};
+/* Command aborted, LUN does not respond to selection */
+const struct SCSISense sense_code_LUN_NOT_RESPONDING = {
+ .key = ABORTED_COMMAND, .asc = 0x05, .ascq = 0x00
+};
+
+/* Command aborted, Command Timeout during processing */
+const struct SCSISense sense_code_COMMAND_TIMEOUT = {
+ .key = ABORTED_COMMAND, .asc = 0x2e, .ascq = 0x02
+};
+
+/* Command aborted, Commands cleared by device server */
+const struct SCSISense sense_code_COMMAND_ABORTED = {
+ .key = ABORTED_COMMAND, .asc = 0x2f, .ascq = 0x02
+};
+
/* Medium Error, Unrecovered read error */
const struct SCSISense sense_code_READ_ERROR = {
.key = MEDIUM_ERROR, .asc = 0x11, .ascq = 0x00
@@ -568,6 +583,14 @@ int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
switch (errno_value) {
case EDOM:
return TASK_SET_FULL;
+ case EBADE:
+ return RESERVATION_CONFLICT;
+ case ENODATA:
+ *sense = SENSE_CODE(READ_ERROR);
+ return CHECK_CONDITION;
+ case EREMOTEIO:
+ *sense = SENSE_CODE(LUN_COMM_FAILURE);
+ return CHECK_CONDITION;
case ENOMEM:
*sense = SENSE_CODE(TARGET_FAILURE);
return CHECK_CONDITION;
@@ -576,14 +599,41 @@ int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
return CHECK_CONDITION;
}
} else {
- if (io_hdr->host_status == SCSI_HOST_NO_LUN ||
- io_hdr->host_status == SCSI_HOST_BUSY ||
- io_hdr->host_status == SCSI_HOST_TIME_OUT ||
- (io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT)) {
+ switch (io_hdr->host_status) {
+ case SCSI_HOST_NO_LUN:
+ *sense = SENSE_CODE(LUN_NOT_RESPONDING);
+ return CHECK_CONDITION;
+ case SCSI_HOST_BUSY:
return BUSY;
- } else if (io_hdr->host_status) {
+ case SCSI_HOST_TIME_OUT:
+ *sense = SENSE_CODE(COMMAND_TIMEOUT);
+ return CHECK_CONDITION;
+ case SCSI_HOST_BAD_RESPONSE:
+ *sense = SENSE_CODE(LUN_COMM_FAILURE);
+ return CHECK_CONDITION;
+ case SCSI_HOST_ABORTED:
+ *sense = SENSE_CODE(COMMAND_ABORTED);
+ return CHECK_CONDITION;
+ case SCSI_HOST_RESET:
+ *sense = SENSE_CODE(RESET);
+ return CHECK_CONDITION;
+ case SCSI_HOST_TRANSPORT_DISRUPTED:
*sense = SENSE_CODE(I_T_NEXUS_LOSS);
return CHECK_CONDITION;
+ case SCSI_HOST_TARGET_FAILURE:
+ *sense = SENSE_CODE(TARGET_FAILURE);
+ return CHECK_CONDITION;
+ case SCSI_HOST_RESERVATION_ERROR:
+ return RESERVATION_CONFLICT;
+ case SCSI_HOST_ALLOCATION_FAILURE:
+ *sense = SENSE_CODE(SPACE_ALLOC_FAILED);
+ return CHECK_CONDITION;
+ case SCSI_HOST_MEDIUM_ERROR:
+ *sense = SENSE_CODE(READ_ERROR);
+ return CHECK_CONDITION;
+ }
+ if (io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT) {
+ return BUSY;
} else if (io_hdr->status) {
return io_hdr->status;
} else if (io_hdr->driver_status & SG_ERR_DRIVER_SENSE) {
--
2.16.4
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 6/7] scsi: split sg_io_sense_from_errno() in two functions
2020-11-16 18:40 [PATCH 0/7] scsi: scsi-disk corrupts data Hannes Reinecke
` (4 preceding siblings ...)
2020-11-16 18:40 ` [PATCH 5/7] scsi: Add mapping for generic SCSI_HOST status to sense codes Hannes Reinecke
@ 2020-11-16 18:40 ` Hannes Reinecke
2020-11-16 18:40 ` [PATCH 7/7] scsi: move host_status handling into SCSI drivers Hannes Reinecke
2020-12-17 10:07 ` [PATCH 0/7] scsi: scsi-disk corrupts data Paolo Bonzini
7 siblings, 0 replies; 20+ messages in thread
From: Hannes Reinecke @ 2020-11-16 18:40 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Hannes Reinecke
Currently sg_io_sense_from_errno() converts the two input parameters
'errno' and 'io_hdr' into sense code and SCSI status. This patch
splits this off into two functions scsi_sense_from_errno() and
scsi_sense_from_host_status(), both of which are available generically.
This allows us to use the function scsi_sense_from_errno() in
scsi-disk.c instead of the switch statement, allowing us to consolidate
the errno handling.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
hw/scsi/scsi-disk.c | 72 +++++++++++--------------
hw/scsi/scsi-generic.c | 19 +++++--
include/scsi/utils.h | 6 +--
scsi/qemu-pr-helper.c | 14 +++--
scsi/utils.c | 139 ++++++++++++++++++++++++++-----------------------
5 files changed, 134 insertions(+), 116 deletions(-)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 797779afd6..6eb0aa3d27 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -445,8 +445,7 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
if (acct_failed) {
block_acct_failed(blk_get_stats(s->qdev.conf.blk), &r->acct);
}
- switch (error) {
- case 0:
+ if (error == 0) {
/* A passthrough command has run and has produced sense data; check
* whether the error has to be handled by the guest or should rather
* pause the host.
@@ -459,41 +458,16 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
return true;
}
error = scsi_sense_buf_to_errno(r->req.sense, sizeof(r->req.sense));
- break;
-#ifdef CONFIG_LINUX
- /* These errno mapping are specific to Linux. For more information:
- * - scsi_decide_disposition in drivers/scsi/scsi_error.c
- * - scsi_result_to_blk_status in drivers/scsi/scsi_lib.c
- * - blk_errors[] in block/blk-core.c
- */
- case EBADE:
- /* DID_NEXUS_FAILURE -> BLK_STS_NEXUS. */
- scsi_req_complete(&r->req, RESERVATION_CONFLICT);
- break;
- case ENODATA:
- /* DID_MEDIUM_ERROR -> BLK_STS_MEDIUM. */
- scsi_check_condition(r, SENSE_CODE(READ_ERROR));
- break;
- case EREMOTEIO:
- /* DID_TARGET_FAILURE -> BLK_STS_TARGET. */
- scsi_req_complete(&r->req, HARDWARE_ERROR);
- break;
-#endif
- case ENOMEDIUM:
- scsi_check_condition(r, SENSE_CODE(NO_MEDIUM));
- break;
- case ENOMEM:
- scsi_check_condition(r, SENSE_CODE(TARGET_FAILURE));
- break;
- case EINVAL:
- scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
- break;
- case ENOSPC:
- scsi_check_condition(r, SENSE_CODE(SPACE_ALLOC_FAILED));
- break;
- default:
- scsi_check_condition(r, SENSE_CODE(IO_ERROR));
- break;
+ } else {
+ SCSISense sense;
+ int status;
+
+ status = scsi_sense_from_errno(error, &sense);
+ if (status == CHECK_CONDITION)
+ scsi_build_sense(r->req.sense, sense);
+ sdc->update_sense(&r->req);
+ scsi_req_complete(&r->req, status);
+ return true;
}
}
@@ -2714,13 +2688,29 @@ static void scsi_block_sgio_complete(void *opaque, int ret)
{
SCSIBlockReq *req = (SCSIBlockReq *)opaque;
SCSIDiskReq *r = &req->req;
+ sg_io_hdr_t io_hdr = req->io_header;
SCSISense sense;
+ int status;
- r->req.status = sg_io_sense_from_errno(-ret, &req->io_header, &sense);
- if (r->req.status == CHECK_CONDITION &&
- req->io_header.status != CHECK_CONDITION)
+ status = scsi_sense_from_errno(-ret, &sense);
+ if (status == CHECK_CONDITION) {
scsi_req_build_sense(&r->req, sense);
-
+ } else if (status == GOOD &&
+ io_hdr.host_status != SCSI_HOST_OK) {
+ status = scsi_sense_from_host_status(io_hdr.host_status, &sense);
+ if (status == CHECK_CONDITION) {
+ scsi_req_build_sense(&r->req, sense);
+ }
+ } else if (io_hdr.status == CHECK_CONDITION ||
+ io_hdr.driver_status & SG_ERR_DRIVER_SENSE) {
+ status = CHECK_CONDITION;
+ r->req.sense_len = io_hdr.sb_len_wr;
+ } else if (io_hdr.driver_status & SG_ERR_DRIVER_TIMEOUT) {
+ status = BUSY;
+ } else if (io_hdr.status) {
+ status = io_hdr.status;
+ }
+ r->req.status = status;
req->cb(req->cb_opaque, ret);
}
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 8687336438..a2b85678b5 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -74,6 +74,7 @@ static void scsi_command_complete_noio(SCSIGenericReq *r, int ret)
{
int status;
SCSISense sense;
+ sg_io_hdr_t io_hdr = r->io_header;
assert(r->req.aiocb == NULL);
@@ -81,13 +82,23 @@ static void scsi_command_complete_noio(SCSIGenericReq *r, int ret)
scsi_req_cancel_complete(&r->req);
goto done;
}
- status = sg_io_sense_from_errno(-ret, &r->io_header, &sense);
+ status = scsi_sense_from_errno(-ret, &sense);
if (status == CHECK_CONDITION) {
- if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
- r->req.sense_len = r->io_header.sb_len_wr;
- } else {
+ scsi_req_build_sense(&r->req, sense);
+ } else if (status == GOOD &&
+ io_hdr.host_status != SCSI_HOST_OK) {
+ status = scsi_sense_from_host_status(io_hdr.host_status, &sense);
+ if (status == CHECK_CONDITION) {
scsi_req_build_sense(&r->req, sense);
}
+ } else if (io_hdr.status == CHECK_CONDITION ||
+ io_hdr.driver_status & SG_ERR_DRIVER_SENSE) {
+ status = CHECK_CONDITION;
+ r->req.sense_len = io_hdr.sb_len_wr;
+ } else if (io_hdr.driver_status & SG_ERR_DRIVER_TIMEOUT) {
+ status = BUSY;
+ } else if (io_hdr.status != GOOD) {
+ status = io_hdr.status;
}
trace_scsi_generic_command_complete_noio(r, r->req.tag, status);
diff --git a/include/scsi/utils.h b/include/scsi/utils.h
index a55ba2c1ea..581caf15e0 100644
--- a/include/scsi/utils.h
+++ b/include/scsi/utils.h
@@ -137,9 +137,9 @@ int scsi_cdb_length(uint8_t *buf);
#ifdef CONFIG_LINUX
#define SG_ERR_DRIVER_TIMEOUT 0x06
#define SG_ERR_DRIVER_SENSE 0x08
-
-int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
- SCSISense *sense);
#endif
+int scsi_sense_from_errno(int errno_value, SCSISense *sense);
+int scsi_sense_from_host_status(uint8_t host_status, SCSISense *sense);
+
#endif
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index d26faaf91e..e6e217a61b 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -134,7 +134,7 @@ static int do_sgio_worker(void *opaque)
PRHelperSGIOData *data = opaque;
struct sg_io_hdr io_hdr;
int ret;
- int status;
+ int status = GOOD;
SCSISense sense_code;
memset(data->sense, 0, PR_HELPER_SENSE_SIZE);
@@ -149,8 +149,16 @@ static int do_sgio_worker(void *opaque)
io_hdr.dxferp = (char *)data->buf;
io_hdr.dxfer_len = data->sz;
ret = ioctl(data->fd, SG_IO, &io_hdr);
- status = sg_io_sense_from_errno(ret < 0 ? errno : 0, &io_hdr,
- &sense_code);
+
+ if (ret < 0) {
+ status = scsi_sense_from_errno(errno, &sense_code);
+ }
+ if (status == GOOD && io_hdr.host_status != SCSI_HOST_OK) {
+ status = scsi_sense_from_host_status(io_hdr.host_status, &sense_code);
+ }
+ if (status == GOOD && io_hdr.driver_status & SG_ERR_DRIVER_SENSE) {
+ status = CHECK_CONDITION;
+ }
if (status == GOOD) {
data->sz -= io_hdr.resid;
} else {
diff --git a/scsi/utils.c b/scsi/utils.c
index ae68881184..6cb75154e8 100644
--- a/scsi/utils.c
+++ b/scsi/utils.c
@@ -575,72 +575,81 @@ const char *scsi_command_name(uint8_t cmd)
return names[cmd];
}
+int scsi_sense_from_errno(int errno_value, SCSISense *sense)
+{
+ switch (errno_value) {
+ case 0:
+ return GOOD;
+ case EDOM:
+ return TASK_SET_FULL;
#ifdef CONFIG_LINUX
-int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
- SCSISense *sense)
+ /* These errno mapping are specific to Linux. For more information:
+ * - scsi_decide_disposition in drivers/scsi/scsi_error.c
+ * - scsi_result_to_blk_status in drivers/scsi/scsi_lib.c
+ * - blk_errors[] in block/blk-core.c
+ */
+ case EBADE:
+ return RESERVATION_CONFLICT;
+ case ENODATA:
+ *sense = SENSE_CODE(READ_ERROR);
+ return CHECK_CONDITION;
+ case EREMOTEIO:
+ *sense = SENSE_CODE(LUN_COMM_FAILURE);
+ return CHECK_CONDITION;
+#endif
+ case ENOMEDIUM:
+ *sense = SENSE_CODE(NO_MEDIUM);
+ return CHECK_CONDITION;
+ case ENOMEM:
+ *sense = SENSE_CODE(TARGET_FAILURE);
+ return CHECK_CONDITION;
+ case EINVAL:
+ *sense = SENSE_CODE(INVALID_FIELD);
+ return CHECK_CONDITION;
+ case ENOSPC:
+ *sense = SENSE_CODE(SPACE_ALLOC_FAILED);
+ return CHECK_CONDITION;
+ default:
+ *sense = SENSE_CODE(IO_ERROR);
+ return CHECK_CONDITION;
+ }
+}
+
+int scsi_sense_from_host_status(uint8_t host_status,
+ SCSISense *sense)
{
- if (errno_value != 0) {
- switch (errno_value) {
- case EDOM:
- return TASK_SET_FULL;
- case EBADE:
- return RESERVATION_CONFLICT;
- case ENODATA:
- *sense = SENSE_CODE(READ_ERROR);
- return CHECK_CONDITION;
- case EREMOTEIO:
- *sense = SENSE_CODE(LUN_COMM_FAILURE);
- return CHECK_CONDITION;
- case ENOMEM:
- *sense = SENSE_CODE(TARGET_FAILURE);
- return CHECK_CONDITION;
- default:
- *sense = SENSE_CODE(IO_ERROR);
- return CHECK_CONDITION;
- }
- } else {
- switch (io_hdr->host_status) {
- case SCSI_HOST_NO_LUN:
- *sense = SENSE_CODE(LUN_NOT_RESPONDING);
- return CHECK_CONDITION;
- case SCSI_HOST_BUSY:
- return BUSY;
- case SCSI_HOST_TIME_OUT:
- *sense = SENSE_CODE(COMMAND_TIMEOUT);
- return CHECK_CONDITION;
- case SCSI_HOST_BAD_RESPONSE:
- *sense = SENSE_CODE(LUN_COMM_FAILURE);
- return CHECK_CONDITION;
- case SCSI_HOST_ABORTED:
- *sense = SENSE_CODE(COMMAND_ABORTED);
- return CHECK_CONDITION;
- case SCSI_HOST_RESET:
- *sense = SENSE_CODE(RESET);
- return CHECK_CONDITION;
- case SCSI_HOST_TRANSPORT_DISRUPTED:
- *sense = SENSE_CODE(I_T_NEXUS_LOSS);
- return CHECK_CONDITION;
- case SCSI_HOST_TARGET_FAILURE:
- *sense = SENSE_CODE(TARGET_FAILURE);
- return CHECK_CONDITION;
- case SCSI_HOST_RESERVATION_ERROR:
- return RESERVATION_CONFLICT;
- case SCSI_HOST_ALLOCATION_FAILURE:
- *sense = SENSE_CODE(SPACE_ALLOC_FAILED);
- return CHECK_CONDITION;
- case SCSI_HOST_MEDIUM_ERROR:
- *sense = SENSE_CODE(READ_ERROR);
- return CHECK_CONDITION;
- }
- if (io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT) {
- return BUSY;
- } else if (io_hdr->status) {
- return io_hdr->status;
- } else if (io_hdr->driver_status & SG_ERR_DRIVER_SENSE) {
- return CHECK_CONDITION;
- } else {
- return GOOD;
- }
+ switch (host_status) {
+ case SCSI_HOST_NO_LUN:
+ *sense = SENSE_CODE(LUN_NOT_RESPONDING);
+ return CHECK_CONDITION;
+ case SCSI_HOST_BUSY:
+ return BUSY;
+ case SCSI_HOST_TIME_OUT:
+ *sense = SENSE_CODE(COMMAND_TIMEOUT);
+ return CHECK_CONDITION;
+ case SCSI_HOST_BAD_RESPONSE:
+ *sense = SENSE_CODE(LUN_COMM_FAILURE);
+ return CHECK_CONDITION;
+ case SCSI_HOST_ABORTED:
+ *sense = SENSE_CODE(COMMAND_ABORTED);
+ return CHECK_CONDITION;
+ case SCSI_HOST_RESET:
+ *sense = SENSE_CODE(RESET);
+ return CHECK_CONDITION;
+ case SCSI_HOST_TRANSPORT_DISRUPTED:
+ *sense = SENSE_CODE(I_T_NEXUS_LOSS);
+ return CHECK_CONDITION;
+ case SCSI_HOST_TARGET_FAILURE:
+ *sense = SENSE_CODE(TARGET_FAILURE);
+ return CHECK_CONDITION;
+ case SCSI_HOST_RESERVATION_ERROR:
+ return RESERVATION_CONFLICT;
+ case SCSI_HOST_ALLOCATION_FAILURE:
+ *sense = SENSE_CODE(SPACE_ALLOC_FAILED);
+ return CHECK_CONDITION;
+ case SCSI_HOST_MEDIUM_ERROR:
+ *sense = SENSE_CODE(READ_ERROR);
+ return CHECK_CONDITION;
}
+ return GOOD;
}
-#endif
--
2.16.4
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 7/7] scsi: move host_status handling into SCSI drivers
2020-11-16 18:40 [PATCH 0/7] scsi: scsi-disk corrupts data Hannes Reinecke
` (5 preceding siblings ...)
2020-11-16 18:40 ` [PATCH 6/7] scsi: split sg_io_sense_from_errno() in two functions Hannes Reinecke
@ 2020-11-16 18:40 ` Hannes Reinecke
2020-11-16 18:58 ` Paolo Bonzini
2020-12-17 10:07 ` [PATCH 0/7] scsi: scsi-disk corrupts data Paolo Bonzini
7 siblings, 1 reply; 20+ messages in thread
From: Hannes Reinecke @ 2020-11-16 18:40 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Hannes Reinecke
Some SCSI drivers like virtio have an internal mapping for the
host_status. This patch moves the host_status translation into
the SCSI drivers to allow those drivers to set up the correct
values.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
hw/scsi/esp.c | 10 ++++++++++
hw/scsi/lsi53c895a.c | 11 +++++++++++
hw/scsi/megasas.c | 9 +++++++++
hw/scsi/mptsas.c | 9 +++++++++
hw/scsi/scsi-disk.c | 10 ++++------
hw/scsi/scsi-generic.c | 8 +++-----
hw/scsi/spapr_vscsi.c | 12 +++++++++++-
hw/scsi/virtio-scsi.c | 41 +++++++++++++++++++++++++++++++++++++++--
hw/scsi/vmw_pvscsi.c | 25 +++++++++++++++++++++++++
include/hw/scsi/scsi.h | 3 ++-
10 files changed, 123 insertions(+), 15 deletions(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 93d9c9c7b9..fc88cfac23 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -28,6 +28,8 @@
#include "migration/vmstate.h"
#include "hw/irq.h"
#include "hw/scsi/esp.h"
+#include "scsi/utils.h"
+#include "scsi/constants.h"
#include "trace.h"
#include "qemu/log.h"
#include "qemu/module.h"
@@ -489,6 +491,14 @@ void esp_command_complete(SCSIRequest *req, size_t resid)
{
ESPState *s = req->hba_private;
+ if (req->host_status != SCSI_HOST_OK) {
+ SCSISense sense;
+
+ req->status = scsi_sense_from_host_status(req->host_status, &sense);
+ if (req->status == CHECK_CONDITION) {
+ scsi_req_build_sense(req, sense);
+ }
+ }
if (s->rregs[ESP_RSTAT] & STAT_INT) {
/* Defer handling command complete until the previous
* interrupt has been handled.
diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index a4e58580e4..b6aa98c95a 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -18,6 +18,8 @@
#include "hw/irq.h"
#include "hw/pci/pci.h"
#include "hw/scsi/scsi.h"
+#include "scsi/utils.h"
+#include "scsi/constants.h"
#include "migration/vmstate.h"
#include "sysemu/dma.h"
#include "qemu/log.h"
@@ -792,6 +794,15 @@ static void lsi_command_complete(SCSIRequest *req, size_t resid)
LSIState *s = LSI53C895A(req->bus->qbus.parent);
int out;
+ if (req->host_status != SCSI_HOST_OK) {
+ SCSISense sense;
+
+ req->status = scsi_sense_from_host_status(req->host_status, &sense);
+ if (req->status == CHECK_CONDITION) {
+ scsi_req_build_sense(req, sense);
+ }
+ }
+
out = (s->sstat1 & PHASE_MASK) == PHASE_DO;
trace_lsi_command_complete(req->status);
s->status = req->status;
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 35867dbd40..1f7d806ffa 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1857,6 +1857,15 @@ static void megasas_command_complete(SCSIRequest *req, size_t resid)
MegasasCmd *cmd = req->hba_private;
uint8_t cmd_status = MFI_STAT_OK;
+ if (req->host_status != SCSI_HOST_OK) {
+ SCSISense sense;
+
+ req->status = scsi_sense_from_host_status(req->host_status, &sense);
+ if (req->status == CHECK_CONDITION) {
+ scsi_req_build_sense(req, sense);
+ }
+ }
+
trace_megasas_command_complete(cmd->index, req->status, resid);
if (req->io_canceled) {
diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index d4fbfb2da7..be3875ce94 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -1143,6 +1143,15 @@ static void mptsas_command_complete(SCSIRequest *sreq,
hwaddr sense_buffer_addr = req->dev->sense_buffer_high_addr |
req->scsi_io.SenseBufferLowAddr;
+ if (sreq->host_status == SCSI_HOST_OK) {
+ SCSISense sense;
+
+ sreq->status = scsi_sense_from_host_status(sreq->host_status, &sense);
+ if (sreq->status == CHECK_CONDITION) {
+ scsi_req_build_sense(sreq, sense);
+ }
+ }
+
trace_mptsas_command_complete(s, req->scsi_io.MsgContext,
sreq->status, resid);
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 6eb0aa3d27..c0cb63707d 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1840,7 +1840,7 @@ static void scsi_disk_emulate_write_data(SCSIRequest *req)
case VERIFY_10:
case VERIFY_12:
case VERIFY_16:
- if (r->req.status == -1) {
+ if (r->req.status == GOOD) {
scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
}
break;
@@ -2122,7 +2122,7 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
}
illegal_request:
- if (r->req.status == -1) {
+ if (r->req.status == GOOD) {
scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
}
return 0;
@@ -2697,10 +2697,8 @@ static void scsi_block_sgio_complete(void *opaque, int ret)
scsi_req_build_sense(&r->req, sense);
} else if (status == GOOD &&
io_hdr.host_status != SCSI_HOST_OK) {
- status = scsi_sense_from_host_status(io_hdr.host_status, &sense);
- if (status == CHECK_CONDITION) {
- scsi_req_build_sense(&r->req, sense);
- }
+ status = INTERMEDIATE_GOOD;
+ r->req.host_status = io_hdr.host_status;
} else if (io_hdr.status == CHECK_CONDITION ||
io_hdr.driver_status & SG_ERR_DRIVER_SENSE) {
status = CHECK_CONDITION;
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index a2b85678b5..eac108fc6e 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -86,11 +86,9 @@ static void scsi_command_complete_noio(SCSIGenericReq *r, int ret)
if (status == CHECK_CONDITION) {
scsi_req_build_sense(&r->req, sense);
} else if (status == GOOD &&
- io_hdr.host_status != SCSI_HOST_OK) {
- status = scsi_sense_from_host_status(io_hdr.host_status, &sense);
- if (status == CHECK_CONDITION) {
- scsi_req_build_sense(&r->req, sense);
- }
+ io_hdr.host_status != SCSI_HOST_OK) {
+ status = INTERMEDIATE_GOOD;
+ r->req.host_status = io_hdr.host_status;
} else if (io_hdr.status == CHECK_CONDITION ||
io_hdr.driver_status & SG_ERR_DRIVER_SENSE) {
status = CHECK_CONDITION;
diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index d653b5a6ad..9327a493c3 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -556,6 +556,16 @@ static void vscsi_command_complete(SCSIRequest *sreq, size_t resid)
VSCSIState *s = VIO_SPAPR_VSCSI_DEVICE(sreq->bus->qbus.parent);
vscsi_req *req = sreq->hba_private;
int32_t res_in = 0, res_out = 0;
+ uint8_t status = sreq->status;
+
+ if (sreq->host_status != SCSI_HOST_OK) {
+ SCSISense sense;
+
+ sreq->status = scsi_sense_from_host_status(sreq->host_status, &sense);
+ if (sreq->status == CHECK_CONDITION) {
+ scsi_req_build_sense(sreq, sense);
+ }
+ }
trace_spapr_vscsi_command_complete(sreq->tag, sreq->status, req);
if (req == NULL) {
@@ -575,7 +585,7 @@ static void vscsi_command_complete(SCSIRequest *sreq, size_t resid)
}
trace_spapr_vscsi_command_complete_status(sreq->status);
- if (sreq->status == 0) {
+ if (sreq->status == GOOD) {
/* We handle overflows, not underflows for normal commands,
* but hopefully nobody cares
*/
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 64cd852d82..62c56713d8 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -504,8 +504,45 @@ static void virtio_scsi_command_complete(SCSIRequest *r, size_t resid)
return;
}
- req->resp.cmd.response = VIRTIO_SCSI_S_OK;
- req->resp.cmd.status = r->status;
+ if (r->host_status != SCSI_HOST_OK) {
+ req->resp.cmd.status = GOOD;
+ switch (r->host_status) {
+ case SCSI_HOST_NO_LUN:
+ req->resp.cmd.response = VIRTIO_SCSI_S_INCORRECT_LUN;
+ break;
+ case SCSI_HOST_BUSY:
+ req->resp.cmd.response = VIRTIO_SCSI_S_BUSY;
+ break;
+ case SCSI_HOST_TIME_OUT:
+ case SCSI_HOST_ABORTED:
+ req->resp.cmd.response = VIRTIO_SCSI_S_ABORTED;
+ break;
+ case SCSI_HOST_BAD_RESPONSE:
+ req->resp.cmd.response = VIRTIO_SCSI_S_BAD_TARGET;
+ break;
+ case SCSI_HOST_RESET:
+ req->resp.cmd.response = VIRTIO_SCSI_S_RESET;
+ break;
+ case SCSI_HOST_TRANSPORT_DISRUPTED:
+ req->resp.cmd.response = VIRTIO_SCSI_S_TRANSPORT_FAILURE;
+ break;
+ case SCSI_HOST_TARGET_FAILURE:
+ req->resp.cmd.response = VIRTIO_SCSI_S_TARGET_FAILURE;
+ break;
+ case SCSI_HOST_RESERVATION_ERROR:
+ req->resp.cmd.response = VIRTIO_SCSI_S_NEXUS_FAILURE;
+ break;
+ case SCSI_HOST_ALLOCATION_FAILURE:
+ case SCSI_HOST_MEDIUM_ERROR:
+ case SCSI_HOST_ERROR:
+ default:
+ req->resp.cmd.response = VIRTIO_SCSI_S_FAILURE;
+ break;
+ }
+ } else {
+ req->resp.cmd.response = VIRTIO_SCSI_S_OK;
+ req->resp.cmd.status = r->status;
+ }
if (req->resp.cmd.status == GOOD) {
req->resp.cmd.resid = virtio_tswap32(vdev, resid);
} else {
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index 0da378ed50..2583105dfd 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -522,6 +522,31 @@ pvscsi_command_complete(SCSIRequest *req, size_t resid)
}
s = pvscsi_req->dev;
+ if (req->host_status != SCSI_HOST_OK) {
+ switch (req->host_status) {
+ case SCSI_HOST_NO_LUN:
+ pvscsi_req->cmp.hostStatus = BTSTAT_LUNMISMATCH;
+ break;
+ case SCSI_HOST_BUSY:
+ pvscsi_req->cmp.hostStatus = BTSTAT_ABORTQUEUE;
+ break;
+ case SCSI_HOST_TIME_OUT:
+ case SCSI_HOST_ABORTED:
+ pvscsi_req->cmp.hostStatus = BTSTAT_SENTRST;
+ break;
+ case SCSI_HOST_BAD_RESPONSE:
+ pvscsi_req->cmp.hostStatus = BTSTAT_SELTIMEO;
+ break;
+ case SCSI_HOST_RESET:
+ pvscsi_req->cmp.hostStatus = BTSTAT_BUSRESET;
+ break;
+ default:
+ pvscsi_req->cmp.hostStatus = BTSTAT_HASOFTWARE;
+ break;
+ }
+ req->status = GOOD;
+ }
+
if (resid) {
/* Short transfer. */
trace_pvscsi_command_complete_data_run();
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 23a9b23e50..a097fecb2a 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -27,7 +27,8 @@ struct SCSIRequest {
uint32_t refcount;
uint32_t tag;
uint32_t lun;
- uint32_t status;
+ uint8_t status;
+ uint8_t host_status;
void *hba_private;
size_t resid;
SCSICommand cmd;
--
2.16.4
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 5/7] scsi: Add mapping for generic SCSI_HOST status to sense codes
2020-11-16 18:40 ` [PATCH 5/7] scsi: Add mapping for generic SCSI_HOST status to sense codes Hannes Reinecke
@ 2020-11-16 18:57 ` Paolo Bonzini
2020-11-16 19:03 ` Hannes Reinecke
0 siblings, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2020-11-16 18:57 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: qemu-devel
On 16/11/20 19:40, Hannes Reinecke wrote:
> + case SCSI_HOST_TARGET_FAILURE:
> + *sense = SENSE_CODE(TARGET_FAILURE);
> + return CHECK_CONDITION;
> + case SCSI_HOST_RESERVATION_ERROR:
> + return RESERVATION_CONFLICT;
> + case SCSI_HOST_ALLOCATION_FAILURE:
> + *sense = SENSE_CODE(SPACE_ALLOC_FAILED);
> + return CHECK_CONDITION;
> + case SCSI_HOST_MEDIUM_ERROR:
> + *sense = SENSE_CODE(READ_ERROR);
> + return CHECK_CONDITION;
Can these actually be visible to userspace? I'd rather avoid having
them in QEMU if possible.
Otherwise, the patches are completely sensible.
Paolo
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 7/7] scsi: move host_status handling into SCSI drivers
2020-11-16 18:40 ` [PATCH 7/7] scsi: move host_status handling into SCSI drivers Hannes Reinecke
@ 2020-11-16 18:58 ` Paolo Bonzini
2020-11-16 19:05 ` Hannes Reinecke
0 siblings, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2020-11-16 18:58 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: qemu-devel
On 16/11/20 19:40, Hannes Reinecke wrote:
>
> + if (sreq->host_status == SCSI_HOST_OK) {
> + SCSISense sense;
> +
> + sreq->status = scsi_sense_from_host_status(sreq->host_status, &sense);
> + if (sreq->status == CHECK_CONDITION) {
> + scsi_req_build_sense(sreq, sense);
> + }
> + }
Should be != of course.
Paolo
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/7] scsi: Add mapping for generic SCSI_HOST status to sense codes
2020-11-16 18:57 ` Paolo Bonzini
@ 2020-11-16 19:03 ` Hannes Reinecke
2020-11-16 20:05 ` Paolo Bonzini
0 siblings, 1 reply; 20+ messages in thread
From: Hannes Reinecke @ 2020-11-16 19:03 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On 11/16/20 7:57 PM, Paolo Bonzini wrote:
> On 16/11/20 19:40, Hannes Reinecke wrote:
>> + case SCSI_HOST_TARGET_FAILURE:
>> + *sense = SENSE_CODE(TARGET_FAILURE);
>> + return CHECK_CONDITION;
>> + case SCSI_HOST_RESERVATION_ERROR:
>> + return RESERVATION_CONFLICT;
>> + case SCSI_HOST_ALLOCATION_FAILURE:
>> + *sense = SENSE_CODE(SPACE_ALLOC_FAILED);
>> + return CHECK_CONDITION;
>> + case SCSI_HOST_MEDIUM_ERROR:
>> + *sense = SENSE_CODE(READ_ERROR);
>> + return CHECK_CONDITION;
>
> Can these actually be visible to userspace? I'd rather avoid having
> them in QEMU if possible.
>
> Otherwise, the patches are completely sensible.
>
And I did it exactly for the opposite purpose: rather than painstakingly
figuring out which codes _might_ be returned (and be utterly surprised
if we missed some) add an interpretation for every _possible_ code,
avoiding nasty surprises.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 7/7] scsi: move host_status handling into SCSI drivers
2020-11-16 18:58 ` Paolo Bonzini
@ 2020-11-16 19:05 ` Hannes Reinecke
2020-11-16 22:00 ` Paolo Bonzini
0 siblings, 1 reply; 20+ messages in thread
From: Hannes Reinecke @ 2020-11-16 19:05 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On 11/16/20 7:58 PM, Paolo Bonzini wrote:
> On 16/11/20 19:40, Hannes Reinecke wrote:
>> + if (sreq->host_status == SCSI_HOST_OK) {
>> + SCSISense sense;
>> +
>> + sreq->status = scsi_sense_from_host_status(sreq->host_status,
>> &sense);
>> + if (sreq->status == CHECK_CONDITION) {
>> + scsi_req_build_sense(sreq, sense);
>> + }
>> + }
>
> Should be != of course.
>
No.
scsi_req_build_sense() transfers the sense code from the second argument
into a proper SCSI sense. Which is only set if the status is
CHECK_CONDITION...
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/7] scsi: Add mapping for generic SCSI_HOST status to sense codes
2020-11-16 19:03 ` Hannes Reinecke
@ 2020-11-16 20:05 ` Paolo Bonzini
2020-11-17 6:53 ` Hannes Reinecke
0 siblings, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2020-11-16 20:05 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: qemu-devel
On 16/11/20 20:03, Hannes Reinecke wrote:
>>
>>> + case SCSI_HOST_TARGET_FAILURE:
>>> + *sense = SENSE_CODE(TARGET_FAILURE);
>>> + return CHECK_CONDITION;
>>> + case SCSI_HOST_RESERVATION_ERROR:
>>> + return RESERVATION_CONFLICT;
>>> + case SCSI_HOST_ALLOCATION_FAILURE:
>>> + *sense = SENSE_CODE(SPACE_ALLOC_FAILED);
>>> + return CHECK_CONDITION;
>>> + case SCSI_HOST_MEDIUM_ERROR:
>>> + *sense = SENSE_CODE(READ_ERROR);
>>> + return CHECK_CONDITION;
>>
>> Can these actually be visible to userspace? I'd rather avoid having
>> them in QEMU if possible.
>>
>> Otherwise, the patches are completely sensible.
>>
> And I did it exactly for the opposite purpose: rather than painstakingly
> figuring out which codes _might_ be returned (and be utterly surprised
> if we missed some) add an interpretation for every _possible_ code,
> avoiding nasty surprises.
And that certainly makes sense too.
On the other hand it'd be nice if Linux was clearer about which the
SCSI_HOST values are part of the userspace API and which are just an
(ugly) implementation detail.
Paolo
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 7/7] scsi: move host_status handling into SCSI drivers
2020-11-16 19:05 ` Hannes Reinecke
@ 2020-11-16 22:00 ` Paolo Bonzini
2020-11-17 6:55 ` Hannes Reinecke
0 siblings, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2020-11-16 22:00 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: qemu-devel
On 16/11/20 20:05, Hannes Reinecke wrote:
>>> + if (sreq->host_status == SCSI_HOST_OK) {
>>> + SCSISense sense;
>>> +
>>> + sreq->status =
>>> scsi_sense_from_host_status(sreq->host_status, &sense);
>>> + if (sreq->status == CHECK_CONDITION) {
>>> + scsi_req_build_sense(sreq, sense);
>>> + }
>>> + }
>>
>> Should be != of course.
>>
> No.
> scsi_req_build_sense() transfers the sense code from the second argument
> into a proper SCSI sense. Which is only set if the status is
> CHECK_CONDITION...
I mean sreq->host_status != SCSI_HOST_OK. I might be wrong, but every
other HBA is using that...
Paolo
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/7] scsi: Add mapping for generic SCSI_HOST status to sense codes
2020-11-16 20:05 ` Paolo Bonzini
@ 2020-11-17 6:53 ` Hannes Reinecke
0 siblings, 0 replies; 20+ messages in thread
From: Hannes Reinecke @ 2020-11-17 6:53 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On 11/16/20 9:05 PM, Paolo Bonzini wrote:
> On 16/11/20 20:03, Hannes Reinecke wrote:
>>>
>>>> + case SCSI_HOST_TARGET_FAILURE:
>>>> + *sense = SENSE_CODE(TARGET_FAILURE);
>>>> + return CHECK_CONDITION;
>>>> + case SCSI_HOST_RESERVATION_ERROR:
>>>> + return RESERVATION_CONFLICT;
>>>> + case SCSI_HOST_ALLOCATION_FAILURE:
>>>> + *sense = SENSE_CODE(SPACE_ALLOC_FAILED);
>>>> + return CHECK_CONDITION;
>>>> + case SCSI_HOST_MEDIUM_ERROR:
>>>> + *sense = SENSE_CODE(READ_ERROR);
>>>> + return CHECK_CONDITION;
>>>
>>> Can these actually be visible to userspace? I'd rather avoid having
>>> them in QEMU if possible.
>>>
>>> Otherwise, the patches are completely sensible.
>>>
>> And I did it exactly for the opposite purpose: rather than
>> painstakingly figuring out which codes _might_ be returned (and be
>> utterly surprised if we missed some) add an interpretation for every
>> _possible_ code, avoiding nasty surprises.
>
> And that certainly makes sense too.
>
> On the other hand it'd be nice if Linux was clearer about which the
> SCSI_HOST values are part of the userspace API and which are just an
> (ugly) implementation detail.
>
Oh, I certainly agree with that.
But that is more of a long-term prospect; I do see some discussions
ahead if one were to try it. Especially as (like DID_BAD_TARGET and
DID_NO_CONNECT) have no clear distinction between them, and are used
more-or-less interchangeably.
But a clear definition of these values would inevitably lead to a change
in various drivers, which then would lead to a change in behaviour, and
a possible user-space regression.
So not that easy, sadly.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 7/7] scsi: move host_status handling into SCSI drivers
2020-11-16 22:00 ` Paolo Bonzini
@ 2020-11-17 6:55 ` Hannes Reinecke
2020-11-17 7:38 ` Paolo Bonzini
0 siblings, 1 reply; 20+ messages in thread
From: Hannes Reinecke @ 2020-11-17 6:55 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On 11/16/20 11:00 PM, Paolo Bonzini wrote:
> On 16/11/20 20:05, Hannes Reinecke wrote:
>>>> + if (sreq->host_status == SCSI_HOST_OK) {
>>>> + SCSISense sense;
>>>> +
>>>> + sreq->status =
>>>> scsi_sense_from_host_status(sreq->host_status, &sense);
>>>> + if (sreq->status == CHECK_CONDITION) {
>>>> + scsi_req_build_sense(sreq, sense);
>>>> + }
>>>> + }
>>>
>>> Should be != of course.
>>>
>> No.
>> scsi_req_build_sense() transfers the sense code from the second argument
>> into a proper SCSI sense. Which is only set if the status is
>> CHECK_CONDITION...
>
> I mean sreq->host_status != SCSI_HOST_OK. I might be wrong, but every
> other HBA is using that...
>
Bah. Yes, of course, you are right.
Shall I resubmit? Or how is the process nowadays?
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 7/7] scsi: move host_status handling into SCSI drivers
2020-11-17 6:55 ` Hannes Reinecke
@ 2020-11-17 7:38 ` Paolo Bonzini
2020-11-17 8:50 ` Hannes Reinecke
0 siblings, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2020-11-17 7:38 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: qemu-devel
On 17/11/20 07:55, Hannes Reinecke wrote:
> On 11/16/20 11:00 PM, Paolo Bonzini wrote:
>> On 16/11/20 20:05, Hannes Reinecke wrote:
>>>>> + if (sreq->host_status == SCSI_HOST_OK) {
>>>>> + SCSISense sense;
>>>>> +
>>>>> + sreq->status =
>>>>> scsi_sense_from_host_status(sreq->host_status, &sense);
>>>>> + if (sreq->status == CHECK_CONDITION) {
>>>>> + scsi_req_build_sense(sreq, sense);
>>>>> + }
>>>>> + }
>>>>
>>>> Should be != of course.
>>>>
>>> No.
>>> scsi_req_build_sense() transfers the sense code from the second argument
>>> into a proper SCSI sense. Which is only set if the status is
>>> CHECK_CONDITION...
>>
>> I mean sreq->host_status != SCSI_HOST_OK. I might be wrong, but every
>> other HBA is using that...
>>
> Bah. Yes, of course, you are right.
>
> Shall I resubmit? Or how is the process nowadays?
Depends on how busy and grumpy I am. :) Since we're right in the middle
of the freeze, let me send a RFC patch for Linux to clean up DID_* a
little bit.
Paolo
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 7/7] scsi: move host_status handling into SCSI drivers
2020-11-17 7:38 ` Paolo Bonzini
@ 2020-11-17 8:50 ` Hannes Reinecke
2020-11-17 11:10 ` Paolo Bonzini
0 siblings, 1 reply; 20+ messages in thread
From: Hannes Reinecke @ 2020-11-17 8:50 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On 11/17/20 8:38 AM, Paolo Bonzini wrote:
> On 17/11/20 07:55, Hannes Reinecke wrote:
>> On 11/16/20 11:00 PM, Paolo Bonzini wrote:
>>> On 16/11/20 20:05, Hannes Reinecke wrote:
>>>>>> + if (sreq->host_status == SCSI_HOST_OK) {
>>>>>> + SCSISense sense;
>>>>>> +
>>>>>> + sreq->status =
>>>>>> scsi_sense_from_host_status(sreq->host_status, &sense);
>>>>>> + if (sreq->status == CHECK_CONDITION) {
>>>>>> + scsi_req_build_sense(sreq, sense);
>>>>>> + }
>>>>>> + }
>>>>>
>>>>> Should be != of course.
>>>>>
>>>> No.
>>>> scsi_req_build_sense() transfers the sense code from the second
>>>> argument
>>>> into a proper SCSI sense. Which is only set if the status is
>>>> CHECK_CONDITION...
>>>
>>> I mean sreq->host_status != SCSI_HOST_OK. I might be wrong, but
>>> every other HBA is using that...
>>>
>> Bah. Yes, of course, you are right.
>>
>> Shall I resubmit? Or how is the process nowadays?
>
> Depends on how busy and grumpy I am. :) Since we're right in the middle
> of the freeze, let me send a RFC patch for Linux to clean up DID_* a
> little bit.
>
What's your intention there? I do have (of course) a larger patchset for
revisiting the SCSI status codes, so I could resubmit those portions
relating to DID_ codes ...
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 7/7] scsi: move host_status handling into SCSI drivers
2020-11-17 8:50 ` Hannes Reinecke
@ 2020-11-17 11:10 ` Paolo Bonzini
0 siblings, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2020-11-17 11:10 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: qemu-devel
On 17/11/20 09:50, Hannes Reinecke wrote:
>> Since we're right in the middle of the freeze, let me send a RFC patch
>> for Linux to clean up DID_* a little bit.
>>
> What's your intention there? I do have (of course) a larger patchset for
> revisiting the SCSI status codes, so I could resubmit those portions
> relating to DID_ codes ...
Not much. First, renaming DID_NEXUS_FAILURE and BLK_STS_NEXUS to
DID_RESERVATION_CONFLICT and BLK_STS_RESERVATION respectively. Second,
adding a __ in front of those DID_* constants that are ultimately
changed back to DID_OK.
Thanks,
Paolo
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/7] scsi: scsi-disk corrupts data
2020-11-16 18:40 [PATCH 0/7] scsi: scsi-disk corrupts data Hannes Reinecke
` (6 preceding siblings ...)
2020-11-16 18:40 ` [PATCH 7/7] scsi: move host_status handling into SCSI drivers Hannes Reinecke
@ 2020-12-17 10:07 ` Paolo Bonzini
7 siblings, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2020-12-17 10:07 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: qemu-devel
On 16/11/20 19:40, Hannes Reinecke wrote:
> Hi all,
>
> a customer of ours reported repeated data corruption in the guest following a command abort.
> After lengthy debugging we found that scsi-disk (and scsi-generic, for that matter) ignores
> the host_status field from SG_IO once a command is aborted. If the command is aborted, SG_IO
> will return with a SCSI status 'GOOD', and host_status 'DID_TIME_OUT'. scsi-disk will now
> ignore the DID_TIME_OUT setting, and just report the SCSI status back to the guest.
> The guest will then assume everything is okay and not retry the command, leading to the data
> corruption.
>
> This patchset moves the (linux only) SG_ERR host_status codes to generic code as SCSI_HOST
> values, and adds a host_status field to SCSIRequest. With that some drivers like virtio_scsi
> can interpret the host_status code and map it onto it driver-specific status.
> This status is then visible to the guest, which then is able to take appropriate action.
>
> As usual, comments and reviews are welcome.
>
> Hannes Reinecke (6):
> scsi-disk: Add sg_io callback to evaluate status
> scsi: drop 'result' argument from command_complete callback
> scsi: Rename linux-specific SG_ERR codes to generic SCSI_HOST error
> codes
> scsi: Add mapping for generic SCSI_HOST status to sense codes
> scsi: split sg_io_sense_from_errno() in two functions
> scsi: move host_status handling into SCSI drivers
>
> Paolo Bonzini (1):
> scsi-disk: convert more errno values back to SCSI statuses
>
> hw/scsi/esp-pci.c | 5 +--
> hw/scsi/esp.c | 17 +++++--
> hw/scsi/lsi53c895a.c | 17 +++++--
> hw/scsi/megasas.c | 15 +++++--
> hw/scsi/mptsas.c | 14 +++++-
> hw/scsi/scsi-bus.c | 2 +-
> hw/scsi/scsi-disk.c | 75 ++++++++++++++++++++-----------
> hw/scsi/scsi-generic.c | 21 ++++++---
> hw/scsi/spapr_vscsi.c | 20 ++++++---
> hw/scsi/virtio-scsi.c | 44 ++++++++++++++++--
> hw/scsi/vmw_pvscsi.c | 29 +++++++++++-
> hw/usb/dev-storage.c | 6 +--
> hw/usb/dev-uas.c | 7 ++-
> include/hw/scsi/esp.h | 2 +-
> include/hw/scsi/scsi.h | 5 ++-
> include/scsi/utils.h | 29 +++++++-----
> scsi/qemu-pr-helper.c | 14 ++++--
> scsi/utils.c | 119 ++++++++++++++++++++++++++++++++++++-------------
> 18 files changed, 328 insertions(+), 113 deletions(-)
>
Queued, thanks.
Paolo
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-12-17 10:12 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-16 18:40 [PATCH 0/7] scsi: scsi-disk corrupts data Hannes Reinecke
2020-11-16 18:40 ` [PATCH 1/7] scsi-disk: Add sg_io callback to evaluate status Hannes Reinecke
2020-11-16 18:40 ` [PATCH 2/7] scsi: drop 'result' argument from command_complete callback Hannes Reinecke
2020-11-16 18:40 ` [PATCH 3/7] scsi-disk: convert more errno values back to SCSI statuses Hannes Reinecke
2020-11-16 18:40 ` [PATCH 4/7] scsi: Rename linux-specific SG_ERR codes to generic SCSI_HOST error codes Hannes Reinecke
2020-11-16 18:40 ` [PATCH 5/7] scsi: Add mapping for generic SCSI_HOST status to sense codes Hannes Reinecke
2020-11-16 18:57 ` Paolo Bonzini
2020-11-16 19:03 ` Hannes Reinecke
2020-11-16 20:05 ` Paolo Bonzini
2020-11-17 6:53 ` Hannes Reinecke
2020-11-16 18:40 ` [PATCH 6/7] scsi: split sg_io_sense_from_errno() in two functions Hannes Reinecke
2020-11-16 18:40 ` [PATCH 7/7] scsi: move host_status handling into SCSI drivers Hannes Reinecke
2020-11-16 18:58 ` Paolo Bonzini
2020-11-16 19:05 ` Hannes Reinecke
2020-11-16 22:00 ` Paolo Bonzini
2020-11-17 6:55 ` Hannes Reinecke
2020-11-17 7:38 ` Paolo Bonzini
2020-11-17 8:50 ` Hannes Reinecke
2020-11-17 11:10 ` Paolo Bonzini
2020-12-17 10:07 ` [PATCH 0/7] scsi: scsi-disk corrupts data Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).