Re: [PATCH v2 18/42] esp: accumulate SCSI commands for PDMA transfers in cmdbuf instead of pdma_buf

[Mark Cave-Ayland]

On 02/03/2021 17:02, Laurent Vivier wrote:

> Le 09/02/2021 à 20:29, Mark Cave-Ayland a écrit :
> > ESP SCSI commands are already accumulated in cmdbuf and so there is no need to
> > keep a separate pdma_buf buffer. Accumulate SCSI commands for PDMA transfers in
> > cmdbuf instead of pdma_buf so update cmdlen accordingly and change pdma_origin
> > for PDMA transfers to CMD which allows the PDMA origin to be removed.
> >
> > This commit also removes a stray memcpy() from get_cmd() which is a no-op 
> > because
> > cmdlen is always zero at the start of a command.
> >
> > Notionally the removal of pdma_buf from vmstate_esp_pdma also breaks migration
> > compatibility for the PDMA subsection until its complete removal by the end of
> > the series.
> >
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > ---
> >   hw/scsi/esp.c         | 56 +++++++++++++++++++------------------------
> >   include/hw/scsi/esp.h |  2 --
> >   2 files changed, 25 insertions(+), 33 deletions(-)
> >
> > diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> > index 7134c0aff4..b846f022fb 100644
> > --- a/hw/scsi/esp.c
> > +++ b/hw/scsi/esp.c
> > @@ -139,8 +139,6 @@ static void set_pdma(ESPState *s, enum pdma_origin_id 
> > origin,
> >   static uint8_t *get_pdma_buf(ESPState *s)
> >   {
> >       switch (s->pdma_origin) {
> > -    case PDMA:
> > -        return s->pdma_buf;
> >       case TI:
> >           return s->ti_buf;
> >       case CMD:
> > @@ -161,14 +159,12 @@ static uint8_t esp_pdma_read(ESPState *s)
> >       }
> >   
> >       switch (s->pdma_origin) {
> > -    case PDMA:
> > -        val = s->pdma_buf[s->pdma_cur++];
> > -        break;
> >       case TI:
> >           val = s->ti_buf[s->pdma_cur++];
> >           break;
> >       case CMD:
> > -        val = s->cmdbuf[s->pdma_cur++];
> > +        val = s->cmdbuf[s->cmdlen++];
> > +        s->pdma_cur++;
> >           break;
> >       case ASYNC:
> >           val = s->async_buf[s->pdma_cur++];
> > @@ -193,14 +189,12 @@ static void esp_pdma_write(ESPState *s, uint8_t val)
> >       }
> >   
> >       switch (s->pdma_origin) {
> > -    case PDMA:
> > -        s->pdma_buf[s->pdma_cur++] = val;
> > -        break;
> >       case TI:
> >           s->ti_buf[s->pdma_cur++] = val;
> >           break;
> >       case CMD:
> > -        s->cmdbuf[s->pdma_cur++] = val;
> > +        s->cmdbuf[s->cmdlen++] = val;
> > +        s->pdma_cur++;
> >           break;
> >       case ASYNC:
> >           s->async_buf[s->pdma_cur++] = val;
> > @@ -256,8 +250,7 @@ static uint32_t get_cmd(ESPState *s, uint8_t *buf, uint8_t 
> > buflen)
> >           if (s->dma_memory_read) {
> >               s->dma_memory_read(s->dma_opaque, buf, dmalen);
> >           } else {
> > -            memcpy(s->pdma_buf, buf, dmalen);
> > -            set_pdma(s, PDMA, 0, dmalen);
> > +            set_pdma(s, CMD, 0, dmalen);
> >               esp_raise_drq(s);
> >               return 0;
> >           }
> > @@ -316,24 +309,24 @@ static void satn_pdma_cb(ESPState *s)
> >       if (get_cmd_cb(s) < 0) {
> >           return;
> >       }
> > -    if (s->pdma_cur != s->pdma_start) {
> > -        do_cmd(s, get_pdma_buf(s) + s->pdma_start);
> > +    s->do_cmd = 0;
> > +    if (s->cmdlen) {
> > +        do_cmd(s, s->cmdbuf);
>
> I don't understant how you can remove the pdma_start: normally it is here to 
> keep track of the
> position in the pDMA if the migration is occuraing while a pDMA transfer.

Hi Laurent,

I was going to follow up on your reviews later this evening, however this one caught 
my eye: as per the cover letter, this patchset is a migration break for the q800 
machine. As there are likely more incompatible changes for the q800 machine coming up 
soon, it didn't seem worth the effort (and indeed I don't think it's possible to 
recreate the new internal state with 100% accuracy from the old state).

Migration for ESP devices that don't use PDMA is still supported, and I've tested 
this during development with qemu-system-sparc.


ATB,

Mark.