Re: [PATCH 3/4] esp-pci.c: synchronise setting of DMA_STAT_DONE with ESP completion interrupt

[Guenter Roeck]

On Fri, Jan 12, 2024 at 01:15:28PM +0000, Mark Cave-Ayland wrote:
> The setting of DMA_STAT_DONE at the end of a DMA transfer can be configured to
> generate an interrupt, however the Linux driver manually checks for 
> DMA_STAT_DONE
> being set and if it is, considers that a DMA transfer has completed.
> 
> If DMA_STAT_DONE is set but the ESP device isn't indicating an interrupt then
> the Linux driver considers this to be a spurious interrupt. However this can
> occur in QEMU as there is a delay between the end of DMA transfer where
> DMA_STAT_DONE is set, and the ESP device raising its completion interrupt.
> 
> This appears to be an incorrect assumption in the Linux driver as the ESP and
> PCI DMA interrupt sources are separate (and may not be raised exactly
> together), however we can work around this by synchronising the setting of
> DMA_STAT_DONE at the end of a DMA transfer with the ESP completion interrupt.
> 
> In conjunction with the previous commit Linux is now able to correctly boot
> from an am53c974 PCI SCSI device on the hppa C3700 machine without emitting
> "iget: checksum invalid" and "Spurious irq, sreg=10" errors.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Tested-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  hw/scsi/esp-pci.c | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
> index 15dc3c004d..875a49199d 100644
> --- a/hw/scsi/esp-pci.c
> +++ b/hw/scsi/esp-pci.c
> @@ -93,6 +93,18 @@ static void esp_irq_handler(void *opaque, int irq_num, int 
> level)
>  
>      if (level) {
>          pci->dma_regs[DMA_STAT] |= DMA_STAT_SCSIINT;
> +
> +        /*
> +         * If raising the ESP IRQ to indicate end of DMA transfer, set
> +         * DMA_STAT_DONE at the same time. In theory this should be done in
> +         * esp_pci_dma_memory_rw(), however there is a delay between setting
> +         * DMA_STAT_DONE and the ESP IRQ arriving which is visible to the
> +         * guest that can cause confusion e.g. Linux
> +         */
> +        if ((pci->dma_regs[DMA_CMD] & DMA_CMD_MASK) == 0x3 &&
> +            pci->dma_regs[DMA_WBC] == 0) {
> +                pci->dma_regs[DMA_STAT] |= DMA_STAT_DONE;
> +        }
>      } else {
>          pci->dma_regs[DMA_STAT] &= ~DMA_STAT_SCSIINT;
>      }
> @@ -306,9 +318,6 @@ static void esp_pci_dma_memory_rw(PCIESPState *pci, 
> uint8_t *buf, int len,
>      /* update status registers */
>      pci->dma_regs[DMA_WBC] -= len;
>      pci->dma_regs[DMA_WAC] += len;
> -    if (pci->dma_regs[DMA_WBC] == 0) {
> -        pci->dma_regs[DMA_STAT] |= DMA_STAT_DONE;
> -    }
>  }
>  
>  static void esp_pci_dma_memory_read(void *opaque, uint8_t *buf, int len)
> @@ -363,24 +372,13 @@ static const VMStateDescription vmstate_esp_pci_scsi = {
>      }
>  };
>  
> -static void esp_pci_command_complete(SCSIRequest *req, size_t resid)
> -{
> -    ESPState *s = req->hba_private;
> -    PCIESPState *pci = container_of(s, PCIESPState, esp);
> -
> -    esp_command_complete(req, resid);
> -    pci->dma_regs[DMA_WBC] = 0;
> -    pci->dma_regs[DMA_STAT] |= DMA_STAT_DONE;
> -    esp_pci_update_irq(pci);
> -}
> -
>  static const struct SCSIBusInfo esp_pci_scsi_info = {
>      .tcq = false,
>      .max_target = ESP_MAX_DEVS,
>      .max_lun = 7,
>  
>      .transfer_data = esp_transfer_data,
> -    .complete = esp_pci_command_complete,
> +    .complete = esp_command_complete,
>      .cancel = esp_request_cancelled,
>  };
>  
> -- 
> 2.39.2
>