[Qemu-devel] [PATCH] LSI53C895A: Do not update current_dma_len with dbc in TIA mode

[Qemu-devel] [PATCH] LSI53C895A: Do not update current_dma_len with dbc in TIA mode

[Justin Chevrier]

Continued testing has shown that even with the update to handle variable length Inquiry commands in scsi-disk.c Openserver still relies on DMA length being updated with the dbc later on. The below patch modifies the current behaviour to update the DMA length with the dbc only when in Direct and Indirect mode. In Table Indirect Access mode the dma length does not come from the dbc, so we don't update it there.

This fixes the Debian Arm target, still works in Openserver and should correct the Windows install issue reported.

Justin

Changelog:

Do not update current_dma_len with the dbc if we are in Table Indirect Access mode.

Signed-off-by: Justin Chevrier <address@hidden>

--- hw/lsi53c895a.c     (revision 5799)
+++ hw/lsi53c895a.c     (working copy)
@@ -920,7 +920,9 @@
             break;
         case PHASE_DI:
             s->waiting = 2;
-            s->current_dma_len = s->dbc;
+           /* Update DMA length in Direct and Indirect modes only */
+           if (!(insn & (1 << 28)))
+                s->current_dma_len = s->dbc;
             lsi_do_dma(s, 0);
             if (s->waiting)
                 s->waiting = 3;



      

Re: [Qemu-devel] [PATCH] LSI53C895A: Do not update current_dma_len with dbc in TIA mode

[Ryan Harper]

* Justin Chevrier <theburner1@yahoo.com> [2008-11-26 09:37]:
> Continued testing has shown that even with the update to handle
> variable length Inquiry commands in scsi-disk.c Openserver still
> relies on DMA length being updated with the dbc later on. The below
> patch modifies the current behaviour to update the DMA length with the
> dbc only when in Direct and Indirect mode. In Table Indirect Access
> mode the dma length does not come from the dbc, so we don't update it
> there.

Even in Table indirect access, we update dbc after fetching the transfer
count (TC) from the table.  The tech manual is unclear about whether or
not dbc is updated:

    "For a MOVE instruction, the 24-bit byte count is fetched
    from system memory. Then the 32-bit physical address is
    brought into the LSI53C895A. Execution of the move
    begins at this point" - LSI53C895A Tech manual, page 230.

However, I think in practice, it would have to do so.

> 
> This fixes the Debian Arm target, still works in Openserver and should
> correct the Windows install issue reported.
> 
> Justin
> 
> Changelog:
> 
> Do not update current_dma_len with the dbc if we are in Table Indirect Access mode.
> 
> Signed-off-by: Justin Chevrier <address@hidden>
>
> --- hw/lsi53c895a.c     (revision 5799)
> +++ hw/lsi53c895a.c     (working copy)
> @@ -920,7 +920,9 @@
>              break;
>          case PHASE_DI:
>              s->waiting = 2;
> -            s->current_dma_len = s->dbc;
> +           /* Update DMA length in Direct and Indirect modes only */
> +           if (!(insn & (1 << 28)))
> +                s->current_dma_len = s->dbc;
>              lsi_do_dma(s, 0);
>              if (s->waiting)
>                  s->waiting = 3;
> 

Nack.  With the variable length Inquiry changes you posted and this
patch, sp3 32-bit fails to install same as before.


-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com

Re: [Qemu-devel] [PATCH] LSI53C895A: Do not update current_dma_len with dbc in TIA mode

[Justin Chevrier]

> Even in Table indirect access, we update dbc after fetching
> the transfer
> count (TC) from the table.  The tech manual is unclear
> about whether or
> not dbc is updated:
> 
>     "For a MOVE instruction, the 24-bit byte count is
> fetched
>     from system memory. Then the 32-bit physical address is
>     brought into the LSI53C895A. Execution of the move
>     begins at this point" - LSI53C895A Tech manual,
> page 230.
> 
> However, I think in practice, it would have to do so.
> 

It is true that we update the dbc with the length acquired from the table. Unfortunately it seems the length acquired from the table is incorrect. Here are snippets of log from the Debian Arm target with current head.

We get a command complete with data ready, length 69632. current_dma_len is updated with this value:

lsi_scsi: MSG IN 0x80
lsi_scsi: MSG IN 0x20
lsi_scsi: MSG IN 0x07
lsi_scsi: Data ready tag=0x10007 len=69632

Then when we issue the TIA block move command the length at the table entry referenced is wrong (at 4096). The DBC gets updated with this new incorrect length, which then overwrites dma_current_length and we only transfer 4096 bytes.
lsi_scsi: SCRIPTS dsp=50000780 opcode 11000000 arg 000002e8
lsi_scsi: DMA addr=0x0000c000 len=4096
lsi_scsi: Command complete sense=0

With the patch sent today and how it behaved before my previous patch the code continues to use an unaltered current_dma_len (which stays at 69632).

It seems that the DSA + offset is pointing to the wrong table entry. I'm looking into this now.

I just wanted to get this patch out to fix breakages introduced by my original patch.

Justin


      

Re: [Qemu-devel] [PATCH] LSI53C895A: Do not update current_dma_len with dbc in TIA mode

[Ryan Harper]

* Justin Chevrier <theburner1@yahoo.com> [2008-11-26 12:08]:
> > Even in Table indirect access, we update dbc after fetching
> > the transfer
> > count (TC) from the table.  The tech manual is unclear
> > about whether or
> > not dbc is updated:
> > 
> >     "For a MOVE instruction, the 24-bit byte count is
> > fetched
> >     from system memory. Then the 32-bit physical address is
> >     brought into the LSI53C895A. Execution of the move
> >     begins at this point" - LSI53C895A Tech manual,
> > page 230.
> > 
> > However, I think in practice, it would have to do so.
> > 
> 
> It is true that we update the dbc with the length acquired from the
> table. Unfortunately it seems the length acquired from the table is
> incorrect. Here are snippets of log from the Debian Arm target with
> current head.
> 
> We get a command complete with data ready, length 69632.
> current_dma_len is updated with this value:
> 
> lsi_scsi: MSG IN 0x80
> lsi_scsi: MSG IN 0x20
> lsi_scsi: MSG IN 0x07
> lsi_scsi: Data ready tag=0x10007 len=69632

Do you also have SCSI debug on in scsi-disk.c ?  I'd really like to see
the scsi command that was generating the read with lengh 69632.

Actually, have you tried this since the 40 bit DMA patch was included?

Also, dumping the value in ccntl1 is very useful to understand what
block mode the guest is trying to use.

> 
> Then when we issue the TIA block move command the length at the table
> entry referenced is wrong (at 4096). The DBC gets updated with this
> new incorrect length, which then overwrites dma_current_length and we
> only transfer 4096 bytes.

We should be able to determine if 4096 is the correct len given the scsi
command that is triggering the DMA.

>
> lsi_scsi: SCRIPTS dsp=50000780 opcode 11000000 arg 000002e8
> lsi_scsi: DMA addr=0x0000c000 len=4096
> lsi_scsi: Command complete sense=0
> 
> With the patch sent today and how it behaved before my previous patch
> the code continues to use an unaltered current_dma_len (which stays at
> 69632).
> 
> It seems that the DSA + offset is pointing to the wrong table entry.
> I'm looking into this now.
> 
> I just wanted to get this patch out to fix breakages introduced by my
> original patch.

Yeah, I think for now, the right thing to do is revert the old patch
until we figure how to handle this correctly for both test cases.

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com

Re: [Qemu-devel] [PATCH] LSI53C895A: Do not update current_dma_len with dbc in TIA mode

[Justin Chevrier]

Ryan Harper wrote:
> Do you also have SCSI debug on in scsi-disk.c ?  I'd
> really like to see
> the scsi command that was generating the read with lengh
> 69632.
> 
> Actually, have you tried this since the 40 bit DMA patch
> was included?

I'm running head.

The command is a Read:

scsi-disk: Command: lun=0 tag=0x1003b data=0x28 0x00 0x00 0x5a 0x81 0x57 0x00 0x00 0x88 0x00
scsi-disk: Read (sector 5931351, count 136)
scsi-disk: Read sector_count=136

The 69632 is coming from 136 * 512

> Yeah, I think for now, the right thing to do is revert the
> old patch
> until we figure how to handle this correctly for both test
> cases.

This reverts the relevant section of the original patch:

--- hw/lsi53c895a.c     (revision 5799)
+++ hw/lsi53c895a.c     (working copy)
@@ -920,7 +920,6 @@
             break;
         case PHASE_DI:
             s->waiting = 2;
-            s->current_dma_len = s->dbc;
             lsi_do_dma(s, 0);
             if (s->waiting)
                 s->waiting = 3;