Re: [Qemu-devel] [Qemu-ppc] [PATCH v11 2/2] migration: spapr: migrate pending_events of spapr state

[Daniel Henrique Barboza]

On 05/18/2017 11:32 PM, David Gibson wrote:
> On Thu, May 18, 2017 at 05:24:02PM -0300, Daniel Henrique Barboza wrote:
> > From: Jianjun Duan <address@hidden>
> >
> > In racing situations between hotplug events and migration operation,
> > a rtas hotplug event could have not yet be delivered to the source
> > guest when migration is started. In this case the pending_events of
> > spapr state need be transmitted to the target so that the hotplug
> > event can be finished on the target.
> >
> > All the different fields of the events are encoded as defined by
> > PAPR. We can migrate them as uint8_t binary stream without any
> > concerns about data padding or endianess.
> >
> > pending_events is put in a subsection in the spapr state VMSD to make
> > sure migration across different versions is not broken.
> >
> > Signed-off-by: Jianjun Duan <address@hidden>
> > Signed-off-by: Daniel Henrique Barboza <address@hidden>
> > ---
> >   hw/ppc/spapr.c         | 32 ++++++++++++++++++++++++++++++++
> >   hw/ppc/spapr_events.c  |  1 +
> >   include/hw/ppc/spapr.h |  3 ++-
> >   3 files changed, 35 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 0980d73..5afd328 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1444,6 +1444,37 @@ static bool version_before_3(void *opaque, int 
> > version_id)
> >       return version_id < 3;
> >   }
> >   
> > +static bool spapr_pending_events_needed(void *opaque)
> > +{
> > +    sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
> > +    return !QTAILQ_EMPTY(&spapr->pending_events);
> > +}
> > +
> > +static const VMStateDescription vmstate_spapr_event_entry = {
> > +    .name = "spapr_event_log_entry",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_INT32(log_type, sPAPREventLogEntry),
> > +        VMSTATE_UINT32(data_size, sPAPREventLogEntry),
> > +        VMSTATE_VBUFFER_ALLOC_UINT32(data, sPAPREventLogEntry, 0,
> > +                                     NULL, data_size),
> > +        VMSTATE_END_OF_LIST()
> > +    },
> > +};
> > +
> > +static const VMStateDescription vmstate_spapr_pending_events = {
> > +    .name = "spapr_pending_events",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = spapr_pending_events_needed,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_QTAILQ_V(pending_events, sPAPRMachineState, 1,
> > +                         vmstate_spapr_event_entry, sPAPREventLogEntry, next),
> > +        VMSTATE_END_OF_LIST()
> > +    },
> > +};
> > +
> >   static bool spapr_ov5_cas_needed(void *opaque)
> >   {
> >       sPAPRMachineState *spapr = opaque;
> > @@ -1542,6 +1573,7 @@ static const VMStateDescription vmstate_spapr = {
> >       .subsections = (const VMStateDescription*[]) {
> >           &vmstate_spapr_ov5_cas,
> >           &vmstate_spapr_patb_entry,
> > +        &vmstate_spapr_pending_events,
> >           NULL
> >       }
> >   };
> > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> > index 73e2a18..96c1605 100644
> > --- a/hw/ppc/spapr_events.c
> > +++ b/hw/ppc/spapr_events.c
> > @@ -350,6 +350,7 @@ static void rtas_event_log_queue(int log_type, void *data)
> >       g_assert(data);
> >       entry->log_type = log_type;
> >       entry->data = data;
> > +    entry->data_size = sizeof(*data);
> This can't be right, since data is a void*.  I'm surprised it even
> compiles.  You'll need to actually look into the data buffer here and
> extract the size field.
Hehe completely forgot here that I was querying the sizeof void. Good catch.
I think it didn't throw an error because the compiler defaulted the type 
of void
in sizeof() to something else (char perhaps?).

I'll fix it by making a switch with the log_type and setting sizeof with 
the proper
structured used.


Daniel
> >       QTAILQ_INSERT_TAIL(&spapr->pending_events, entry, next);
> >   }
> >   
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 02239a5..0554e11 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -597,8 +597,9 @@ struct sPAPRTCETable {
> >   sPAPRTCETable *spapr_tce_find_by_liobn(target_ulong liobn);
> >   
> >   struct sPAPREventLogEntry {
> > -    int log_type;
> > +    int32_t log_type;
> >       void *data;
> > +    uint32_t data_size;
> >       QTAILQ_ENTRY(sPAPREventLogEntry) next;
> >   };
> >