Re: [Qemu-devel] [PATCH 2/2] scsi: esp: Improve consistency of RSTAT, RSEQ, and RINTR

[Guenter Roeck]

On 11/29/18 1:58 AM, Paolo Bonzini wrote:
> On 28/11/18 22:56, Guenter Roeck wrote:
> > The guest OS reads RSTAT, RSEQ, and RINTR, and expects those registers
> > to reflect a consistent state. However, it is possible that the registers
> > can change after RSTAT was read, but before RINTR is read.
> >
> > Guest OS                qemu
> > --------                ----
> > Read RSTAT
> >                         esp_command_complete()
> >                          RSTAT = STAT_ST
> >                          esp_dma_done()
> >                           RSTAT |= STAT_TC
> >                           RSEQ = 0
> >                           RINTR = INTR_BS
> >
> > Read RSEQ
> > Read RINTR              RINTR = 0
> >                         RSTAT &= ~STAT_TC
> >                         RSEQ = SEQ_CD
> >
> > The guest OS would then try to handle INTR_BS combined with an old
> > value of RSTAT. This sometimes resulted in lost events, spurious
> > interrupts, guest OS confusion, and stalled SCSI operations.
>
> The question is, why was the guest running the interrupt routine before
> STAT_INT was set in RSTAT?  The code in esp_raise_irq seems good:

It doesn't. It polls for interrupts, but only enters the interrupt handler
if one was observed. It uses the DMA status (see first patch) for the polling,
presumably to not disturb RSTAT. Qemu produces two interrupts in a row,
the second while the first one is still handled.

Also, it works fine with a real controller (yes, I did buy one, and an
old SCSI drive, to test).

Guenter

>      if (!(s->rregs[ESP_RSTAT] & STAT_INT)) {
>          s->rregs[ESP_RSTAT] |= STAT_INT;
>          qemu_irq_raise(s->irq);
>          trace_esp_raise_irq();
>      }
>
> Paolo
>
> > A typical guest error log (observed with various versions of Linux)
> > looks as follows.
> >
> > scsi host1: Spurious irq, sreg=13.
> > ...
> > scsi host1: Aborting command [84531f10:2a]
> > scsi host1: Current command [f882eea8:35]
> > scsi host1: Queued command [84531f10:2a]
> > scsi host1:  Active command [f882eea8:35]
> > scsi host1: Dumping command log
> > scsi host1: ent[15] CMD val[44] sreg[90] seqreg[00] sreg2[00] ireg[20] ss[00] 
> > event[0c]
> > scsi host1: ent[16] CMD val[01] sreg[90] seqreg[00] sreg2[00] ireg[20] ss[02] 
> > event[0c]
> > scsi host1: ent[17] CMD val[43] sreg[90] seqreg[00] sreg2[00] ireg[20] ss[02] 
> > event[0c]
> > scsi host1: ent[18] EVENT val[0d] sreg[92] seqreg[04] sreg2[00] ireg[18] ss[00] 
> > event[0c]
> > ...