Re: [PATCH v2 33/42] esp: defer command completion interrupt on incoming data transfers

[Laurent Vivier]

Le 18/02/2021 à 18:25, Mark Cave-Ayland a écrit :
> On 09/02/2021 19:30, Mark Cave-Ayland wrote:
> 
>> The MacOS toolbox ROM issues a command to the ESP controller as part of its
>> "FAST" SCSI routines and then proceeds to read the incoming data soon after
>> receiving the command completion interrupt.
>>
>> Unfortunately due to SCSI block transfers being asynchronous the incoming 
>> data
>> may not yet be present causing an underflow error. Resolve this by waiting 
>> for
>> the SCSI subsystem transfer_data callback before raising the command 
>> completion
>> interrupt.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/scsi/esp.c         | 54 +++++++++++++++++++++++++++++++++++++++----
>>   include/hw/scsi/esp.h |  1 +
>>   2 files changed, 51 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>> index 728d4ddf99..ce6a7a1ed0 100644
>> --- a/hw/scsi/esp.c
>> +++ b/hw/scsi/esp.c
>> @@ -183,6 +183,14 @@ static int esp_select(ESPState *s)
>>           esp_raise_irq(s);
>>           return -1;
>>       }
>> +
>> +    /*
>> +     * Note that we deliberately don't raise the IRQ here: this will be done
>> +     * either in do_busid_cmd() for DATA OUT transfers or by the deferred
>> +     * IRQ mechanism in esp_transfer_data() for DATA IN transfers
>> +     */
>> +    s->rregs[ESP_RINTR] |= INTR_FC;
>> +    s->rregs[ESP_RSEQ] = SEQ_CD;
>>       return 0;
>>   }
>>   @@ -237,18 +245,24 @@ static void do_busid_cmd(ESPState *s, uint8_t *buf, 
>> uint8_t busid)
>>       s->ti_size = datalen;
>>       if (datalen != 0) {
>>           s->rregs[ESP_RSTAT] = STAT_TC;
>> +        s->rregs[ESP_RSEQ] = SEQ_CD;
>>           esp_set_tc(s, 0);
>>           if (datalen > 0) {
>> +            /*
>> +             * Switch to DATA IN phase but wait until initial data xfer is
>> +             * complete before raising the command completion interrupt
>> +             */
>> +            s->data_in_ready = false;
>>               s->rregs[ESP_RSTAT] |= STAT_DI;
>>           } else {
>>               s->rregs[ESP_RSTAT] |= STAT_DO;
>> +            s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
>> +            esp_raise_irq(s);
>> +            esp_lower_drq(s);
>>           }
>>           scsi_req_continue(s->current_req);
>> +        return;
>>       }
>> -    s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
>> -    s->rregs[ESP_RSEQ] = SEQ_CD;
>> -    esp_raise_irq(s);
>> -    esp_lower_drq(s);
>>   }
>>     static void do_cmd(ESPState *s)
>> @@ -603,12 +617,35 @@ void esp_command_complete(SCSIRequest *req, uint32_t 
>> status,
>>   void esp_transfer_data(SCSIRequest *req, uint32_t len)
>>   {
>>       ESPState *s = req->hba_private;
>> +    int to_device = ((s->rregs[ESP_RSTAT] & 7) == STAT_DO);
>>       uint32_t dmalen = esp_get_tc(s);
>>         assert(!s->do_cmd);
>>       trace_esp_transfer_data(dmalen, s->ti_size);
>>       s->async_len = len;
>>       s->async_buf = scsi_req_get_buf(req);
>> +
>> +    if (!to_device && !s->data_in_ready) {
>> +        /*
>> +         * Initial incoming data xfer is complete so raise command
>> +         * completion interrupt
>> +         */
>> +        s->data_in_ready = true;
>> +        s->rregs[ESP_RSTAT] |= STAT_TC;
>> +        s->rregs[ESP_RINTR] |= INTR_BS;
>> +        esp_raise_irq(s);
>> +
>> +        /*
>> +         * If data is ready to transfer and the TI command has already
>> +         * been executed, start DMA immediately. Otherwise DMA will start
>> +         * when host sends the TI command
>> +         */
>> +        if (s->ti_size && (s->rregs[ESP_CMD] == (CMD_TI | CMD_DMA))) {
>> +            esp_do_dma(s);
>> +        }
>> +        return;
>> +    }
>> +
>>       if (dmalen) {
>>           esp_do_dma(s);
>>       } else if (s->ti_size <= 0) {
>> @@ -870,6 +907,14 @@ static bool esp_is_before_version_5(void *opaque, int 
>> version_id)
>>       return version_id < 5;
>>   }
>>   +static bool esp_is_version_5(void *opaque, int version_id)
>> +{
>> +    ESPState *s = ESP(opaque);
>> +
>> +    version_id = MIN(version_id, s->mig_version_id);
>> +    return version_id == 5;
>> +}
>> +
>>   static int esp_pre_save(void *opaque)
>>   {
>>       ESPState *s = ESP(opaque);
>> @@ -914,6 +959,7 @@ const VMStateDescription vmstate_esp = {
>>           VMSTATE_UINT32(cmdlen, ESPState),
>>           VMSTATE_UINT32(do_cmd, ESPState),
>>           VMSTATE_UINT32_TEST(mig_dma_left, ESPState, 
>> esp_is_before_version_5),
>> +        VMSTATE_BOOL_TEST(data_in_ready, ESPState, esp_is_version_5),
>>           VMSTATE_END_OF_LIST()
>>       },
>>   };
>> diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
>> index 6618f4e091..3b69aedebe 100644
>> --- a/include/hw/scsi/esp.h
>> +++ b/include/hw/scsi/esp.h
>> @@ -41,6 +41,7 @@ struct ESPState {
>>       uint32_t cmdlen;
>>       uint32_t do_cmd;
>>   +    bool data_in_ready;
>>       int dma_enabled;
>>         uint32_t async_len;
> 
> Whilst doing some testing earlier, I discovered that the same change is 
> required in
> do_dma_pdma_cb(): it seems during boot the ROM attempts several 128k DMA 
> requests in a row, and with
> heavy debugging enabled it's enough to trigger the same underflow problem.
> 
> Fortunately the fix is easy, and I'll squash this into the v3 series:
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index a175191718..248c1ce27a 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -453,17 +453,13 @@ static void do_dma_pdma_cb(ESPState *s)
>      } else {
>          if (s->async_len == 0) {
>              if (s->current_req) {
> +               /*
> +                * Defer until the scsi layer has completed.
> +                */
>                  scsi_req_continue(s->current_req);
> +                s->data_in_ready = false;
>              }
> -
> -            /*
> -             * If there is still data to be read from the device then
> -             * complete the DMA operation immediately.  Otherwise defer
> -             * until the scsi layer has completed.
> -             */
> -            if (esp_get_tc(s) != 0 || s->ti_size == 0) {
> -                return;
> -            }
> +            return;
>          }
> 
>          if (esp_get_tc(s) != 0) {
> 
> 
> ATB,
> 
> Mark.


Reviewed-by: Laurent Vivier <laurent@vivier.eu>