Re: [Qemu-devel] [PATCH v3] lsi_scsi: Reselection needed to remove pending commands from queue

[Paolo Bonzini]

On 31/10/2018 12:29, George Kennedy wrote:
>>>               case 2: /* Wait Reselect */
>>> +                if (!s->resel_dsp) {
>>> +                    s->resel_dsp = s->dsp - 8;
>>> +                }
>>
>> What is the reason for the "if"?
>
> The save of the Rselection Script address only needs to be done once

That's not correct.  There could be more than one WAIT RESELECT instruction.

(I'm also not sure why the "- 8", which causes the WAIT RESELECT opcode
to run again.  I think that, instead of that "- 8", you're missing a
call to lsi_wait_reselect where you set s->dsp = s->resel_dsp.  It makes
sense: after disconnecting from one target, you immediately fake a
reselect from the target.  This isn't quite right, the reselection
should be in lsi_transfer_data, but it's a start).

>>>                   if (!lsi_irq_on_rsl(s)) {
>>> +                    if (save_dsp) {
>>> +                        s->dsp = save_dsp;
>>> +                        save_dsp = 0;
>>> +                    }
>>>                       lsi_wait_reselect(s);
>>>                   }
>>>                   break;
>>> @@ -1476,6 +1501,10 @@ again:
>>>                 if (insn & (1 << 28)) {
>>>                   addr = s->dsa + sextract32(addr, 0, 24);
>>> +            } else if (s->want_resel && s->resel_dsp && !(insn & (1
>>> << 24))) {
>>> +                save_dsp = s->dsp;
>>> +                s->dsp = s->resel_dsp;
>>> +                s->want_resel = false;
>>>               }
>> Can you explain why this is placed under the LOAD AND STORE instruction?
>>
>> Your previous patch has, more specifically, a store from the scratch
>> register into memory.  In the Linux scripts this occurs twice, both
>> after a DISCONNECT instruction.  Is that the place where you want to
>> hook?
>
> Yes. I can revert to what was in the previous patch. I had figured that,
> like the change to capture the Reselection Script address, the point
> where the DSP address gets loaded with the Script Reselection address
> could be handled in a similar manner.

There is only one correct way to handle SCRIPTS instructions, and it's
the one that matches the data sheet.  In other words, even if we agree
to an incomplete solution it still must have some foundation in the
datasheet and parallel SCSI spec.  We have to adjust for the coalescing
of initiator/target/scripts that occurs in lsi53c895a.c, but that should
be it.

In particular, the reselection script is captured in WAIT RESELECT
because the data sheet says so:

  If the LSI53C895A is reselected, it fetches the next
  instruction from the address pointed to by the DMA
  SCRIPTS Pointer (DSP) register.

but LOAD AND STORE has nothing to do with reselection.

Paolo