[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC][PATCH] lsi53c895a: Update dnad when skipping MSGO
From: |
Nicholas A. Bellinger |
Subject: |
Re: [Qemu-devel] [RFC][PATCH] lsi53c895a: Update dnad when skipping MSGOUT bytes |
Date: |
Sun, 09 Jan 2011 15:19:20 -0800 |
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;