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

[Laurent Vivier]

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