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

[Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>]

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 <duanj@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 <duanj@linux.vnet.ibm.com>
>>>>> Signed-off-by: Daniel Henrique Barboza <danielhb@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.

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