| Message ID | 20210519100803.10293-5-mark.cave-ayland@ilande.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | esp: fixes for MacOS toolbox ROM | expand |
On 19/05/21 12:08, Mark Cave-Ayland wrote: > This commit from nearly 10 years ago no longer appears to be required and in its > current form prevents the MacOS CDROM driver from detecting the CDROM drive. The > error is caused by the MacOS CDROM driver sending this CDB without DMA: > > 0x12 0x00 0x00 0x00 0x05 0x00 (INQUIRY) > > This is a valid INQUIRY command, however with this logic present the 3rd byte > (0x0) is copied over the 1st byte (0x12) which silently converts the INQUIRY > command to a TEST UNIT READY command before passing it to the QEMU SCSI layer. > Since the TEST UNIT READY command has a zero length response the MacOS CDROM > driver never receives a response and assumes the CDROM is not present. > > Resolve the issue by reverting the original commit which I'm fairly sure is now > obsolete so that the original MacOS CDB is passed unaltered to the QEMU SCSI > layer. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > hw/scsi/esp.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c > index afb4a7f9f1..a6f7c6c1bf 100644 > --- a/hw/scsi/esp.c > +++ b/hw/scsi/esp.c > @@ -260,9 +260,6 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen) > return 0; > } > n = esp_fifo_pop_buf(&s->fifo, buf, dmalen); > - if (n >= 3) { > - buf[0] = buf[2] >> 5; > - } > n = MIN(fifo8_num_free(&s->cmdfifo), n); > fifo8_push_all(&s->cmdfifo, buf, n); > } > This is probably related to S vs SATN, and the bug is that it's doing it even in the S case where cmdfifo_cdb_offset is zero. You can see that the flow is bus[0] = bus[2] >> 5; -> busid = esp_fifo_pop(&s->cmdfifo); [do_cmd] -> lun = busid & 7; [do_busid_cmd] However it does seem bogus. Perhaps the "S without ATN" cases (handle_s_without_atn, s_without_satn_pdma_cb) should do something like busid = (busid & ~7) | (buf[2] >> 5); before calling do_busid_cmd? Paolo
On 09/06/2021 13:13, Paolo Bonzini wrote: > On 19/05/21 12:08, Mark Cave-Ayland wrote: >> This commit from nearly 10 years ago no longer appears to be required and in its >> current form prevents the MacOS CDROM driver from detecting the CDROM drive. The >> error is caused by the MacOS CDROM driver sending this CDB without DMA: >> >> 0x12 0x00 0x00 0x00 0x05 0x00 (INQUIRY) >> >> This is a valid INQUIRY command, however with this logic present the 3rd byte >> (0x0) is copied over the 1st byte (0x12) which silently converts the INQUIRY >> command to a TEST UNIT READY command before passing it to the QEMU SCSI layer. >> Since the TEST UNIT READY command has a zero length response the MacOS CDROM >> driver never receives a response and assumes the CDROM is not present. >> >> Resolve the issue by reverting the original commit which I'm fairly sure is now >> obsolete so that the original MacOS CDB is passed unaltered to the QEMU SCSI >> layer. >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> hw/scsi/esp.c | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c >> index afb4a7f9f1..a6f7c6c1bf 100644 >> --- a/hw/scsi/esp.c >> +++ b/hw/scsi/esp.c >> @@ -260,9 +260,6 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen) >> return 0; >> } >> n = esp_fifo_pop_buf(&s->fifo, buf, dmalen); >> - if (n >= 3) { >> - buf[0] = buf[2] >> 5; >> - } >> n = MIN(fifo8_num_free(&s->cmdfifo), n); >> fifo8_push_all(&s->cmdfifo, buf, n); >> } >> > > This is probably related to S vs SATN, and the bug is that it's doing it even in the > S case where cmdfifo_cdb_offset is zero. You can see that the flow is > > bus[0] = bus[2] >> 5; > -> busid = esp_fifo_pop(&s->cmdfifo); [do_cmd] > -> lun = busid & 7; [do_busid_cmd] > > However it does seem bogus. > > Perhaps the "S without ATN" cases (handle_s_without_atn, s_without_satn_pdma_cb) > should do something like > > busid = (busid & ~7) | (buf[2] >> 5); > > before calling do_busid_cmd? That is entirely possible. For reference here is the sequence for the INQUIRY command with -trace 'esp*' -trace 'scsi*' without this patch applied: esp_mem_writeb reg[3]: 0x41 -> 0x01 esp_mem_writeb_cmd_flush Flush FIFO (0x01) esp_mem_writeb reg[7]: 0x00 -> 0x00 esp_mem_readb reg[12]: 0x04 esp_mem_writeb reg[12]: 0x04 -> 0x04 esp_mem_writeb reg[3]: 0x01 -> 0x44 esp_mem_writeb_cmd_ensel Enable selection (0x44) esp_mem_readb reg[4]: 0x00 esp_mem_writeb reg[4]: 0x04 -> 0x03 esp_mem_readb reg[4]: 0x00 esp_mem_writeb reg[2]: 0x00 -> 0x12 esp_mem_readb reg[4]: 0x00 esp_mem_writeb reg[2]: 0x12 -> 0x00 esp_mem_readb reg[4]: 0x00 esp_mem_writeb reg[2]: 0x00 -> 0x00 esp_mem_readb reg[4]: 0x00 esp_mem_writeb reg[2]: 0x00 -> 0x00 esp_mem_readb reg[4]: 0x00 esp_mem_writeb reg[2]: 0x00 -> 0x05 esp_mem_readb reg[4]: 0x00 esp_mem_writeb reg[2]: 0x05 -> 0x00 esp_mem_readb reg[4]: 0x00 esp_mem_writeb reg[3]: 0x44 -> 0x41 esp_mem_writeb_cmd_sel Select without ATN (0x41) esp_get_cmd len 6 target 3 esp_do_busid_cmd busid 0x0 scsi_req_parsed target 3 lun 0 tag 0 command 0 dir 0 length 0 scsi_req_parsed_lba target 3 lun 0 tag 0 command 0 lba 0 scsi_req_alloc target 3 lun 0 tag 0 scsi_disk_new_request Command: lun=0 tag=0x0 data= 0x00 0x00 0x00 0x00 0x05 0x00 scsi_test_unit_ready target 3 lun 0 tag 0 scsi_req_dequeue target 3 lun 0 tag 0 esp_command_complete SCSI Command complete According to the datasheet the differences between S and SATN are: S: sends 6, 10, or 12 byte CDB SATN: sends 1 message phase byte followed by a 6, 10 or 12 byte CDB giving a transfer total of 7, 11 or 13 bytes Due to the absence of the IDENTIFY byte in the S case I'm guessing from the patch that the LUN is in encoded in buf[1] (the top bits being "Reserved" according to my copy of the specification). The patch below passes a trivial test with MacOS - does it look right to you? If so, I'll run it around all my test images and submit a new patch if it doesn't cause any regressions. diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index bfdb94292b..2163becb52 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -260,6 +260,9 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen) return 0; } n = esp_fifo_pop_buf(&s->fifo, buf, dmalen); + //if (n >= 3) { + // buf[0] = buf[2] >> 5; + //} n = MIN(fifo8_num_free(&s->cmdfifo), n); fifo8_push_all(&s->cmdfifo, buf, n); } @@ -366,16 +369,20 @@ static void handle_satn(ESPState *s) static void s_without_satn_pdma_cb(ESPState *s) { + uint8_t busid; + if (!esp_get_tc(s) && !fifo8_is_empty(&s->cmdfifo)) { s->cmdfifo_cdb_offset = 0; s->do_cmd = 0; - do_busid_cmd(s, 0); + busid = s->cmdfifo.data[1] >> 5; + do_busid_cmd(s, busid); } } static void handle_s_without_atn(ESPState *s) { int32_t cmdlen; + uint8_t busid; if (s->dma && !s->dma_enabled) { s->dma_cb = handle_s_without_atn; @@ -386,7 +393,8 @@ static void handle_s_without_atn(ESPState *s) if (cmdlen > 0) { s->cmdfifo_cdb_offset = 0; s->do_cmd = 0; - do_busid_cmd(s, 0); + busid = s->cmdfifo.data[1] >> 5; + do_busid_cmd(s, busid); } else if (cmdlen == 0) { s->do_cmd = 1; /* Target present, but no cmd yet - switch to command phase */ ATB, Mark.
On 09/06/21 17:31, Mark Cave-Ayland wrote: > Due to the absence of the IDENTIFY byte in the S case I'm guessing from > the patch that the LUN is in encoded in buf[1] (the top bits being > "Reserved" according to my copy of the specification). They used to be the LUN many years ago. > static void s_without_satn_pdma_cb(ESPState *s) > { > + uint8_t busid; > + > if (!esp_get_tc(s) && !fifo8_is_empty(&s->cmdfifo)) { > s->cmdfifo_cdb_offset = 0; > s->do_cmd = 0; > - do_busid_cmd(s, 0); > + busid = s->cmdfifo.data[1] >> 5; > + do_busid_cmd(s, busid); > } > } Considering how obsolete the LUN-in-CDB case is (and now that I actually understand that the first byte is a message-phase byte), it is probably be more correct to keep the previous busid: with no message phase, the previously-selected LUN would be addressed. I can send a patch for this, but it's orthogonal to this one so I queued it as well. Paolo
On 11/06/2021 12:38, Paolo Bonzini wrote: > On 09/06/21 17:31, Mark Cave-Ayland wrote: >> Due to the absence of the IDENTIFY byte in the S case I'm guessing from the patch >> that the LUN is in encoded in buf[1] (the top bits being "Reserved" according to my >> copy of the specification). > > They used to be the LUN many years ago. Got it :) >> static void s_without_satn_pdma_cb(ESPState *s) >> { >> + uint8_t busid; >> + >> if (!esp_get_tc(s) && !fifo8_is_empty(&s->cmdfifo)) { >> s->cmdfifo_cdb_offset = 0; >> s->do_cmd = 0; >> - do_busid_cmd(s, 0); >> + busid = s->cmdfifo.data[1] >> 5; >> + do_busid_cmd(s, busid); >> } >> } > > Considering how obsolete the LUN-in-CDB case is (and now that I actually understand > that the first byte is a message-phase byte), it is probably be more correct to keep > the previous busid: with no message phase, the previously-selected LUN would be > addressed. I can send a patch for this, but it's orthogonal to this one so I queued > it as well. That was going to be my next question - how do you determine the LUN if there is no message phase byte? But that makes sense to me. I don't think that I have a LUN-in-CDB example in my set of ESP test images, but I'm happy to run them against your patch and make sure there are no regressions. ATB, Mark.
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index afb4a7f9f1..a6f7c6c1bf 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -260,9 +260,6 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen) return 0; } n = esp_fifo_pop_buf(&s->fifo, buf, dmalen); - if (n >= 3) { - buf[0] = buf[2] >> 5; - } n = MIN(fifo8_num_free(&s->cmdfifo), n); fifo8_push_all(&s->cmdfifo, buf, n); }
This commit from nearly 10 years ago no longer appears to be required and in its current form prevents the MacOS CDROM driver from detecting the CDROM drive. The error is caused by the MacOS CDROM driver sending this CDB without DMA: 0x12 0x00 0x00 0x00 0x05 0x00 (INQUIRY) This is a valid INQUIRY command, however with this logic present the 3rd byte (0x0) is copied over the 1st byte (0x12) which silently converts the INQUIRY command to a TEST UNIT READY command before passing it to the QEMU SCSI layer. Since the TEST UNIT READY command has a zero length response the MacOS CDROM driver never receives a response and assumes the CDROM is not present. Resolve the issue by reverting the original commit which I'm fairly sure is now obsolete so that the original MacOS CDB is passed unaltered to the QEMU SCSI layer. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- hw/scsi/esp.c | 3 --- 1 file changed, 3 deletions(-)