Re: [Qemu-devel] [Qemu-ppc] [PATCH v10] migration: spapr: migrate pending_events of spapr state

[Daniel Henrique Barboza]

On 05/16/2017 09:04 AM, Daniel Henrique Barboza wrote:
> On 05/16/2017 01:25 AM, David Gibson wrote:
> > On Mon, May 15, 2017 at 10:10:52AM -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>
> > Ok, thanks for splitting this out, but you don't seem to have
> > addressed the other comments I had on this patch as presented before.
>
> Sorry, I haven't noticed you had previous comments on this patch. I'll 
> address
> them and re-send.
>
>
> Daniel
>
> > > ---
> > >   hw/ppc/spapr.c         | 33 +++++++++++++++++++++++++++++++++
> > >   hw/ppc/spapr_events.c  | 24 +++++++++++++-----------
> > >   include/hw/ppc/spapr.h |  3 ++-
> > >   3 files changed, 48 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 80d12d0..8cfdc71 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1437,6 +1437,38 @@ 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_BOOL(exception, sPAPREventLogEntry),
> > I'd like some more information to convince me there isn't redundant
> > data here.
I'll quote David's v9 review here for reference:

"So, at the moment, AFAICT every event is marked as exception == true,
so this doesn't actually tell us anything.   If that becomes not the
case in future, can the exception flag be derived from the log_type or
information in the even buffer? "

I've checked the code and we're just using exception == true.  The two
event logs that we currently support are RTAS_LOG_TYPE_EPOW and
RTAS_LOG_TYPE_HOTPLUG, both are being added in the queue by
calling rtas_event_log_queue() with exception == true.

This boolean is passed as a parameter in the functions 
rtas_event_log_contains
and rtas_event_log_dequeue. The former is called once with exception=true
inside check_exception, the latter is called once with exception=true in 
check_exception
and exception=false in event_scan.

I didn't find anywhere in the code where, once set as true, we change 
this boolean
to false. So in my opinion we can discard this boolean from the 
migration and,
in post_load, set it to true if log_type is RTAS_LOG_TYPE_EPOW or
RTAS_LOG_TYPE_HOTPLUG. This would mean that when we implement more event
log types we will need to also change the post_load to reflect the change.



PS: I've read the LoPAPR document [1] and it says in section 10.2.3 page 
289:

"Hot Plug Events, when implemented, are reported through the event-scan 
RTAS call."

Why are we setting the RTAS_LOG_TYPE_HOTPLUG as exception==true and 
therefore
reporting it in check_exception instead? Does the sPAPR spec differ from 
the LoPAPR
in this regard?

[1] 
https://openpowerfoundation.org/wp-content/uploads/2016/05/LoPAPR_DRAFT_v11_24March2016_cmt1.pdf


Thanks,

Daniel

> > > +        VMSTATE_UINT32(data_size, sPAPREventLogEntry),
> > > +        VMSTATE_VARRAY_UINT32_ALLOC(data, sPAPREventLogEntry, 
> > > data_size,
> > > +                                    0, vmstate_info_uint8, uint8_t),
> > And I think this should be VBUFFER rather than VARRAY to avoid the
> > data type change.
> >
> > > +        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;
> > > @@ -1535,6 +1567,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 f0b28d8..70c7cfc 100644
> > > --- a/hw/ppc/spapr_events.c
> > > +++ b/hw/ppc/spapr_events.c
> > > @@ -342,7 +342,8 @@ static int 
> > > rtas_event_log_to_irq(sPAPRMachineState *spapr, int log_type)
> > >       return source->irq;
> > >   }
> > >   -static void rtas_event_log_queue(int log_type, void *data, bool 
> > > exception)
> > > +static void rtas_event_log_queue(int log_type, void *data, bool 
> > > exception,
> >   +                                 int data_size)
> >
> > This could derive data_size from the header passed in.
> >
> > >   {
> > >       sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > >       sPAPREventLogEntry *entry = g_new(sPAPREventLogEntry, 1);
> > > @@ -351,6 +352,7 @@ static void rtas_event_log_queue(int log_type, 
> > > void *data, bool exception)
> > >       entry->log_type = log_type;
> > >       entry->exception = exception;
> > >       entry->data = data;
> > > +    entry->data_size = data_size;
> > >       QTAILQ_INSERT_TAIL(&spapr->pending_events, entry, next);
> > >   }
> > >   @@ -445,6 +447,7 @@ static void spapr_powerdown_req(Notifier *n, 
> > > void *opaque)
> > >       struct rtas_event_log_v6_mainb *mainb;
> > >       struct rtas_event_log_v6_epow *epow;
> > >       struct epow_log_full *new_epow;
> > > +    uint32_t data_size;
> > >         new_epow = g_malloc0(sizeof(*new_epow));
> > >       hdr = &new_epow->hdr;
> > > @@ -453,14 +456,13 @@ static void spapr_powerdown_req(Notifier *n, 
> > > void *opaque)
> > >       mainb = &new_epow->mainb;
> > >       epow = &new_epow->epow;
> > >   +    data_size = sizeof(*new_epow);
> > >       hdr->summary = cpu_to_be32(RTAS_LOG_VERSION_6
> > >                                  | RTAS_LOG_SEVERITY_EVENT
> > >                                  | RTAS_LOG_DISPOSITION_NOT_RECOVERED
> > >                                  | RTAS_LOG_OPTIONAL_PART_PRESENT
> > >                                  | RTAS_LOG_TYPE_EPOW);
> > > -    hdr->extended_length = cpu_to_be32(sizeof(*new_epow)
> > > -                                       - sizeof(new_epow->hdr));
> > > -
> > > +    hdr->extended_length = cpu_to_be32(data_size - 
> > > sizeof(new_epow->hdr));
> > >       spapr_init_v6hdr(v6hdr);
> > >       spapr_init_maina(maina, 3 /* Main-A, Main-B and EPOW */);
> > >   @@ -479,7 +481,7 @@ static void spapr_powerdown_req(Notifier *n, 
> > > void *opaque)
> > >       epow->event_modifier = RTAS_LOG_V6_EPOW_MODIFIER_NORMAL;
> > >       epow->extended_modifier = 
> > > RTAS_LOG_V6_EPOW_XMODIFIER_PARTITION_SPECIFIC;
> > >   -    rtas_event_log_queue(RTAS_LOG_TYPE_EPOW, new_epow, true);
> > > +    rtas_event_log_queue(RTAS_LOG_TYPE_EPOW, new_epow, true, 
> > > data_size);
> > >         qemu_irq_pulse(xics_get_qirq(XICS_FABRIC(spapr),
> > > rtas_event_log_to_irq(spapr,
> > > @@ -504,6 +506,7 @@ static void spapr_hotplug_req_event(uint8_t 
> > > hp_id, uint8_t hp_action,
> > >       struct rtas_event_log_v6_maina *maina;
> > >       struct rtas_event_log_v6_mainb *mainb;
> > >       struct rtas_event_log_v6_hp *hp;
> > > +    uint32_t data_size;
> > >         new_hp = g_malloc0(sizeof(struct hp_log_full));
> > >       hdr = &new_hp->hdr;
> > > @@ -512,14 +515,14 @@ static void spapr_hotplug_req_event(uint8_t 
> > > hp_id, uint8_t hp_action,
> > >       mainb = &new_hp->mainb;
> > >       hp = &new_hp->hp;
> > >   +    data_size = sizeof(*new_hp);
> > >       hdr->summary = cpu_to_be32(RTAS_LOG_VERSION_6
> > >                                  | RTAS_LOG_SEVERITY_EVENT
> > >                                  | RTAS_LOG_DISPOSITION_NOT_RECOVERED
> > >                                  | RTAS_LOG_OPTIONAL_PART_PRESENT
> > >                                  | RTAS_LOG_INITIATOR_HOTPLUG
> > >                                  | RTAS_LOG_TYPE_HOTPLUG);
> > > -    hdr->extended_length = cpu_to_be32(sizeof(*new_hp)
> > > -                                       - sizeof(new_hp->hdr));
> > > +    hdr->extended_length = cpu_to_be32(data_size - 
> > > sizeof(new_hp->hdr));
> > >         spapr_init_v6hdr(v6hdr);
> > >       spapr_init_maina(maina, 3 /* Main-A, Main-B, HP */);
> > > @@ -572,7 +575,7 @@ static void spapr_hotplug_req_event(uint8_t 
> > > hp_id, uint8_t hp_action,
> > >               cpu_to_be32(drc_id->count_indexed.index);
> > >       }
> > >   -    rtas_event_log_queue(RTAS_LOG_TYPE_HOTPLUG, new_hp, true);
> > > +    rtas_event_log_queue(RTAS_LOG_TYPE_HOTPLUG, new_hp, true, 
> > > data_size);
> > >         qemu_irq_pulse(xics_get_qirq(XICS_FABRIC(spapr),
> > > rtas_event_log_to_irq(spapr,
> > > @@ -671,8 +674,7 @@ static void check_exception(PowerPCCPU *cpu, 
> > > sPAPRMachineState *spapr,
> > >       if (!event) {
> > >           goto out_no_events;
> > >       }
> > > -
> > > -    hdr = event->data;
> > > +    hdr = (struct rtas_error_log *)event->data;
> > >       event_len = be32_to_cpu(hdr->extended_length) + sizeof(*hdr);
> > >         if (event_len < len) {
> > > @@ -728,7 +730,7 @@ static void event_scan(PowerPCCPU *cpu, 
> > > sPAPRMachineState *spapr,
> > >           goto out_no_events;
> > >       }
> > >   -    hdr = event->data;
> > > +    hdr = (struct rtas_error_log *)event->data;
> > >       event_len = be32_to_cpu(hdr->extended_length) + sizeof(*hdr);
> > >         if (event_len < len) {
> > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > index 5802f88..fbe1d93 100644
> > > --- a/include/hw/ppc/spapr.h
> > > +++ b/include/hw/ppc/spapr.h
> > > @@ -599,7 +599,8 @@ sPAPRTCETable 
> > > *spapr_tce_find_by_liobn(target_ulong liobn);
> > >   struct sPAPREventLogEntry {
> > >       int log_type;
> > >       bool exception;
> > > -    void *data;
> > > +    uint8_t *data;
> > > +    uint32_t data_size;
> > >       QTAILQ_ENTRY(sPAPREventLogEntry) next;
> > >   };