[Qemu-devel] [patch] fix scsi-generic

[Qemu-devel] [patch] fix scsi-generic

[adq]

[-- Attachment #1: Type: text/plain, Size: 1098 bytes --]

Hi, I've been tracking down why scsi generic devices (using SG_IO)
don't work any more. After adding debug, I can see that it actually
submits the scsi CDB in hw/scsi-generic.c/execute_command(), but that
the hw/scsi-generic.c/scsi_read_complete() callback is never called.

This is because these are done with ioctls, and the posix async ioctl
code is, I think, broken right now. Some more debugging, led me to
posix-aio-compat.c/posix_aio_process_queue():

            if (acb->async_context_id != async_context_id) {

The async_context_ids don't match, so the request is never handled.
This is because the acb->async_context_id field is not initialised in
posix-aio-compat.c/paio_ioctl() (compare with
posix-aio-compat.c/paio_submit()). The attached patch adds the missing
line in.

This seems to fix the problem. Of course, /now/ I'm getting other
weird problems (as I'm trying to see if I can get slysoft anydvd
working in a KVM winXP vm), but they need further investigation and
likely other fixes.

Please forgive me if I'm mistaken in this, I've only just started
looking at the qemu code.

[-- Attachment #2: posix-aio-ioctl-async-context-id.patch --]
[-- Type: text/x-patch, Size: 498 bytes --]

diff -Naur qemu-kvm-0.12.5.orig//posix-aio-compat.c qemu-kvm-0.12.5/posix-aio-compat.c
--- qemu-kvm-0.12.5.orig//posix-aio-compat.c	2010-07-27 01:43:53.000000000 +0100
+++ qemu-kvm-0.12.5/posix-aio-compat.c	2010-08-07 01:49:07.051265778 +0100
@@ -597,6 +597,7 @@
     acb->aio_type = QEMU_AIO_IOCTL;
     acb->aio_fildes = fd;
     acb->ev_signo = SIGUSR2;
+    acb->async_context_id = get_async_context_id();
     acb->aio_offset = 0;
     acb->aio_ioctl_buf = buf;
     acb->aio_ioctl_cmd = req;

[Qemu-devel] Re: [patch] fix scsi-generic

[adq]

On 7 August 2010 01:55, adq <adq@lidskialf.net> wrote:
> Hi, I've been tracking down why scsi generic devices (using SG_IO)
> don't work any more. After adding debug, I can see that it actually
> submits the scsi CDB in hw/scsi-generic.c/execute_command(), but that
> the hw/scsi-generic.c/scsi_read_complete() callback is never called.
>
> This is because these are done with ioctls, and the posix async ioctl
> code is, I think, broken right now. Some more debugging, led me to
> posix-aio-compat.c/posix_aio_process_queue():
>
>            if (acb->async_context_id != async_context_id) {
>
> The async_context_ids don't match, so the request is never handled.
> This is because the acb->async_context_id field is not initialised in
> posix-aio-compat.c/paio_ioctl() (compare with
> posix-aio-compat.c/paio_submit()). The attached patch adds the missing
> line in.
>
> This seems to fix the problem. Of course, /now/ I'm getting other
> weird problems (as I'm trying to see if I can get slysoft anydvd
> working in a KVM winXP vm), but they need further investigation and
> likely other fixes.
>
> Please forgive me if I'm mistaken in this, I've only just started
> looking at the qemu code.

Hi, after further investigation, I can generate the "weird problem"
just from vanilla winxp. I'm using an lsi53c895a virtual SCSI device
BTW.

You can generate it by setting up a scsi generic cdrom, right clicking
on the cd/dvd device in winxp, click properties, hardware, select the
device from the list, and then choose Properties. Qemu will crash
instantly with the following debug from the emulated LSI code:

[snip]
lsi_scsi: Compare phase 1 == 7
lsi_scsi: Control condition failed
lsi_scsi: SCRIPTS dsp=f2022658 opcode 80880000 arg 00000414
lsi_scsi: Jump to 0xf2022a74
lsi_scsi: SCRIPTS dsp=f2022a74 opcode 58000008 arg 00000000
lsi_scsi: Set ATN
lsi_scsi: SCRIPTS dsp=f2022a7c opcode 60000040 arg 00000000
lsi_scsi: Clear ACK
lsi_scsi: SCRIPTS dsp=f2022a84 opcode 9e030000 arg 0000ff08
lsi_scsi: Compare phase 6 != 6
lsi_scsi: Control condition failed
lsi_scsi: SCRIPTS dsp=f2022a8c opcode 7c027f00 arg 00000000
lsi_scsi: Read-Modify-Write reg 0x2 AND data8=0x7f sfbr=0x00
lsi_scsi: Read reg 2
lsi_scsi: Write reg 2 = 00
lsi_scsi: SCRIPTS dsp=f2022a94 opcode 0e000001 arg f2022cc0
lsi_scsi: MSG out len=1
lsi_scsi: error: Unimplemented message 0x0c

At this point it crashes.

0xc seems to be the SCSI BUS DEVICE RESET message.. can anyone shed
any light on what is going wrong here?

[Qemu-devel] Re: [patch] fix scsi-generic

[adq]

On 7 August 2010 02:50, adq <adq@lidskialf.net> wrote:
> On 7 August 2010 01:55, adq <adq@lidskialf.net> wrote:
>> Hi, I've been tracking down why scsi generic devices (using SG_IO)
>> don't work any more. After adding debug, I can see that it actually
>> submits the scsi CDB in hw/scsi-generic.c/execute_command(), but that
>> the hw/scsi-generic.c/scsi_read_complete() callback is never called.
>>
>> This is because these are done with ioctls, and the posix async ioctl
>> code is, I think, broken right now. Some more debugging, led me to
>> posix-aio-compat.c/posix_aio_process_queue():
>>
>>            if (acb->async_context_id != async_context_id) {
>>
>> The async_context_ids don't match, so the request is never handled.
>> This is because the acb->async_context_id field is not initialised in
>> posix-aio-compat.c/paio_ioctl() (compare with
>> posix-aio-compat.c/paio_submit()). The attached patch adds the missing
>> line in.
>>
>> This seems to fix the problem. Of course, /now/ I'm getting other
>> weird problems (as I'm trying to see if I can get slysoft anydvd
>> working in a KVM winXP vm), but they need further investigation and
>> likely other fixes.
>>
>> Please forgive me if I'm mistaken in this, I've only just started
>> looking at the qemu code.
>
> Hi, after further investigation, I can generate the "weird problem"
> just from vanilla winxp. I'm using an lsi53c895a virtual SCSI device
> BTW.
>
> You can generate it by setting up a scsi generic cdrom, right clicking
> on the cd/dvd device in winxp, click properties, hardware, select the
> device from the list, and then choose Properties. Qemu will crash
> instantly with the following debug from the emulated LSI code:
>
> [snip]
> lsi_scsi: Compare phase 1 == 7
> lsi_scsi: Control condition failed
> lsi_scsi: SCRIPTS dsp=f2022658 opcode 80880000 arg 00000414
> lsi_scsi: Jump to 0xf2022a74
> lsi_scsi: SCRIPTS dsp=f2022a74 opcode 58000008 arg 00000000
> lsi_scsi: Set ATN
> lsi_scsi: SCRIPTS dsp=f2022a7c opcode 60000040 arg 00000000
> lsi_scsi: Clear ACK
> lsi_scsi: SCRIPTS dsp=f2022a84 opcode 9e030000 arg 0000ff08
> lsi_scsi: Compare phase 6 != 6
> lsi_scsi: Control condition failed
> lsi_scsi: SCRIPTS dsp=f2022a8c opcode 7c027f00 arg 00000000
> lsi_scsi: Read-Modify-Write reg 0x2 AND data8=0x7f sfbr=0x00
> lsi_scsi: Read reg 2
> lsi_scsi: Write reg 2 = 00
> lsi_scsi: SCRIPTS dsp=f2022a94 opcode 0e000001 arg f2022cc0
> lsi_scsi: MSG out len=1
> lsi_scsi: error: Unimplemented message 0x0c
>
> At this point it crashes.
>
> 0xc seems to be the SCSI BUS DEVICE RESET message.. can anyone shed
> any light on what is going wrong here?
>

Ah, just spotted that when you enable LSI_DEBUG, the BADF() macro has
an implicit exit() in it. Ok, disabling that, gives me the real cause
of the crash I'm seeing:

lsi_scsi: SCRIPTS dsp=f20229bc opcode 60000048 arg 00000000
lsi_scsi: Clear ATN ACK
lsi_scsi: SCRIPTS dsp=f20229c4 opcode 818b0000 arg fffffc4c
lsi_scsi: Compare phase 2 == 1
lsi_scsi: Control condition failed
lsi_scsi: SCRIPTS dsp=f20229cc opcode 808a0000 arg fffffc44
lsi_scsi: Compare phase 2 == 0
lsi_scsi: Control condition failed
lsi_scsi: SCRIPTS dsp=f20229d4 opcode 828a0000 arg fffffc0c
lsi_scsi: Compare phase 2 == 2
lsi_scsi: Jump to 0xf20225e8
lsi_scsi: SCRIPTS dsp=f20225e8 opcode 1a000000 arg 00000020
lsi_scsi: Send command len=524288
qemu: /root/qemu-kvm/src/qemu-kvm-0.12.5/hw/lsi53c895a.c:713:
lsi_do_command: Assertion `s->current == ((void *)0)' failed.

Re: [Qemu-devel] [patch] fix scsi-generic

[Kevin Wolf]

Am 07.08.2010 02:55, schrieb adq:
> Hi, I've been tracking down why scsi generic devices (using SG_IO)
> don't work any more. After adding debug, I can see that it actually
> submits the scsi CDB in hw/scsi-generic.c/execute_command(), but that
> the hw/scsi-generic.c/scsi_read_complete() callback is never called.
> 
> This is because these are done with ioctls, and the posix async ioctl
> code is, I think, broken right now. Some more debugging, led me to
> posix-aio-compat.c/posix_aio_process_queue():
> 
>             if (acb->async_context_id != async_context_id) {
> 
> The async_context_ids don't match, so the request is never handled.
> This is because the acb->async_context_id field is not initialised in
> posix-aio-compat.c/paio_ioctl() (compare with
> posix-aio-compat.c/paio_submit()). The attached patch adds the missing
> line in.
> 
> This seems to fix the problem. Of course, /now/ I'm getting other
> weird problems (as I'm trying to see if I can get slysoft anydvd
> working in a KVM winXP vm), but they need further investigation and
> likely other fixes.
> 
> Please forgive me if I'm mistaken in this, I've only just started
> looking at the qemu code.

The patch looks correct to me.

Please use git format-patch to generate the patch, so that it contains a
decent commit message and I can apply it with git am. Also, please don't
forget the Signed-off-by line, otherwise we can't accept it.

Kevin

Re: [Qemu-devel] [patch] fix scsi-generic

[adq]

[-- Attachment #1: Type: text/plain, Size: 1634 bytes --]

On 8 August 2010 14:11, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 07.08.2010 02:55, schrieb adq:
>> Hi, I've been tracking down why scsi generic devices (using SG_IO)
>> don't work any more. After adding debug, I can see that it actually
>> submits the scsi CDB in hw/scsi-generic.c/execute_command(), but that
>> the hw/scsi-generic.c/scsi_read_complete() callback is never called.
>>
>> This is because these are done with ioctls, and the posix async ioctl
>> code is, I think, broken right now. Some more debugging, led me to
>> posix-aio-compat.c/posix_aio_process_queue():
>>
>>             if (acb->async_context_id != async_context_id) {
>>
>> The async_context_ids don't match, so the request is never handled.
>> This is because the acb->async_context_id field is not initialised in
>> posix-aio-compat.c/paio_ioctl() (compare with
>> posix-aio-compat.c/paio_submit()). The attached patch adds the missing
>> line in.
>>
>> This seems to fix the problem. Of course, /now/ I'm getting other
>> weird problems (as I'm trying to see if I can get slysoft anydvd
>> working in a KVM winXP vm), but they need further investigation and
>> likely other fixes.
>>
>> Please forgive me if I'm mistaken in this, I've only just started
>> looking at the qemu code.
>
> The patch looks correct to me.
>
> Please use git format-patch to generate the patch, so that it contains a
> decent commit message and I can apply it with git am. Also, please don't
> forget the Signed-off-by line, otherwise we can't accept it.

Hi, please find it attached; I've not used format-patch before, hope
this is correct!

[-- Attachment #2: 0001-Set-the-async_context_id-field-when-queuing-an-async.patch --]
[-- Type: text/x-patch, Size: 831 bytes --]

From db3cfa4f12ca7e3ed300304ba8a4db15e49b4189 Mon Sep 17 00:00:00 2001
From: Andrew de Quincey <adq@lidskialf.net>
Date: Sun, 8 Aug 2010 21:04:50 +0100
Subject: [PATCH] Set the async_context_id field when queuing an async ioctl call

Signed-off-by: Andrew de Quincey <adq@lidskialf.net>
---
 posix-aio-compat.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/posix-aio-compat.c b/posix-aio-compat.c
index a67ffe3..efc5968 100644
--- a/posix-aio-compat.c
+++ b/posix-aio-compat.c
@@ -599,6 +599,7 @@ BlockDriverAIOCB *paio_ioctl(BlockDriverState *bs, int fd,
     acb->aio_type = QEMU_AIO_IOCTL;
     acb->aio_fildes = fd;
     acb->ev_signo = SIGUSR2;
+    acb->async_context_id = get_async_context_id();
     acb->aio_offset = 0;
     acb->aio_ioctl_buf = buf;
     acb->aio_ioctl_cmd = req;
-- 
1.7.2.1

Re: [Qemu-devel] [patch] fix scsi-generic

[adq]

Hi, more information on the command that is (now) killing my
qemu+scsi-generic in case someone else can help... Its our old friend
the READ DVD STRUCTURE command:

CMD: 00 ad
CMD: 01 00
CMD: 02 00
CMD: 03 00
CMD: 04 00
CMD: 05 00
CMD: 06 00
CMD: 07 01
CMD: 08 00
CMD: 09 08
CMD: 0a 00
CMD: 0b 00

What seems to happen is that it is sent to the sg device, but
lsi_command_complete() function is never called for it. Instead, I get
a message 0xc, or BUS DEVICE RESET. Since that isn't actually
implemented, everything seems to screw up after that point.

Re: [Qemu-devel] [patch] fix scsi-generic

[adq]

[-- Attachment #1: Type: text/plain, Size: 1076 bytes --]

On 8 August 2010 23:13, adq <adq@lidskialf.net> wrote:
> Hi, more information on the command that is (now) killing my
> qemu+scsi-generic in case someone else can help... Its our old friend
> the READ DVD STRUCTURE command:
>
> CMD: 00 ad
> CMD: 01 00
> CMD: 02 00
> CMD: 03 00
> CMD: 04 00
> CMD: 05 00
> CMD: 06 00
> CMD: 07 01
> CMD: 08 00
> CMD: 09 08
> CMD: 0a 00
> CMD: 0b 00
>
> What seems to happen is that it is sent to the sg device, but
> lsi_command_complete() function is never called for it. Instead, I get
> a message 0xc, or BUS DEVICE RESET. Since that isn't actually
> implemented, everything seems to screw up after that point.
>

Figured out what the problem is - READ DVD STRUCTURE has its xfer
length in an unexpected place, so hw/scsi-bus.c retrieves completely
the wrong value for the transfer length. Attached nasty hacky (!)
patch fixes it as a proof of concept, will see what I can do to clean
it up later. I'd probably want it to warn if it sees SCSI commands it
doesn't know how to explicitly handle to aid debugging this sort of
thing in future.

[-- Attachment #2: hack.patch --]
[-- Type: text/x-patch, Size: 2870 bytes --]

--- qemu-kvm-0.12.5.orig/hw/scsi-defs.h	2010-07-27 01:43:53.000000000 +0100
+++ qemu-kvm-0.12.5/hw/scsi-defs.h	2010-08-09 00:43:21.882843087 +0100
@@ -28,7 +28,7 @@
 #define TEST_UNIT_READY       0x00
 #define REZERO_UNIT           0x01
 #define REQUEST_SENSE         0x03
-#define FORMAT_UNIT           0x04
+#define FORMAT_UNIT           0x04 // adq: set xfer to 0, cmdlen 6
 #define READ_BLOCK_LIMITS     0x05
 #define REASSIGN_BLOCKS       0x07
 #define READ_6                0x08
@@ -77,23 +77,38 @@
 #define CHANGE_DEFINITION     0x40
 #define WRITE_SAME            0x41
 #define READ_TOC              0x43
+#define PLAY_AUDIO_10         0x45
+#define PLAY_AUDIO_MSF        0x47
+#define PAUSE_RESUME          0x4b
 #define LOG_SELECT            0x4c
 #define LOG_SENSE             0x4d
+#define STOP_PLAY_SCAN        0x4e
+#define RESERVE_TRACK         0x53
 #define MODE_SELECT_10        0x55
 #define RESERVE_10            0x56
 #define RELEASE_10            0x57
 #define MODE_SENSE_10         0x5a
+#define SEND_CUE_SHEET        0x5d // adq: xfer 6+7+8 -- conflict with SEND EVENT
 #define PERSISTENT_RESERVE_IN 0x5e
 #define PERSISTENT_RESERVE_OUT 0x5f
 #define MOVE_MEDIUM           0xa5
+#define SET_READ_AHEAD        0xa7
 #define READ_12               0xa8
 #define WRITE_12              0xaa
+#define GET_PERFORMANCE       0xac // adq: troublesome!
+#define READ_DVD_STRUCTURE    0xad
 #define WRITE_VERIFY_12       0xae
 #define SEARCH_HIGH_12        0xb0
 #define SEARCH_EQUAL_12       0xb1
 #define SEARCH_LOW_12         0xb2
 #define READ_ELEMENT_STATUS   0xb8
+#define SET_STREAMING         0xb6 // adq: conflict with SEND_VOLUME_TAG
 #define SEND_VOLUME_TAG       0xb6
+#define READ_CD_MSF           0xb9 // adq: troublesome
+#define SCAN                  0xba
+#define MECHANISM_STATUS      0xbd
+#define READ_CD               0xbe
+#define SEND_DVD_STRUCTURE    0xbf
 #define WRITE_LONG_2          0xea
 
 /* from hw/scsi-generic.c */
--- qemu-kvm-0.12.5.orig/hw/scsi-bus.c	2010-07-27 01:43:53.000000000 +0100
+++ qemu-kvm-0.12.5/hw/scsi-bus.c	2010-08-09 00:43:08.732844290 +0100
@@ -201,6 +201,14 @@
     case WRITE_LONG:
     case MOVE_MEDIUM:
     case UPDATE_BLOCK:
+    case PLAY_AUDIO_10:
+    case PLAY_AUDIO_MSF:
+    case PAUSE_RESUME:
+    case STOP_PLAY_SCAN:
+    case RESERVE_TRACK:
+    case FORMAT_UNIT:
+    case SET_READ_AHEAD:
+    case SCAN:
         req->cmd.xfer = 0;
         break;
     case MODE_SENSE:
@@ -243,6 +251,15 @@
     case INQUIRY:
         req->cmd.xfer = cmd[4] | (cmd[3] << 8);
         break;
+    case READ_DVD_STRUCTURE:
+    case MECHANISM_STATUS:
+    case SEND_DVD_STRUCTURE:
+        req->cmd.xfer = cmd[9] | (cmd[8] << 8);
+        break;
+    case READ_CD:
+    case SEND_CUE_SHEET:
+        req->cmd.xfer = (cmd[8] << 8) | (cmd[7] << 16) | (cmd[6] << 24);
+        break;
     }
     return 0;
 }

Re: [Qemu-devel] [patch] fix scsi-generic

[Kevin Wolf]

Am 08.08.2010 22:08, schrieb adq:
> On 8 August 2010 14:11, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 07.08.2010 02:55, schrieb adq:
>>> Hi, I've been tracking down why scsi generic devices (using SG_IO)
>>> don't work any more. After adding debug, I can see that it actually
>>> submits the scsi CDB in hw/scsi-generic.c/execute_command(), but that
>>> the hw/scsi-generic.c/scsi_read_complete() callback is never called.
>>>
>>> This is because these are done with ioctls, and the posix async ioctl
>>> code is, I think, broken right now. Some more debugging, led me to
>>> posix-aio-compat.c/posix_aio_process_queue():
>>>
>>>             if (acb->async_context_id != async_context_id) {
>>>
>>> The async_context_ids don't match, so the request is never handled.
>>> This is because the acb->async_context_id field is not initialised in
>>> posix-aio-compat.c/paio_ioctl() (compare with
>>> posix-aio-compat.c/paio_submit()). The attached patch adds the missing
>>> line in.
>>>
>>> This seems to fix the problem. Of course, /now/ I'm getting other
>>> weird problems (as I'm trying to see if I can get slysoft anydvd
>>> working in a KVM winXP vm), but they need further investigation and
>>> likely other fixes.
>>>
>>> Please forgive me if I'm mistaken in this, I've only just started
>>> looking at the qemu code.
>>
>> The patch looks correct to me.
>>
>> Please use git format-patch to generate the patch, so that it contains a
>> decent commit message and I can apply it with git am. Also, please don't
>> forget the Signed-off-by line, otherwise we can't accept it.
> 
> Hi, please find it attached; I've not used format-patch before, hope
> this is correct!

Thanks, applied to the block branch. And sorry for the delay.

Kevin

[Qemu-devel] Re: [patch] fix scsi-generic

[Paolo Bonzini]

On 08/09/2010 01:51 AM, adq wrote:
> Figured out what the problem is - READ DVD STRUCTURE has its xfer
> length in an unexpected place, so hw/scsi-bus.c retrieves completely
> the wrong value for the transfer length. Attached nasty hacky (!)
> patch fixes it as a proof of concept, will see what I can do to clean
> it up later. I'd probably want it to warn if it sees SCSI commands it
> doesn't know how to explicitly handle to aid debugging this sort of
> thing in future.

Hi Andrew, are you going to submit a similar patch in definitive form?

Paolo

[Qemu-devel] Re: [patch] fix scsi-generic

[adq]

I'll have a go at resurrecting it; I wasn't sure if there was any
interest before.

On 12 November 2010 10:00, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 08/09/2010 01:51 AM, adq wrote:
>>
>> Figured out what the problem is - READ DVD STRUCTURE has its xfer
>> length in an unexpected place, so hw/scsi-bus.c retrieves completely
>> the wrong value for the transfer length. Attached nasty hacky (!)
>> patch fixes it as a proof of concept, will see what I can do to clean
>> it up later. I'd probably want it to warn if it sees SCSI commands it
>> doesn't know how to explicitly handle to aid debugging this sort of
>> thing in future.
>
> Hi Andrew, are you going to submit a similar patch in definitive form?
>
> Paolo
>

[Qemu-devel] Re: [patch] fix scsi-generic

[adq]

> On 12 November 2010 10:00, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 08/09/2010 01:51 AM, adq wrote:
>>>
>>> Figured out what the problem is - READ DVD STRUCTURE has its xfer
>>> length in an unexpected place, so hw/scsi-bus.c retrieves completely
>>> the wrong value for the transfer length. Attached nasty hacky (!)
>>> patch fixes it as a proof of concept, will see what I can do to clean
>>> it up later. I'd probably want it to warn if it sees SCSI commands it
>>> doesn't know how to explicitly handle to aid debugging this sort of
>>> thing in future.
>>
>> Hi Andrew, are you going to submit a similar patch in definitive form?

Oops, sorry for top replying before.

Anyway, found the patch and it looks to be in good condition (just
missing one last SCSI MMC command and testing, which I shall work on).

However, could someone please check the code for the existing
SEND_VOLUME_TAG code in hw/scsi-bus.c? A doc on this command is h ere:
http://www.t10.org/ftp/t10/document.05/05-414r4.pdf

I'm not certain "req->cmd.xfer *= 40;" is correct. For a type 5 SCSI
command, req->cmd.xfer will be set to the value in bytes 9/8/7/6,
which is defined as PARAMETER LIST LENGTH (i.e. the number of bytes in
the data transfer according to SPC3). The PARAMETER LIST LENGTH for
this command should be 40, so multiplying it by 40 again ain't right
surely?

As I've never seen this command used and have no access to a device
which could generate it, I just don't know....

[Qemu-devel] Re: [patch] fix scsi-generic

[adq]

[-- Attachment #1: Type: text/plain, Size: 2166 bytes --]

On 17 November 2010 23:33, adq <adq@lidskialf.net> wrote:
>> On 12 November 2010 10:00, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> On 08/09/2010 01:51 AM, adq wrote:
>>>>
>>>> Figured out what the problem is - READ DVD STRUCTURE has its xfer
>>>> length in an unexpected place, so hw/scsi-bus.c retrieves completely
>>>> the wrong value for the transfer length. Attached nasty hacky (!)
>>>> patch fixes it as a proof of concept, will see what I can do to clean
>>>> it up later. I'd probably want it to warn if it sees SCSI commands it
>>>> doesn't know how to explicitly handle to aid debugging this sort of
>>>> thing in future.
>>>
>>> Hi Andrew, are you going to submit a similar patch in definitive form?
>
> Oops, sorry for top replying before.
>
> Anyway, found the patch and it looks to be in good condition (just
> missing one last SCSI MMC command and testing, which I shall work on).
>
> However, could someone please check the code for the existing
> SEND_VOLUME_TAG code in hw/scsi-bus.c? A doc on this command is h ere:
> http://www.t10.org/ftp/t10/document.05/05-414r4.pdf
>
> I'm not certain "req->cmd.xfer *= 40;" is correct. For a type 5 SCSI
> command, req->cmd.xfer will be set to the value in bytes 9/8/7/6,
> which is defined as PARAMETER LIST LENGTH (i.e. the number of bytes in
> the data transfer according to SPC3). The PARAMETER LIST LENGTH for
> this command should be 40, so multiplying it by 40 again ain't right
> surely?
>
> As I've never seen this command used and have no access to a device
> which could generate it, I just don't know....
>

Hi, attached is a cleaned up version of my patch against the latest
git. It adds support for all the SCSI MMC commands I could find
(although not all have been tested yet). AnyDVD is now able to rip a
video DVD.

Please let me know what you think. The SEND_VOLUME_TAG issue is still
unresolved.

Note for testers: to setup a CDROM to use with this, use a command
line something like: "-device lsi -device scsi-generic,drive=sgcd
-drive file=/dev/sg1,media=cdrom,if=none,id=sgcd" to hook in the
/dev/sgX device for your DVD drive.

Signed-off-by: Andrew de Quincey <adq@lidskialf.net>

[-- Attachment #2: qemu-scsi-mmc-1.patch --]
[-- Type: text/x-patch, Size: 7875 bytes --]

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 5a3fd4b..aac8fa1 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -220,8 +220,19 @@ static int scsi_req_length(SCSIRequest *req, uint8_t *cmd)
     case SET_CD_SPEED:
     case SET_LIMITS:
     case WRITE_LONG:
-    case MOVE_MEDIUM:
+    case MOVE_MEDIUM: // also handles MMC PLAY_AUDIO_12
     case UPDATE_BLOCK:
+    case PLAY_AUDIO_10:
+    case PLAY_AUDIO_MSF:
+    case PAUSE_RESUME:
+    case STOP_PLAY_SCAN:
+    case RESERVE_TRACK:
+    case FORMAT_UNIT:
+    case SET_READ_AHEAD:
+    case SCAN:
+    case BLANK:
+    case CLOSE_TRACK_SESSION:
+    case REPAIR_TRACK:
         req->cmd.xfer = 0;
         break;
     case MODE_SENSE:
@@ -239,7 +250,11 @@ static int scsi_req_length(SCSIRequest *req, uint8_t *cmd)
         req->cmd.xfer = 20;
         break;
     case SEND_VOLUME_TAG:
-        req->cmd.xfer *= 40;
+        if (req->dev->type == TYPE_ROM) {
+            req->cmd.xfer = cmd[10] | (cmd[9] << 8); // MMC SET_STREAMING operation
+        } else {
+            req->cmd.xfer *= 40; // FIXME: questionable
+        }
         break;
     case MEDIUM_SCAN:
         req->cmd.xfer *= 8;
@@ -271,6 +286,46 @@ static int scsi_req_length(SCSIRequest *req, uint8_t *cmd)
             req->cmd.xfer = cmd[9] | (cmd[8] << 8);
         }
         break;
+    case READ_DISC_STRUCTURE:
+    case MECHANISM_STATUS:
+    case SEND_DISC_STRUCTURE:
+        req->cmd.xfer = cmd[9] | (cmd[8] << 8);
+        break;
+    case READ_CD:
+    case SEND_CUE_SHEET:
+    case READ_MASTER_CUE:
+        req->cmd.xfer = cmd[8] | (cmd[7] << 8) | (cmd[6] << 16);
+        break;
+    case READ_CD_MSF:
+        req->cmd.xfer = (((cmd[6] - cmd[3]) * 60 * 75) + 
+                         ((cmd[7] - cmd[4]) * 75) + 
+                          (cmd[8] - cmd[5])) * 2352;
+        break;
+    case GET_PERFORMANCE:
+	req->cmd.xfer = 8;
+
+	int max_descriptors = cmd[9] | (cmd[8] << 8);
+	switch(cmd[10]) {
+	case 0:	// performance
+		req->cmd.xfer += (max_descriptors * 16);
+		break;
+	case 1:	// Unusable area data
+		req->cmd.xfer += (max_descriptors * 8);
+		break;
+	case 2:	// defect status data
+		req->cmd.xfer += (max_descriptors * 2048);
+		break;
+	case 3:	// write speed
+		req->cmd.xfer += (max_descriptors * 16);
+		break;
+	case 4:	// DBI
+		req->cmd.xfer += (max_descriptors * 8);
+		break;
+	case 5:	// DBI cache zone
+		req->cmd.xfer += (max_descriptors * 8);
+		break;
+	}
+        break;
     }
     return 0;
 }
@@ -332,20 +387,34 @@ static void scsi_req_xfer_mode(SCSIRequest *req)
     case SEARCH_LOW_12:
     case SET_WINDOW:
     case MEDIUM_SCAN:
-    case SEND_VOLUME_TAG:
+    case SEND_VOLUME_TAG: // also handles MMC SET_STREAMING
     case WRITE_LONG_2:
     case PERSISTENT_RESERVE_OUT:
-    case MAINTENANCE_OUT:
+    case SEND_DISC_STRUCTURE:
+    case SEND_CUE_SHEET:
+    case SEND_OPC_INFORMATION:
+    case SECURITY_PROTOCOL_OUT:
         req->cmd.mode = SCSI_XFER_TO_DEV;
+        return;
+    case MAINTENANCE_OUT: /* same code as MMC REPORT KEY */
+        if (req->dev->type != TYPE_ROM) {
+            req->cmd.mode = SCSI_XFER_TO_DEV;
+            return;
+        }
         break;
-    default:
-        if (req->cmd.xfer)
-            req->cmd.mode = SCSI_XFER_FROM_DEV;
-        else {
-            req->cmd.mode = SCSI_XFER_NONE;
+    case MAINTENANCE_IN: /* same code as MMC SEND KEY */
+        if (req->dev->type == TYPE_ROM) {
+            req->cmd.mode = SCSI_XFER_TO_DEV;
+            return;
         }
         break;
     }
+
+    if (req->cmd.xfer)
+        req->cmd.mode = SCSI_XFER_FROM_DEV;
+    else {
+        req->cmd.mode = SCSI_XFER_NONE;
+    }
 }
 
 static uint64_t scsi_req_lba(SCSIRequest *req)
@@ -485,6 +554,33 @@ static const char *scsi_command_name(uint8_t cmd)
         [ LOAD_UNLOAD              ] = "LOAD_UNLOAD",
         [ SET_CD_SPEED             ] = "SET_CD_SPEED",
         [ BLANK                    ] = "BLANK",
+        [ READ_FORMAT_CAPACITIES   ] = "READ_FORMAT_CAPACITIES",
+        [ PLAY_AUDIO_10            ] = "PLAY_AUDIO_10",
+        [ PLAY_AUDIO_MSF           ] = "PLAY_AUDIO_MSF",
+        [ GET_EVENT_STATUS         ] = "GET_EVENT_STATUS",
+        [ PAUSE_RESUME             ] = "PAUSE_RESUME",
+        [ STOP_PLAY_SCAN           ] = "STOP_PLAY_SCAN",
+        [ READ_DISC_INFORMATION    ] = "READ_DISC_INFORMATION",
+        [ READ_TRACK_INFORMATION   ] = "READ_TRACK_INFORMATION",
+        [ RESERVE_TRACK            ] = "RESERVE_TRACK",
+        [ SEND_OPC_INFORMATION     ] = "SEND_OPC_INFORMATION",
+        [ REPAIR_TRACK             ] = "REPAIR_TRACK",
+        [ CLOSE_TRACK_SESSION      ] = "CLOSE_TRACK_SESSION",
+        [ READ_BUFFER_CAPACITY     ] = "READ_BUFFER_CAPACITY",
+        [ SEND_CUE_SHEET           ] = "SEND_CUE_SHEET",
+        [ SECURITY_PROTOCOL_IN     ] = "SECURITY_PROTOCOL_IN",
+        [ SECURITY_PROTOCOL_OUT    ] = "SECURITY_PROTOCOL_OUT",
+        [ SET_READ_AHEAD           ] = "SET_READ_AHEAD",
+        [ READ_MEDIA_SERIAL_NUMBER ] = "READ_MEDIA_SERIAL_NUMBER",
+        [ READ_DISC_STRUCTURE      ] = "READ_DISC_STRUCTURE",
+        [ SEND_DISC_STRUCTURE      ] = "SEND_DISC_STRUCTURE",
+        [ READ_CD_MSF              ] = "READ_CD_MSF",
+        [ READ_CD                  ] = "READ_CD",
+        [ SCAN                     ] = "SCAN",
+        [ MECHANISM_STATUS         ] = "MECHANISM_STATUS",
+        [ READ_SUB_CHANNEL         ] = "READ_SUB_CHANNEL",
+        [ READ_HEADER              ] = "READ_HEADER",
+        [ READ_MASTER_CUE          ] = "READ_MASTER_CUE",
     };
 
     if (cmd >= ARRAY_SIZE(names) || names[cmd] == NULL)
diff --git a/hw/scsi-defs.h b/hw/scsi-defs.h
index a4a3518..2d56044 100644
--- a/hw/scsi-defs.h
+++ b/hw/scsi-defs.h
@@ -49,6 +49,7 @@
 #define SEND_DIAGNOSTIC       0x1d
 #define ALLOW_MEDIUM_REMOVAL  0x1e
 
+#define READ_FORMAT_CAPACITIES 0x23
 #define SET_WINDOW            0x24
 #define READ_CAPACITY         0x25
 #define READ_10               0x28
@@ -75,26 +76,53 @@
 #define WRITE_LONG            0x3f
 #define CHANGE_DEFINITION     0x40
 #define WRITE_SAME            0x41
+#define READ_SUB_CHANNEL      0x42
 #define READ_TOC              0x43
+#define READ_HEADER           0x44
+#define PLAY_AUDIO_10         0x45
+#define PLAY_AUDIO_MSF        0x47
+#define GET_EVENT_STATUS      0x4a
+#define PAUSE_RESUME          0x4b
 #define LOG_SELECT            0x4c
 #define LOG_SENSE             0x4d
+#define STOP_PLAY_SCAN        0x4e
+#define READ_DISC_INFORMATION 0x51
+#define READ_TRACK_INFORMATION 0x52
+#define RESERVE_TRACK         0x53
+#define SEND_OPC_INFORMATION  0x54
 #define MODE_SELECT_10        0x55
 #define RESERVE_10            0x56
 #define RELEASE_10            0x57
+#define REPAIR_TRACK          0x58
+#define READ_MASTER_CUE       0x59
 #define MODE_SENSE_10         0x5a
+#define CLOSE_TRACK_SESSION   0x5b
+#define READ_BUFFER_CAPACITY  0x5c
+#define SEND_CUE_SHEET        0x5d
 #define PERSISTENT_RESERVE_IN 0x5e
 #define PERSISTENT_RESERVE_OUT 0x5f
+#define SECURITY_PROTOCOL_IN  0xa2
 #define MAINTENANCE_IN        0xa3
 #define MAINTENANCE_OUT       0xa4
 #define MOVE_MEDIUM           0xa5
+#define SET_READ_AHEAD        0xa7
 #define READ_12               0xa8
 #define WRITE_12              0xaa
+#define READ_MEDIA_SERIAL_NUMBER 0xab 
+#define GET_PERFORMANCE       0xac
+#define READ_DISC_STRUCTURE   0xad
 #define WRITE_VERIFY_12       0xae
 #define SEARCH_HIGH_12        0xb0
 #define SEARCH_EQUAL_12       0xb1
 #define SEARCH_LOW_12         0xb2
+#define SECURITY_PROTOCOL_OUT 0xb5
 #define READ_ELEMENT_STATUS   0xb8
 #define SEND_VOLUME_TAG       0xb6
+#define READ_CD_MSF           0xb9
+#define SCAN                  0xba
+#define MECHANISM_STATUS      0xbd
+#define READ_CD               0xbe
+#define SEND_DISC_STRUCTURE   0xbf
 #define WRITE_LONG_2          0xea
 
 /* from hw/scsi-generic.c */