Re: [Qemu-devel] [RFC][PATCH] lsi53c895a: Update dnad when skipping MSGOUT bytes

[Nicholas A. Bellinger]

On Sat, 2011-01-08 at 20:20 +0000, Stefan Hajnoczi wrote:
> Update not only dbc but also dnad when skipping bytes during the MSGOUT
> phase.  Previously only dbc was updated which is probably wrong and
> could lead to bogus message codes being read.
> 
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
> I don't know the LSI SCSI code well but it seems odd that only dbc is updated
> but the actual address isn't bumped when skipping bytes.  Unfortunately I
> cannot test this because I don't know how to trigger SDTR/WDTR extended
> messages.  Any ideas?
> 
> Came across this issue while looking into the following bug report:
> https://bugs.launchpad.net/qemu/+bug/697510
> 

Hi Stefan and Paul,

After taking a look at this patch with v0.12.5 into Linux guest with the
modern sym53c8xx_2 driver, I think incrementing DNAD is indeed the
proper fix to handle ignored extended MSG outs..  The Linux driver will
automatically generate SDTRs and WDTRs using scsi_transport_spi.c code
during initial negotiation, and upon each INQUIRY and REQUEST sense
according to the comment above sym_prepare_nego() in sym_queue_scsiio()
here:

http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=blob;f=drivers/scsi/sym53c8xx_2/sym_hipd.c;hb=HEAD#l5216

Looking deeper with hw/lsi53c895a.c:LSI_DEBUG=1 using a TCM_Loop virtual
SCSI LUN, the following sized message OUTs build in Linux SPI transport
spi_populate_[sync,width]_msg() appear as:

<SNIP>

lsi_scsi: MSG out len=8
lsi_scsi: Select LUN 0
lsi_scsi: SIMPLE queue tag=0x1
lsi_scsi: Extended message 0x1 (len 3)
lsi_scsi: SDTR (ignored)

and after a handful of attempts at the same MSG + Extended message
length for the initial SDTRs, many more WDTRs attempts are made and
(still) ignored by lsi53c895a.c until sym53c8xx_2 gives up and moves
forward with it's internal defaults.. (I think..? Matthew CC'ed for good
measure ;)

<SNIP>

lsi_scsi: MSG out len=7
lsi_scsi: Select LUN 0
lsi_scsi: SIMPLE queue tag=0x15
lsi_scsi: Extended message 0x3 (len 2)
lsi_scsi: WDTR (ignored)

It's also worth mentioning that the sym53c8xx driver still works without
this patch, which may be attributed to the Win2003 driver perhaps
either:

*) Sending contiguous 'Extended Messages' instead of individual messages
(as sym53c8xx_2 appears to do) to cause the sanity check in
lsi_add_msg_byte() and trigger the BUG

*) Sending a different/wrong sized MSG out or Extended message length
for SDTR / WDTR negoitation messages

In any event, I think your change looks good and thanks for tracking
this one down.  Please add my:

Reviewed-by: Nicholas A. Bellinger <address@hidden>

Thanks!

>  hw/lsi53c895a.c |   11 +++++++++--
>  1 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
> index 0129ae3..c73f60a 100644
> --- a/hw/lsi53c895a.c
> +++ b/hw/lsi53c895a.c
> @@ -842,6 +842,13 @@ static uint8_t lsi_get_msgbyte(LSIState *s)
>      return data;
>  }
>  
> +/* Skip the next n bytes during a MSGOUT phase. */
> +static void lsi_skip_msgbytes(LSIState *s, unsigned int n)
> +{
> +    s->dnad += n;
> +    s->dbc  -= n;
> +}
> +
>  static void lsi_do_msgout(LSIState *s)
>  {
>      uint8_t msg;
> @@ -869,11 +876,11 @@ static void lsi_do_msgout(LSIState *s)
>              switch (msg) {
>              case 1:
>                  DPRINTF("SDTR (ignored)\n");
> -                s->dbc -= 2;
> +                lsi_skip_msgbytes(s, 2);
>                  break;
>              case 3:
>                  DPRINTF("WDTR (ignored)\n");
> -                s->dbc -= 1;
> +                lsi_skip_msgbytes(s, 1);
>                  break;
>              default:
>                  goto bad;