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 <stefa...@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 <n...@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;