* [Qemu-devel] [PATCH] [v2] scsi-disk: correct SCSI error reporting
@ 2008-10-09 16:34 Laurent Vivier
2008-10-09 18:12 ` Blue Swirl
2008-10-11 9:34 ` Blue Swirl
0 siblings, 2 replies; 3+ messages in thread
From: Laurent Vivier @ 2008-10-09 16:34 UTC (permalink / raw)
To: qemu-devel@nongnu.org
[-- Attachment #1: Type: text/plain, Size: 2490 bytes --]
Hi,
scsi-disk.c seems to not report to SCSI controller (through completion()
routine) the good error value.
I've tested this by using a little program trying to read beyond the
end of the disk (see attachment read10.c)
On real disk:
# ./read10 /dev/sg0
READ_10 duration=40 millisecs, resid=0, msg_status=0 status=2 sense=0
driver_status 8
If I use this command with qemu and a scsi disk, the result is:
# ./read10 /dev/sg0
sym0: bad DSA (5da40ff) in done queue.
sd 0:0:0:0: ABORT operation started
sym0: SCSI BUS reset detected.
sym0: SCSI BUS has been reset.
sd 0:0:0:0: ABORT operation complete.
sd 0:0:0:0: ABORT operation started
sd 0:0:0:0: ABORT operation failed.
sd 0:0:0:0: DEVICE RESET operation started
target0:0:0: control msgout: c.
lsi_scsi: error: Unimplemented message 0x0c
target0:0:0: has been reset
sd 0:0:0:0: DEVICE RESET operation complete.
sd 0:0:0:0: DEVICE RESET operation started
target0:0:0: control msgout: c.
lsi_scsi: error: Unimplemented message 0x0c
target0:0:0: has been reset
sd 0:0:0:0: DEVICE RESET operation complete.
sd 0:0:0:0: M_REJECT received (0:0).
sd 0:0:0:0: M_REJECT received (0:0).
READ_10 duration=30032 millisecs, resid=0, msg_status=0 status=0 sense=0
driver_status 0
The following patch corrects this issue by sending to the controller not
the sense key but the status code.
The result of the command is now:
# ./read10 /dev/sg0
READ_10 duration=0 millisecs, resid=0, msg_status=0 status=2 sense=0
driver_status 8
This patch has been tested with linux (x86_64 and sparc) and windows XP.
The VERIFY commands has been implemented (empty) because previously it
was simply ignored (and it is needed by windows to format disks).
Changelog:
[v2] The v1 was very close to the (reversed) patch of Marcelo Tosatti
(r4260) which was breaking sparc scsi support. In fact, management of
invalid LUN was wrong: on an invalid LUN, disk sends a SCSI status of
"CHECK_CONDITION" and the driver sends "REQUEST_SENSE" to retrieve the
sense key. The disk must accept and answer to this request (to send the
sense key ILLEGAL_REQUEST). Moreover, INQUIRY with an invalid LUN must
answer with PERIPHERAL QUALIFIER field set to 011b and PERIPHERAL DEVICE
TYPE field set to 1Fh.
Laurent
--
----------------- Laurent.Vivier@bull.net ------------------
"La perfection est atteinte non quand il ne reste rien à
ajouter mais quand il ne reste rien à enlever." Saint Exupéry
[-- Attachment #2: Type: text/x-csrc, Size: 2457 bytes --]
#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <errno.h>
#include <sys/ioctl.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <scsi/sg.h>
/* modified file from /usr/share/doc/sg3-utils/examples/sg_simple16.c
* * Copyright (C) 2001 D. Gilbert
* */
#define READ10_REPLY_LEN 512
#define READ10_CMD_LEN 10
#define EBUFF_SZ 256
int main(int argc, char * argv[])
{
int sg_fd, k, ok;
unsigned char r10CmdBlk [READ10_CMD_LEN] =
{0x28, 0, 0x7f, 0xff, 0xff, 0xff, 0, 0, 1, 0};
sg_io_hdr_t io_hdr;
char * file_name = 0;
char ebuff[EBUFF_SZ];
unsigned char inBuff[READ10_REPLY_LEN];
unsigned char sense_buffer[32];
for (k = 1; k < argc; ++k) {
if (*argv[k] == '-') {
printf("Unrecognized switch: %s\n", argv[k]);
file_name = 0;
break;
}
else if (0 == file_name)
file_name = argv[k];
else {
printf("too many arguments\n");
file_name = 0;
break;
}
}
if (0 == file_name) {
printf("Usage: 'sg_simple10 <sg_device>'\n");
return 1;
}
if ((sg_fd = open(file_name, O_RDWR)) < 0) {
snprintf(ebuff, EBUFF_SZ,
"sg_simple10: error opening file: %s", file_name);
perror(ebuff);
return 1;
}
/* Prepare READ_10 command */
memset(&io_hdr, 0, sizeof(sg_io_hdr_t));
io_hdr.interface_id = 'S';
io_hdr.cmd_len = sizeof(r10CmdBlk);
/* io_hdr.iovec_count = 0; */ /* memset takes care of this */
io_hdr.mx_sb_len = sizeof(sense_buffer);
io_hdr.dxfer_direction = SG_DXFER_FROM_DEV;
io_hdr.dxfer_len = READ10_REPLY_LEN;
io_hdr.dxferp = inBuff;
io_hdr.cmdp = r10CmdBlk;
io_hdr.sbp = sense_buffer;
io_hdr.timeout = 20000; /* 20000 millisecs == 20 seconds */
/* io_hdr.flags = 0; */ /* take defaults: indirect IO, etc */
/* io_hdr.pack_id = 0; */
/* io_hdr.usr_ptr = NULL; */
if (ioctl(sg_fd, SG_IO, &io_hdr) < 0) {
perror("sg_simple10: Inquiry SG_IO ioctl error");
close(sg_fd);
return 1;
}
printf("READ_10 duration=%u millisecs, resid=%d, msg_status=%d status=%d sense=%d driver_status %x\n",
io_hdr.duration, io_hdr.resid, (int)io_hdr.msg_status,
io_hdr.status, sense_buffer[2], io_hdr.driver_status);
close(sg_fd);
return 0;
}
[-- Attachment #3: Type: text/x-patch, Size: 5779 bytes --]
Signed-off-by: Laurent Vivier <Laurent.Vivier@bull.net>
---
hw/scsi-disk.c | 42 +++++++++++++++++++++++++++---------------
1 file changed, 27 insertions(+), 15 deletions(-)
Index: qemu/hw/scsi-disk.c
===================================================================
--- qemu.orig/hw/scsi-disk.c 2008-10-09 16:32:38.000000000 +0200
+++ qemu/hw/scsi-disk.c 2008-10-09 18:08:12.000000000 +0200
@@ -34,6 +34,9 @@ do { fprintf(stderr, "scsi-disk: " fmt ,
#define SENSE_HARDWARE_ERROR 4
#define SENSE_ILLEGAL_REQUEST 5
+#define STATUS_GOOD 0
+#define STATUS_CHECK_CONDITION 2
+
#define SCSI_DMA_BUF_SIZE 131072
typedef struct SCSIRequest {
@@ -124,15 +127,15 @@ static SCSIRequest *scsi_find_request(SC
}
/* Helper function for command completion. */
-static void scsi_command_complete(SCSIRequest *r, int sense)
+static void scsi_command_complete(SCSIRequest *r, int status, int sense)
{
SCSIDeviceState *s = r->dev;
uint32_t tag;
- DPRINTF("Command complete tag=0x%x sense=%d\n", r->tag, sense);
+ DPRINTF("Command complete tag=0x%x status=%d sense=%d\n", r->tag, status, sense);
s->sense = sense;
tag = r->tag;
scsi_remove_request(r);
- s->completion(s->opaque, SCSI_REASON_DONE, tag, sense);
+ s->completion(s->opaque, SCSI_REASON_DONE, tag, status);
}
/* Cancel a pending data transfer. */
@@ -157,7 +160,8 @@ static void scsi_read_complete(void * op
if (ret) {
DPRINTF("IO error\n");
- scsi_command_complete(r, SENSE_HARDWARE_ERROR);
+ s->completion(s->opaque, SCSI_REASON_DATA, r->tag, 0);
+ scsi_command_complete(r, STATUS_CHECK_CONDITION, SENSE_NO_SENSE);
return;
}
DPRINTF("Data ready tag=0x%x len=%d\n", r->tag, r->buf_len);
@@ -176,7 +180,7 @@ static void scsi_read_data(SCSIDevice *d
if (!r) {
BADF("Bad read tag 0x%x\n", tag);
/* ??? This is the wrong error. */
- scsi_command_complete(r, SENSE_HARDWARE_ERROR);
+ scsi_command_complete(r, STATUS_CHECK_CONDITION, SENSE_HARDWARE_ERROR);
return;
}
if (r->sector_count == (uint32_t)-1) {
@@ -187,7 +191,7 @@ static void scsi_read_data(SCSIDevice *d
}
DPRINTF("Read sector_count=%d\n", r->sector_count);
if (r->sector_count == 0) {
- scsi_command_complete(r, SENSE_NO_SENSE);
+ scsi_command_complete(r, STATUS_GOOD, SENSE_NO_SENSE);
return;
}
@@ -199,7 +203,7 @@ static void scsi_read_data(SCSIDevice *d
r->aiocb = bdrv_aio_read(s->bdrv, r->sector, r->dma_buf, n,
scsi_read_complete, r);
if (r->aiocb == NULL)
- scsi_command_complete(r, SENSE_HARDWARE_ERROR);
+ scsi_command_complete(r, STATUS_CHECK_CONDITION, SENSE_HARDWARE_ERROR);
r->sector += n;
r->sector_count -= n;
}
@@ -217,7 +221,7 @@ static void scsi_write_complete(void * o
r->aiocb = NULL;
if (r->sector_count == 0) {
- scsi_command_complete(r, SENSE_NO_SENSE);
+ scsi_command_complete(r, STATUS_GOOD, SENSE_NO_SENSE);
} else {
len = r->sector_count * 512;
if (len > SCSI_DMA_BUF_SIZE) {
@@ -241,7 +245,7 @@ static int scsi_write_data(SCSIDevice *d
r = scsi_find_request(s, tag);
if (!r) {
BADF("Bad write tag 0x%x\n", tag);
- scsi_command_complete(r, SENSE_HARDWARE_ERROR);
+ scsi_command_complete(r, STATUS_CHECK_CONDITION, SENSE_HARDWARE_ERROR);
return 1;
}
if (r->aiocb)
@@ -251,7 +255,8 @@ static int scsi_write_data(SCSIDevice *d
r->aiocb = bdrv_aio_write(s->bdrv, r->sector, r->dma_buf, n,
scsi_write_complete, r);
if (r->aiocb == NULL)
- scsi_command_complete(r, SENSE_HARDWARE_ERROR);
+ scsi_command_complete(r, STATUS_CHECK_CONDITION,
+ SENSE_HARDWARE_ERROR);
r->sector += n;
r->sector_count -= n;
} else {
@@ -344,7 +349,8 @@ static int32_t scsi_send_command(SCSIDev
if (lun || buf[1] >> 5) {
/* Only LUN 0 supported. */
DPRINTF("Unimplemented LUN %d\n", lun ? lun : buf[1] >> 5);
- goto fail;
+ if (command != 0x03 && command != 0x12) /* REQUEST SENSE and INQUIRY */
+ goto fail;
}
switch (command) {
case 0x0:
@@ -487,7 +493,10 @@ static int32_t scsi_send_command(SCSIDev
}
}
memset(outbuf, 0, 36);
- if (bdrv_get_type_hint(s->bdrv) == BDRV_TYPE_CDROM) {
+
+ if (lun || buf[1] >> 5) {
+ outbuf[0] = 0x7f; /* LUN not supported */
+ } else if (bdrv_get_type_hint(s->bdrv) == BDRV_TYPE_CDROM) {
outbuf[0] = 5;
outbuf[1] = 0x80;
memcpy(&outbuf[16], "QEMU CD-ROM ", 16);
@@ -670,7 +679,7 @@ static int32_t scsi_send_command(SCSIDev
outbuf[7] = 0;
r->buf_len = 8;
} else {
- scsi_command_complete(r, SENSE_NOT_READY);
+ scsi_command_complete(r, STATUS_CHECK_CONDITION, SENSE_NOT_READY);
return 0;
}
break;
@@ -754,14 +763,17 @@ static int32_t scsi_send_command(SCSIDev
outbuf[3] = 8;
r->buf_len = 16;
break;
+ case 0x2f:
+ DPRINTF("Verify (sector %d, count %d)\n", lba, len);
+ break;
default:
DPRINTF("Unknown SCSI command (%2.2x)\n", buf[0]);
fail:
- scsi_command_complete(r, SENSE_ILLEGAL_REQUEST);
+ scsi_command_complete(r, STATUS_CHECK_CONDITION, SENSE_ILLEGAL_REQUEST);
return 0;
}
if (r->sector_count == 0 && r->buf_len == 0) {
- scsi_command_complete(r, SENSE_NO_SENSE);
+ scsi_command_complete(r, STATUS_GOOD, SENSE_NO_SENSE);
}
len = r->sector_count * 512 + r->buf_len;
if (is_write) {
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] [v2] scsi-disk: correct SCSI error reporting
2008-10-09 16:34 [Qemu-devel] [PATCH] [v2] scsi-disk: correct SCSI error reporting Laurent Vivier
@ 2008-10-09 18:12 ` Blue Swirl
2008-10-11 9:34 ` Blue Swirl
1 sibling, 0 replies; 3+ messages in thread
From: Blue Swirl @ 2008-10-09 18:12 UTC (permalink / raw)
To: qemu-devel
On 10/9/08, Laurent Vivier <Laurent.Vivier@bull.net> wrote:
> Hi,
>
> scsi-disk.c seems to not report to SCSI controller (through completion()
> routine) the good error value.
>
> I've tested this by using a little program trying to read beyond the
> end of the disk (see attachment read10.c)
>
> On real disk:
>
> # ./read10 /dev/sg0
> READ_10 duration=40 millisecs, resid=0, msg_status=0 status=2 sense=0
> driver_status 8
>
> If I use this command with qemu and a scsi disk, the result is:
>
> # ./read10 /dev/sg0
> sym0: bad DSA (5da40ff) in done queue.
> sd 0:0:0:0: ABORT operation started
> sym0: SCSI BUS reset detected.
> sym0: SCSI BUS has been reset.
> sd 0:0:0:0: ABORT operation complete.
> sd 0:0:0:0: ABORT operation started
> sd 0:0:0:0: ABORT operation failed.
> sd 0:0:0:0: DEVICE RESET operation started
> target0:0:0: control msgout: c.
> lsi_scsi: error: Unimplemented message 0x0c
> target0:0:0: has been reset
> sd 0:0:0:0: DEVICE RESET operation complete.
> sd 0:0:0:0: DEVICE RESET operation started
> target0:0:0: control msgout: c.
> lsi_scsi: error: Unimplemented message 0x0c
> target0:0:0: has been reset
> sd 0:0:0:0: DEVICE RESET operation complete.
> sd 0:0:0:0: M_REJECT received (0:0).
> sd 0:0:0:0: M_REJECT received (0:0).
> READ_10 duration=30032 millisecs, resid=0, msg_status=0 status=0 sense=0
> driver_status 0
>
> The following patch corrects this issue by sending to the controller not
> the sense key but the status code.
>
> The result of the command is now:
>
> # ./read10 /dev/sg0
> READ_10 duration=0 millisecs, resid=0, msg_status=0 status=2 sense=0
> driver_status 8
>
> This patch has been tested with linux (x86_64 and sparc) and windows XP.
>
> The VERIFY commands has been implemented (empty) because previously it
> was simply ignored (and it is needed by windows to format disks).
>
> Changelog:
>
> [v2] The v1 was very close to the (reversed) patch of Marcelo Tosatti
> (r4260) which was breaking sparc scsi support. In fact, management of
> invalid LUN was wrong: on an invalid LUN, disk sends a SCSI status of
> "CHECK_CONDITION" and the driver sends "REQUEST_SENSE" to retrieve the
> sense key. The disk must accept and answer to this request (to send the
> sense key ILLEGAL_REQUEST). Moreover, INQUIRY with an invalid LUN must
> answer with PERIPHERAL QUALIFIER field set to 011b and PERIPHERAL DEVICE
> TYPE field set to 1Fh.
This patch fixes LUN handling and does not cause problems like the
earlier one. I'd like to apply this in a few days, unless there are
objections.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] [v2] scsi-disk: correct SCSI error reporting
2008-10-09 16:34 [Qemu-devel] [PATCH] [v2] scsi-disk: correct SCSI error reporting Laurent Vivier
2008-10-09 18:12 ` Blue Swirl
@ 2008-10-11 9:34 ` Blue Swirl
1 sibling, 0 replies; 3+ messages in thread
From: Blue Swirl @ 2008-10-11 9:34 UTC (permalink / raw)
To: qemu-devel
On 10/9/08, Laurent Vivier <Laurent.Vivier@bull.net> wrote:
> Hi,
>
> scsi-disk.c seems to not report to SCSI controller (through completion()
> routine) the good error value.
Thanks, applied as r5455.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2008-10-11 9:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-10-09 16:34 [Qemu-devel] [PATCH] [v2] scsi-disk: correct SCSI error reporting Laurent Vivier
2008-10-09 18:12 ` Blue Swirl
2008-10-11 9:34 ` Blue Swirl
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).