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

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

[Daniel Henrique Barboza]

v13:
- rebased with dgibson/ppc-for-2.10
- patch 1 removed from the series (already applied at ppc-for-2.10)
- data_size is now being calculated by using the extended_length field inside
rtas_event_log_queue

v12:
- patch 2: added a switch statement to get the proper data_size based on
the log_type

v11:
- patch 1 (new): cleanup of spapr_events.c:
    * removed the 'exception' boolean from the sPAPREventLogEntry
    * simplified the 'event_scan' function
- patch 2:
    * data_size is now calculated inside rtas_event_log_queue()
    * using VBUFFER instead of VARRAY to avoid casts
    * log_type changed to int32_t

v10: detached from DRC patch set

v9: no changes

v8: no changes

v7: no changes

v6: - Rebased with QEMU master after 6+ months.
class and minor improvements.
    - Added clarifications from the previous v5 discussions in the commit messages.

v5: - Rebased on David's ppc-for-2.8.

v4: - Rebased on David's ppc-for-2.7. 

v3: - Simplify overall design followng discussion with Paolo. No longer need
      metadata to migrate QTAILQ.
    - Extend VMStateInfo instead of adding similar fields to VMStateField.

v2: - Put the newly added migrating fields in subsections so that backward 
      migration is not broken.  
(link: https://lists.nongnu.org/archive/html/qemu-devel/2016-05/msg04188.html)

v1: - Inital version.
(link: https://lists.nongnu.org/archive/html/qemu-devel/2016-04/msg02601.html)


This patch was detached from the patchset:

"[PATCH v9 0/6] migration/ppc: migrating DRC, ccs_list and pending_events"

Because it is independent and has use outside of the scope of the
pseries DRC migration patchset.



Jianjun Duan (1):
  migration: spapr: migrate pending_events of spapr state

 hw/ppc/spapr.c         | 32 ++++++++++++++++++++++++++++++++
 hw/ppc/spapr_events.c  | 19 +++++++++++++++++++
 include/hw/ppc/spapr.h |  3 ++-
 3 files changed, 53 insertions(+), 1 deletion(-)

-- 
2.9.4

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

[Daniel Henrique Barboza]

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);
+    }
+
     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;
 };
 
-- 
2.9.4

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5669 bytes --]

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.

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.


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

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Daniel Henrique Barboza]

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