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

[Michael Roth]

Quoting Daniel Henrique Barboza (2017-05-18 09:33:58)
> 
> 
> On 05/18/2017 01:30 AM, David Gibson wrote:
> > On Wed, May 17, 2017 at 05:31:44PM -0300, Daniel Henrique Barboza wrote:
> >>
> >> 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.
> > This actually suggests we should just remove the exception flag as a
> > preliminary cleanup.
> 
> Definitely an option. This would imply directly that both 
> rtas_event_log_contains
> and rtas_event_log_dequeue will not consider this flag in their logic - 
> we would simply
> remove the flag from their parameter list. In the case of 'events_scan', 
> however,
> this piece of code would be directly impacted:
> 
> 
>      event = rtas_event_log_dequeue(mask, false);
>      if (!event) {
>          goto out_no_events;
>      }
> (...)
> out_no_events:
>      rtas_st(rets, 0, RTAS_OUT_NO_ERRORS_FOUND);
> 
> } // end of events_scan
> 
> 
> rtas_event_log_dequeue(mask, false) would make no sense here anymore 
> (assuming
> that the flag is always true, this would always return NULL) . I propose 
> 2 alternatives:
> 
> 1 - a placeholder function, rtas_event_log_dequeue_events() for example, 
> that would return
> NULL here. The remaining logic would be untouched. When we implement 
> more log_types
> that would qualify to be returned by events_scan(), we simply implement 
> the remaining of
> rtas_event_log_dequeue_events function.
> 
> 2 - remove everything after that dequeue() call (including the call 
> itself) and simply execute
> rtas_st(rets, 0, RTAS_OUT_NO_ERRORS_FOUND), the execution after the 
> 'out_no_events' jump .
> We can retrieve the body contents with git when we implement more 
> log_types that are
> eligible to be retrieved here.

This seems reasonable. Looking back at the original commit:

  79853e18d904b0a4bcef62701d48559688007c93

we only added event_scan() because the guest relies on the RTAS call
being advertised to initialize some event-reporting stuff we need
for check_exception(). So it was always just a stub, albeit with some
considerations for future event support that we could always
re-implement if they ever actually arise.

> 
> I prefer (2) because this would be a proper cleanup but I wouldn't lose 
> my sleep if we
> go for (1). Any alternatives/ideas are welcome here.
> 
> 
> Daniel
> 
> 
> >
> >>
> >>
> >> 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;
> >>>>>    };
> >>>
>