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

[Daniel Henrique Barboza]

On 05/18/2017 01:46 PM, Michael Roth wrote:
> Quoting Daniel Henrique Barboza (2017-05-17 15:31:44)
> > 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?
> Published versions of PAPR/LoPAPR are a bit behind on the current
> documentation for hotplug (and a few other things). That section in
> particular has been update to read:
>
>    10.2.3 Hot Plug Events
>
>    Hot Plug Events, when implemented, are reported through either the
>    event-scan RTAS call or a hotplug interrupt.
>
>    An OS that wants to be notified of hotplug events will need to set the
>    appropriate arch-vector bit (XXX TBD) look for the
>    hot-plug-events node in the /event-sources node of the OF device tree
>    (see C.6.12.1.4), enable the interrupts listed in its
>    “interrupts” property and provide an interrupt handler to call
>    check-exception when one of those interrupts are received.
>
>    When a hotplug event occurs, whether reported by check-exception or
>    event-scan, RTAS will directly pass back the Hotplug
>    Event Log as described in Table XXX “Platform Event Log, Version 6,
>    Hotplug Section” on page XXX.
>
> Published documentation also lacks a description of the actual,
> newly-added hotplug event format. A summary of that and most of the
> other changes is included in:
>
>    qemu.git/docs/specs/ppc-spapr-hotplug.txt
>
> There's some work to get the changes published in LoPAPR but for now
> that's probably the best reference.

Thanks for the info Mike!

> > [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;
> > > > >    };