Re: [PATCH v2 09/10] esp: include the current PDMA callback in the migration stream

[Mark Cave-Ayland]

On 03/03/2022 15:40, Peter Maydell wrote:

> On Wed, 2 Mar 2022 at 21:38, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
> > This involves (re)adding a PDMA-specific subsection to hold the reference to the
> > current PDMA callback.
> >
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > ---
> >   hw/scsi/esp.c | 23 +++++++++++++++++++++++
> >   1 file changed, 23 insertions(+)
> >
> > diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> > index a818b2b07a..32926834bc 100644
> > --- a/hw/scsi/esp.c
> > +++ b/hw/scsi/esp.c
> > @@ -1209,6 +1209,25 @@ static int esp_post_load(void *opaque, int version_id)
> >       return 0;
> >   }
> >
> > +static bool esp_pdma_needed(void *opaque)
> > +{
> > +    ESPState *s = ESP(opaque);
> > +
> > +    return s->dma_memory_read == NULL && s->dma_memory_write == NULL &&
> > +           s->dma_enabled;
>
> A comment about why this is the correct condition would be helpful.
> If I understand it correctly, something like this ?
>
>   /*
>    * pdma_cb is used only by the sysbus ESP device, not the PCI ESP
>    * device. The PCI device sets the s->dma_memory_read and
>    * s->dma_memory_write function pointers at realize.
>    */

Even more specifically, PDMA is only used by the Macintosh which is detected by not 
having DMA memory access functions (because the movement between the SCSI bus and the 
CPU is done by the guest) yet DMA is enabled.

> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> > +}
> > +
> > +static const VMStateDescription vmstate_esp_pdma = {
> > +    .name = "esp/pdma",
> > +    .version_id = 0,
> > +    .minimum_version_id = 0,
> > +    .needed = esp_pdma_needed,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT8(pdma_cb, ESPState),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >   const VMStateDescription vmstate_esp = {
> >       .name = "esp",
> >       .version_id = 6,
> > @@ -1243,6 +1262,10 @@ const VMStateDescription vmstate_esp = {
> >           VMSTATE_UINT8_TEST(lun, ESPState, esp_is_version_6),
> >           VMSTATE_END_OF_LIST()
> >       },
> > +    .subsections = (const VMStateDescription * []) {
> > +        &vmstate_esp_pdma,
> > +        NULL
> > +    }
> >   };
>
> Do we need to do something similar to handle s->dma_cb ?

I don't believe so. From IRC my understanding was that for normal DMA where the SCSI 
data is copied directly to memory, if migration is requested the iothread will run to 
completion first which finishes the SCSIRequest before migration starts. It is only 
in the PDMA case where the guest OS has to do the data movement that a reference to 
the current callback is required to allow the SCSIRequest to continue post-migration.


ATB,

Mark.