| Message ID | 20240112131529.515642-4-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | esp-pci: fixes for Linux and MS-DOS | expand |
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 >
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, };
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> --- hw/scsi/esp-pci.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-)