| Message ID | 1320940872-4940-2-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 10.11.2011 17:01, schrieb Paolo Bonzini: > Mode page 2A of emulated ATAPI DVD-ROM should have page length 0x14 > like SCSI CD-ROM, rather than 0x12. > > Mode page length is off by 8, as it should contain the length of the > payload after the first two bytes. > > MODE SENSE(6) should be thrown out of ATAPI DVD-ROM emulation. It is > not specified in the ATAPI list of MMC-2, and MMC-5 prescribes to use > MODE SENSE(10). Anyway, its implementation is wrong. > > Reported-by: Thomas Schmitt <scdbackup@gmx.net> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/ide/atapi.c | 21 ++++++++------------- > 1 files changed, 8 insertions(+), 13 deletions(-) > > diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c > index d4179a0..cf0e66b 100644 > --- a/hw/ide/atapi.c > +++ b/hw/ide/atapi.c > @@ -689,12 +689,7 @@ static void cmd_mode_sense(IDEState *s, uint8_t *buf) > int action, code; > int max_len; > > - if (buf[0] == GPCMD_MODE_SENSE_10) { > - max_len = ube16_to_cpu(buf + 7); > - } else { > - max_len = buf[4]; > - } > - > + max_len = ube16_to_cpu(buf + 7); > action = buf[2] >> 6; > code = buf[2] & 0x3f; > > @@ -702,7 +697,7 @@ static void cmd_mode_sense(IDEState *s, uint8_t *buf) > case 0: /* current values */ > switch(code) { > case MODE_PAGE_R_W_ERROR: /* error recovery */ > - cpu_to_ube16(&buf[0], 16 + 6); > + cpu_to_ube16(&buf[0], 16 - 2); > buf[2] = 0x70; > buf[3] = 0; > buf[4] = 0; > @@ -717,11 +712,10 @@ static void cmd_mode_sense(IDEState *s, uint8_t *buf) > buf[12] = 0x00; > buf[13] = 0x00; > buf[14] = 0x00; > - buf[15] = 0x00; Why did you drop this? It still seems to be part of the buffer. Kevin
On 11/11/2011 02:36 PM, Kevin Wolf wrote: >> > @@ -717,11 +712,10 @@ static void cmd_mode_sense(IDEState *s, uint8_t *buf) >> > buf[12] = 0x00; >> > buf[13] = 0x00; >> > buf[14] = 0x00; >> > - buf[15] = 0x00; > Why did you drop this? It still seems to be part of the buffer. Ouch. No idea. Actually, I think it's best if these patches wait until Thomas can give a shot at testing them. If that means missing 1.0, so be it. Paolo
Hi, Paolo Bonzini wrote: > > > case MODE_PAGE_R_W_ERROR: /* error recovery */ > > > [...] > > > - buf[15] = 0x00; Kevin Wolf wrote: > > Why did you drop this? It still seems to be part of the buffer. Paolo Bonzini wrote: > Actually, I think it's best if these patches wait until Thomas can give a > shot at testing them. If that means missing 1.0, so be it. libburn does not use mode page 1 "Read/Write Error Recovery Parameters". So can only judge by theory and not by test. MMC-1 says it has 8 bytes (beginning at buf[8] = MODE_PAGE_R_W_ERROR). MMC-2 says it has 12. MMC-6 says it has 12. So buf[15] = 0x00 matches MMC-1 and the announcement made by buf[9] = 16 - 10; (6 is the number of bytes after buf[9]). I would advise to keep buf[15] = 0x00. Have a nice day :) Thomas
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c index d4179a0..cf0e66b 100644 --- a/hw/ide/atapi.c +++ b/hw/ide/atapi.c @@ -689,12 +689,7 @@ static void cmd_mode_sense(IDEState *s, uint8_t *buf) int action, code; int max_len; - if (buf[0] == GPCMD_MODE_SENSE_10) { - max_len = ube16_to_cpu(buf + 7); - } else { - max_len = buf[4]; - } - + max_len = ube16_to_cpu(buf + 7); action = buf[2] >> 6; code = buf[2] & 0x3f; @@ -702,7 +697,7 @@ static void cmd_mode_sense(IDEState *s, uint8_t *buf) case 0: /* current values */ switch(code) { case MODE_PAGE_R_W_ERROR: /* error recovery */ - cpu_to_ube16(&buf[0], 16 + 6); + cpu_to_ube16(&buf[0], 16 - 2); buf[2] = 0x70; buf[3] = 0; buf[4] = 0; @@ -717,11 +712,10 @@ static void cmd_mode_sense(IDEState *s, uint8_t *buf) buf[12] = 0x00; buf[13] = 0x00; buf[14] = 0x00; - buf[15] = 0x00; ide_atapi_cmd_reply(s, 16, max_len); break; case MODE_PAGE_AUDIO_CTL: - cpu_to_ube16(&buf[0], 24 + 6); + cpu_to_ube16(&buf[0], 24 - 2); buf[2] = 0x70; buf[3] = 0; buf[4] = 0; @@ -740,7 +734,7 @@ static void cmd_mode_sense(IDEState *s, uint8_t *buf) ide_atapi_cmd_reply(s, 24, max_len); break; case MODE_PAGE_CAPABILITIES: - cpu_to_ube16(&buf[0], 28 + 6); + cpu_to_ube16(&buf[0], 30 - 2); buf[2] = 0x70; buf[3] = 0; buf[4] = 0; @@ -749,7 +743,7 @@ static void cmd_mode_sense(IDEState *s, uint8_t *buf) buf[7] = 0; buf[8] = MODE_PAGE_CAPABILITIES; - buf[9] = 28 - 10; + buf[9] = 30 - 10; buf[10] = 0x3b; /* read CDR/CDRW/DVDROM/DVDR/DVDRAM */ buf[11] = 0x00; @@ -771,7 +765,9 @@ static void cmd_mode_sense(IDEState *s, uint8_t *buf) buf[25] = 0; buf[26] = 0; buf[27] = 0; - ide_atapi_cmd_reply(s, 28, max_len); + buf[28] = 0; + buf[29] = 0; + ide_atapi_cmd_reply(s, 30, max_len); break; default: goto error_cmd; @@ -1037,7 +1033,6 @@ static const struct { [ 0x00 ] = { cmd_test_unit_ready, CHECK_READY }, [ 0x03 ] = { cmd_request_sense, ALLOW_UA }, [ 0x12 ] = { cmd_inquiry, ALLOW_UA }, - [ 0x1a ] = { cmd_mode_sense, /* (6) */ 0 }, [ 0x1b ] = { cmd_start_stop_unit, 0 }, /* [1] */ [ 0x1e ] = { cmd_prevent_allow_medium_removal, 0 }, [ 0x25 ] = { cmd_read_cdvd_capacity, CHECK_READY },
Mode page 2A of emulated ATAPI DVD-ROM should have page length 0x14 like SCSI CD-ROM, rather than 0x12. Mode page length is off by 8, as it should contain the length of the payload after the first two bytes. MODE SENSE(6) should be thrown out of ATAPI DVD-ROM emulation. It is not specified in the ATAPI list of MMC-2, and MMC-5 prescribes to use MODE SENSE(10). Anyway, its implementation is wrong. Reported-by: Thomas Schmitt <scdbackup@gmx.net> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/ide/atapi.c | 21 ++++++++------------- 1 files changed, 8 insertions(+), 13 deletions(-)