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

From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
To: Laurent Vivier <laurent@vivier.eu>,
	qemu-devel@nongnu.org, pbonzini@redhat.com, fam@euphon.net
Subject: Re: [PATCH v2 18/42] esp: accumulate SCSI commands for PDMA transfers in cmdbuf instead of pdma_buf
Date: Tue, 2 Mar 2021 19:29:13 +0000	[thread overview]
Message-ID: <15ceb143-d902-b415-3f5d-48999840a560@ilande.co.uk> (raw)
In-Reply-To: <68838f99-f2fa-5eb3-683c-85b39b155ab4@vivier.eu>

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.