Re: [PATCH 4/5] esp: revert 75ef849696 "esp: correctly fill bus id with requested lun"

[Paolo Bonzini <pbonzini@redhat.com>]

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