Re: [Qemu-devel] [PATCH 1/2] esp-pci: Fix status register write erase control

[Paolo Bonzini]

On 28/11/18 22:56, Guenter Roeck wrote:
> Per AM53C974 datasheet, definition of "SCSI Bus and Control (SBAC)"
> register:
> 
> Bit 24 – STATUS – Write Erase Control
> 
> This bit controls the Write Erase feature on bits 3:1 and bit 6 of the DMA
> Status Register ((B)+54h). When this bit is programmed to ‘1’, the state
> of bits 3:1 are preserved when read. Bits 3:1 are only cleared when a ‘1’
> is written to the corresponding bit location. For example, to clear bit 1,
> the value of ‘0000_0010b’ should be written to the register. When the DMA
> Status Preserve bit is ‘0’, bits 3:1 are cleared when read.
> 
> The status register is currently defined to bit 12, not bit 24.
> Also, its implementation is reversed: The status is auto-cleared if
> the bit is set to 1, and must be cleared explicitly when the bit is
> set to 0. This results in spurious interrupts reported by the Linux
> kernel, and in some cases even results in stalled SCSI operations.
> 
> Set SBAC_STATUS to bit 24 and reverse the logic to fix the problem.
> 
> Signed-off-by: Guenter Roeck <address@hidden>
> ---
>  hw/scsi/esp-pci.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
> index 419fc66..d956909 100644
> --- a/hw/scsi/esp-pci.c
> +++ b/hw/scsi/esp-pci.c
> @@ -59,7 +59,7 @@
>  #define DMA_STAT_SCSIINT 0x10
>  #define DMA_STAT_BCMBLT  0x20
>  
> -#define SBAC_STATUS 0x1000
> +#define SBAC_STATUS (1 << 24)
>  
>  typedef struct PCIESPState {
>      /*< private >*/
> @@ -136,7 +136,7 @@ static void esp_pci_dma_write(PCIESPState *pci, uint32_t 
> saddr, uint32_t val)
>          pci->dma_regs[saddr] = val;
>          break;
>      case DMA_STAT:
> -        if (!(pci->sbac & SBAC_STATUS)) {
> +        if (pci->sbac & SBAC_STATUS) {
>              /* clear some bits on write */
>              uint32_t mask = DMA_STAT_ERROR | DMA_STAT_ABORT | DMA_STAT_DONE;
>              pci->dma_regs[DMA_STAT] &= ~(val & mask);
> @@ -157,7 +157,7 @@ static uint32_t esp_pci_dma_read(PCIESPState *pci, 
> uint32_t saddr)
>          if (pci->esp.rregs[ESP_RSTAT] & STAT_INT) {
>              val |= DMA_STAT_SCSIINT;
>          }
> -        if (pci->sbac & SBAC_STATUS) {
> +        if (!(pci->sbac & SBAC_STATUS)) {
>              pci->dma_regs[DMA_STAT] &= ~(DMA_STAT_ERROR | DMA_STAT_ABORT |
>                                           DMA_STAT_DONE);
>          }
> 

Queued this one only, for now at least.

Paolo