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

[George Kennedy <george.kennedy@oracle.com>]

On 10/23/2018 5:50 PM, Paolo Bonzini wrote:
> On 23/10/2018 23:36, George Kennedy wrote:
>>
>> On 10/23/2018 10:33 AM, Paolo Bonzini wrote:
>>> On 22/10/2018 23:28, George Kennedy wrote:
>>>> As you suggested I moved the loading of "s->resel_dsp" down to the
>>>> "Wait Reselect"
>>>> case. The address of the Reselection Scripts, though, is contained in
>>>> "s->dsp - 8"
>>>> and not in s->dnad.
>>> Are you sure?  s->dsp - 8 should be the address of the Wait Reselect
>>> instruction itself.  But you're right that s->dnad is the address at
>>> which to jump "if the LSI53C895A is selected before being reselected"
>>> (as the spec puts it) so the reselection DSP should be just s->dsp.
>> See within the 1st 25 lines of lsi_execute_script() where dsp is bumped
>> up by 8, "s->dsp += 8", so it needs to be adjusted back to what it was.
> The spec says "If the LSI53C895A is reselected, it fetches the next
> instruction from the address pointed to by the DMA
> SCRIPTS Pointer (DSP) register".  The first instruction of the
> reselection scripts is the one after WAIT RESELECT, i.e. s->dsp.
>
>
>>> Reselection should only happen when the target needs access to the bus,
>>> which is when I/O has finished.  There should be no need for such a
>>> deadline; reselection should already be happening at the right time when
>>> lsi_transfer_data calls lsi_queue_req, which in turn calls lsi_reselect.
>> Agree that it should happen as you describe, but under heavy IO (fio),
>> it does not.
>>
>> When it works as expected the check for "s->waiting == 1" (Wait Reselect
>> instruction has been issued) in lsi_transfer_data() is true. Under heavy
>> IO, s->waiting is not "1" for an extended period of time
> What about "req->hba_private != s->current"?  That should cause a call
> to lsi_queue_req, and then you can check s->want_resel in lsi_queue_req.

For the extended period of time where lsi_queue_req() is not being 
called from lsi_transfer_data(), my debug shows "s->waiting" is not "1" 
and req->hba_private is equal to s->current.

req->hba_private is set to NULL in lsi_command_complete() and that's 
where I tried to add a call to lsi_reselect(), but the Scripts are not 
in the correct state to allow the call.

lsi_transfer_data() or lsi_command_complete() are probably the 2 
potential places where a fix could be added if the Script state would 
allow it.
>
>> I am not strongly attached to my proposed fix. If an alternative fix can
>> be suggested, I'd be more than willing to try that.
> The problem is that the timeout has no explanation under the SCSI
> protocol, so I would like to understand where the logic is wrong in the
> Parallel SCSI emulation.

Agreed. That's why I'm hoping for an alternate fix.

Thank you,
George
>
> Paolo