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 <du...@linux.vnet.ibm.com>
> >>>>>
> >>>>> 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 <du...@linux.vnet.ibm.com>
> >>>>> Signed-off-by: Daniel Henrique Barboza <danie...@linux.vnet.ibm.com>
> >>>> 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;
> >>>>> };
> >>>
>
