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-ayl...@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-laur...@vivier.eu/diff/20190910193347.16000-1-laur...@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