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

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

On 05/23/2017 01:07 AM, David Gibson wrote:
> On Mon, May 22, 2017 at 03:40:39PM -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 a binary stream inside VBUFFER 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>
>> ---
>>   hw/ppc/spapr.c         | 32 ++++++++++++++++++++++++++++++++
>>   hw/ppc/spapr_events.c  | 19 +++++++++++++++++++
>>   include/hw/ppc/spapr.h |  3 ++-
>>   3 files changed, 53 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 75e298b..558f951 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1453,6 +1453,37 @@ 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_UINT32(data_size, sPAPREventLogEntry),
>> +        VMSTATE_VBUFFER_ALLOC_UINT32(data, sPAPREventLogEntry, 0,
>> +                                     NULL, data_size),
>> +        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;
>> @@ -1551,6 +1582,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 73e2a18..6c42041 100644
>> --- a/hw/ppc/spapr_events.c
>> +++ b/hw/ppc/spapr_events.c
>> @@ -346,10 +346,29 @@ static void rtas_event_log_queue(int log_type, void *data)
>>   {
>>       sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>>       sPAPREventLogEntry *entry = g_new(sPAPREventLogEntry, 1);
>> +    struct epow_log_full *new_epow = NULL;
>> +    struct hp_log_full *new_hp = NULL;
>> +    uint32_t ext_length = 0;
>>   
>>       g_assert(data);
>>       entry->log_type = log_type;
>>       entry->data = data;
>> +
>> +    switch (log_type) {
>> +    case RTAS_LOG_TYPE_EPOW:
>> +        new_epow = (struct epow_log_full *)data;
>> +        ext_length = be32_to_cpu(new_epow->hdr.extended_length);
>> +        entry->data_size = ext_length + sizeof(new_epow->hdr);
>> +        break;
>> +    case RTAS_LOG_TYPE_HOTPLUG:
>> +        new_hp = (struct hp_log_full *)data;
>> +        ext_length = be32_to_cpu(new_hp->hdr.extended_length);
>> +        entry->data_size = ext_length + sizeof(new_hp->hdr);
>> +        break;
>> +    default:
>> +        g_assert(false);
>> +    }
>> +
> You're still overcomplicating this.  Both epow_log_full and
> hp_log_full start with an rtas_error_log header, which is what
> contains the extended_length field.  You can just case data directly
> to struct rtas_error_log *, and derive the data size from there.

Yeah this is indeed overcomplicated. Just saw the following in
check_exception():

     hdr = event->data;
     event_len = be32_to_cpu(hdr->extended_length) + sizeof(*hdr);

Way simpler than what I was doing.

>
> And.. come to think of it, you don't need the log_type in the vmsd
> either, since it can be derived from the summary field in the same
> header.
>
> And.. going even further, we could alter the existing code so that
> instead of embedding the rtas_error_log header in the allocated
> buffer, we could inline it into the sPAPREventLogEntry structure, with
> just the extended data in the variable sized buffer.  Then we wouldn't
> need a new data_size field, the extended_size field from the
> rtas_error_log substructure would already be exactly what we need to
> size the buffer.

Sounds good. I'll see what I can do.


Daniel

>
>
>>       QTAILQ_INSERT_TAIL(&spapr->pending_events, entry, next);
>>   }
>>   
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index be2b3b8..b2fcd62 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -598,8 +598,9 @@ struct sPAPRTCETable {
>>   sPAPRTCETable *spapr_tce_find_by_liobn(target_ulong liobn);
>>   
>>   struct sPAPREventLogEntry {
>> -    int log_type;
>> +    int32_t log_type;
>>       void *data;
>> +    uint32_t data_size;
>>       QTAILQ_ENTRY(sPAPREventLogEntry) next;
>>   };
>>