[RFC] lsi53c895a: Update dnad when skipping MSGOUT bytes

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 <stefanha@linux.vnet.ibm.com>
---
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

 hw/lsi53c895a.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

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 <stefanha@linux.vnet.ibm.com>
> ---
> 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 <nab@linux-iscsi.org>

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;

On Sun, Jan 9, 2011 at 11:19 PM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
> 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 <nab@linux-iscsi.org>

Thanks for the review, I'll resend as [PATCH] and will try to find a
Windows 2003 Server guest for testing.

Stefan