| Message ID | 20210209193018.31339-19-mark.cave-ayland@ilande.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | esp: consolidate PDMA transfer buffers and other fixes | expand |
On 2/9/21 8:29 PM, Mark Cave-Ayland wrote: > 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(-) Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
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. Thanks, Laurent
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.
Le 02/03/2021 à 18:34, Mark Cave-Ayland a écrit : > 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. > I don't mean we can't migrate from a previous version to the new one, I mean the migration between two machines of the current version is not possible anymore as we don't keep track of the position of the pDMA index inside the buffer. With a DMA, the migration cannot happen in the middle of the DMA, while with pDMA it can (as it's a processor loop). The whole purpose of get_pdma() and set_pdma() was for the migration. https://patchew.org/QEMU/20190910113323.17324-1-laurent@vivier.eu/diff/20190910193347.16000-1-laurent@vivier.eu/ Previously the Q800 was not migratable also because the CPU was not migratable, but I added recently the VMSTATE for the CPU. Thanks, Laurent
On 02/03/2021 17:59, Laurent Vivier wrote: > Le 02/03/2021 à 18:34, Mark Cave-Ayland a écrit : >> 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. >> > > I don't mean we can't migrate from a previous version to the new one, I mean the migration between > two machines of the current version is not possible anymore as we don't keep track of the position > of the pDMA index inside the buffer. > > With a DMA, the migration cannot happen in the middle of the DMA, while with pDMA it can (as it's a > processor loop). > > The whole purpose of get_pdma() and set_pdma() was for the migration. > > https://patchew.org/QEMU/20190910113323.17324-1-laurent@vivier.eu/diff/20190910193347.16000-1-laurent@vivier.eu/ > > Previously the Q800 was not migratable also because the CPU was not migratable, but I added recently > the VMSTATE for the CPU. What should happen here is that the PDMA bytes for the message out and command phases are accumulated in cmdbuf and cmdlen as per normal ESP DMA - these are already included in the migration stream so there should be no problem there. The PDMA callbacks are invoked when pdma_len == 0 where pdma_len is initially set to len in esp_do_dma: this is effectively the TC which is set to the length of the CDB for a DMA transfer. This means that the PDMA callback and hence do_cmd() are only called at the end of the transfer once the entire CDB has been accumulated where pdma_start is 0 (cmdbuf always includes the preceding IDENTIFY byte). ATB, Mark.
Le 02/03/2021 à 20:29, Mark Cave-Ayland a écrit : > On 02/03/2021 17:59, Laurent Vivier wrote: > >> Le 02/03/2021 à 18:34, Mark Cave-Ayland a écrit : >>> 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. >>> >> >> I don't mean we can't migrate from a previous version to the new one, I mean the migration between >> two machines of the current version is not possible anymore as we don't keep track of the position >> of the pDMA index inside the buffer. >> >> With a DMA, the migration cannot happen in the middle of the DMA, while with pDMA it can (as it's a >> processor loop). >> >> The whole purpose of get_pdma() and set_pdma() was for the migration. >> >> https://patchew.org/QEMU/20190910113323.17324-1-laurent@vivier.eu/diff/20190910193347.16000-1-laurent@vivier.eu/ >> >> >> Previously the Q800 was not migratable also because the CPU was not migratable, but I added recently >> the VMSTATE for the CPU. > > What should happen here is that the PDMA bytes for the message out and command phases are > accumulated in cmdbuf and cmdlen as per normal ESP DMA - these are already included in the migration > stream so there should be no problem there. > > The PDMA callbacks are invoked when pdma_len == 0 where pdma_len is initially set to len in > esp_do_dma: this is effectively the TC which is set to the length of the CDB for a DMA transfer. > This means that the PDMA callback and hence do_cmd() are only called at the end of the transfer once > the entire CDB has been accumulated where pdma_start is 0 (cmdbuf always includes the preceding > IDENTIFY byte). > OK, I think I was worried about the migration of the value of pdma_cur, used in sysbus_esp_pdma_read()/sysbus_esp_pdma_write(), but it seems you take care of that later in the series... Thanks, Laurent
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); > } > } > > static void handle_satn(ESPState *s) > { > - uint8_t buf[32]; > - int len; > - > if (s->dma && !s->dma_enabled) { > s->dma_cb = handle_satn; > return; > } > s->pdma_cb = satn_pdma_cb; > - len = get_cmd(s, buf, sizeof(buf)); > - if (len) { > - do_cmd(s, buf); > + s->cmdlen = get_cmd(s, s->cmdbuf, sizeof(s->cmdbuf)); > + if (s->cmdlen) { > + do_cmd(s, s->cmdbuf); > + } else { > + s->do_cmd = 1; > } > } > > @@ -342,24 +335,24 @@ static void s_without_satn_pdma_cb(ESPState *s) > if (get_cmd_cb(s) < 0) { > return; > } > - if (s->pdma_cur != s->pdma_start) { > + s->do_cmd = 0; > + if (s->cmdlen) { > do_busid_cmd(s, get_pdma_buf(s) + s->pdma_start, 0); > } > } > > static void handle_s_without_atn(ESPState *s) > { > - uint8_t buf[32]; > - int len; > - > if (s->dma && !s->dma_enabled) { > s->dma_cb = handle_s_without_atn; > return; > } > s->pdma_cb = s_without_satn_pdma_cb; > - len = get_cmd(s, buf, sizeof(buf)); > - if (len) { > - do_busid_cmd(s, buf, 0); > + s->cmdlen = get_cmd(s, s->cmdbuf, sizeof(s->cmdbuf)); > + if (s->cmdlen) { > + do_busid_cmd(s, s->cmdbuf, 0); > + } else { > + s->do_cmd = 1; > } > } > > @@ -368,7 +361,7 @@ static void satn_stop_pdma_cb(ESPState *s) > if (get_cmd_cb(s) < 0) { > return; > } > - s->cmdlen = s->pdma_cur - s->pdma_start; > + s->do_cmd = 0; > if (s->cmdlen) { > trace_esp_handle_satn_stop(s->cmdlen); > s->do_cmd = 1; > @@ -394,6 +387,8 @@ static void handle_satn_stop(ESPState *s) > s->rregs[ESP_RINTR] = INTR_BS | INTR_FC; > s->rregs[ESP_RSEQ] = SEQ_CD; > esp_raise_irq(s); > + } else { > + s->do_cmd = 1; > } > } > > @@ -865,11 +860,10 @@ static bool esp_pdma_needed(void *opaque) > > static const VMStateDescription vmstate_esp_pdma = { > .name = "esp/pdma", > - .version_id = 1, > - .minimum_version_id = 1, > + .version_id = 2, > + .minimum_version_id = 2, > .needed = esp_pdma_needed, > .fields = (VMStateField[]) { > - VMSTATE_BUFFER(pdma_buf, ESPState), > VMSTATE_INT32(pdma_origin, ESPState), > VMSTATE_UINT32(pdma_len, ESPState), > VMSTATE_UINT32(pdma_start, ESPState), > diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h > index 9fad320513..c323d43f70 100644 > --- a/include/hw/scsi/esp.h > +++ b/include/hw/scsi/esp.h > @@ -16,7 +16,6 @@ typedef void (*ESPDMAMemoryReadWriteFunc)(void *opaque, uint8_t *buf, int len); > typedef struct ESPState ESPState; > > enum pdma_origin_id { > - PDMA, > TI, > CMD, > ASYNC, > @@ -57,7 +56,6 @@ struct ESPState { > ESPDMAMemoryReadWriteFunc dma_memory_write; > void *dma_opaque; > void (*dma_cb)(ESPState *s); > - uint8_t pdma_buf[32]; > int pdma_origin; > uint32_t pdma_len; > uint32_t pdma_start; > Reviewed-by: Laurent Vivier <laurent@vivier.eu>
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); } } static void handle_satn(ESPState *s) { - uint8_t buf[32]; - int len; - if (s->dma && !s->dma_enabled) { s->dma_cb = handle_satn; return; } s->pdma_cb = satn_pdma_cb; - len = get_cmd(s, buf, sizeof(buf)); - if (len) { - do_cmd(s, buf); + s->cmdlen = get_cmd(s, s->cmdbuf, sizeof(s->cmdbuf)); + if (s->cmdlen) { + do_cmd(s, s->cmdbuf); + } else { + s->do_cmd = 1; } } @@ -342,24 +335,24 @@ static void s_without_satn_pdma_cb(ESPState *s) if (get_cmd_cb(s) < 0) { return; } - if (s->pdma_cur != s->pdma_start) { + s->do_cmd = 0; + if (s->cmdlen) { do_busid_cmd(s, get_pdma_buf(s) + s->pdma_start, 0); } } static void handle_s_without_atn(ESPState *s) { - uint8_t buf[32]; - int len; - if (s->dma && !s->dma_enabled) { s->dma_cb = handle_s_without_atn; return; } s->pdma_cb = s_without_satn_pdma_cb; - len = get_cmd(s, buf, sizeof(buf)); - if (len) { - do_busid_cmd(s, buf, 0); + s->cmdlen = get_cmd(s, s->cmdbuf, sizeof(s->cmdbuf)); + if (s->cmdlen) { + do_busid_cmd(s, s->cmdbuf, 0); + } else { + s->do_cmd = 1; } } @@ -368,7 +361,7 @@ static void satn_stop_pdma_cb(ESPState *s) if (get_cmd_cb(s) < 0) { return; } - s->cmdlen = s->pdma_cur - s->pdma_start; + s->do_cmd = 0; if (s->cmdlen) { trace_esp_handle_satn_stop(s->cmdlen); s->do_cmd = 1; @@ -394,6 +387,8 @@ static void handle_satn_stop(ESPState *s) s->rregs[ESP_RINTR] = INTR_BS | INTR_FC; s->rregs[ESP_RSEQ] = SEQ_CD; esp_raise_irq(s); + } else { + s->do_cmd = 1; } } @@ -865,11 +860,10 @@ static bool esp_pdma_needed(void *opaque) static const VMStateDescription vmstate_esp_pdma = { .name = "esp/pdma", - .version_id = 1, - .minimum_version_id = 1, + .version_id = 2, + .minimum_version_id = 2, .needed = esp_pdma_needed, .fields = (VMStateField[]) { - VMSTATE_BUFFER(pdma_buf, ESPState), VMSTATE_INT32(pdma_origin, ESPState), VMSTATE_UINT32(pdma_len, ESPState), VMSTATE_UINT32(pdma_start, ESPState), diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h index 9fad320513..c323d43f70 100644 --- a/include/hw/scsi/esp.h +++ b/include/hw/scsi/esp.h @@ -16,7 +16,6 @@ typedef void (*ESPDMAMemoryReadWriteFunc)(void *opaque, uint8_t *buf, int len); typedef struct ESPState ESPState; enum pdma_origin_id { - PDMA, TI, CMD, ASYNC, @@ -57,7 +56,6 @@ struct ESPState { ESPDMAMemoryReadWriteFunc dma_memory_write; void *dma_opaque; void (*dma_cb)(ESPState *s); - uint8_t pdma_buf[32]; int pdma_origin; uint32_t pdma_len; uint32_t pdma_start;
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(-)