[v10] migration: spapr: migrate pending_events of spapr state

Commit Message

Daniel Henrique Barboza May 15, 2017, 1:10 p.m. UTC

From: Jianjun Duan <[e-mail]>

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 <[e-mail]>
Signed-off-by: Daniel Henrique Barboza <[e-mail]>
---
 hw/ppc/spapr.c         | 33 +++++++++++++++++++++++++++++++++
 hw/ppc/spapr_events.c  | 24 +++++++++++++-----------
 include/hw/ppc/spapr.h |  3 ++-
 3 files changed, 48 insertions(+), 12 deletions(-)

Comments

Michael Roth May 16, 2017, 3:22 a.m. UTC | #1

Quoting Daniel Henrique Barboza (2017-05-15 08:10:52)
> From: Jianjun Duan <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Daniel Henrique Barboza <[e-mail]>

I think you might've missed some of the review comments David made in
v9.

> ---
>  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),
> +        VMSTATE_UINT32(data_size, sPAPREventLogEntry),
> +        VMSTATE_VARRAY_UINT32_ALLOC(data, sPAPREventLogEntry, data_size,
> +                                    0, vmstate_info_uint8, uint8_t),
> +        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)
>  {
>      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;
>  };
> 
> -- 
> 2.9.3
>

Michael Roth May 16, 2017, 3:26 a.m. UTC | #2

Quoting Daniel Henrique Barboza (2017-05-15 08:10:52)
> From: Jianjun Duan <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Daniel Henrique Barboza <[e-mail]>

I'd also be a little weary of treating this patch as totally standalone.
There's pros/cons, but, without migrated DRC state, migrated hotplug
events won't necessarilly be handled better than the current behavior of
just dropping them.

> ---
>  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),
> +        VMSTATE_UINT32(data_size, sPAPREventLogEntry),
> +        VMSTATE_VARRAY_UINT32_ALLOC(data, sPAPREventLogEntry, data_size,
> +                                    0, vmstate_info_uint8, uint8_t),
> +        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)
>  {
>      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;
>  };
> 
> -- 
> 2.9.3
>

David Gibson May 16, 2017, 4:25 a.m. UTC | #3

On Mon, May 15, 2017 at 10:10:52AM -0300, Daniel Henrique Barboza wrote:
> From: Jianjun Duan <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Daniel Henrique Barboza <[e-mail]>

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.

> ---
>  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.

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

Daniel Henrique Barboza May 16, 2017, 12:04 p.m. UTC | #4

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 <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: Daniel Henrique Barboza <[e-mail]>
> 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.
>
>> +        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;
>>   };
>>

Michael Roth May 17, 2017, 4:05 p.m. UTC | #5

Quoting Michael Roth (2017-05-15 22:22:49)
> Quoting Daniel Henrique Barboza (2017-05-15 08:10:52)
> > From: Jianjun Duan <[e-mail]>
> > 
> > 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 <[e-mail]>
> > Signed-off-by: Daniel Henrique Barboza <[e-mail]>
> 
> I think you might've missed some of the review comments David made in
> v9.

Please ignore, didn't realize I still had this sitting in my outgoing
queue.

> 
> > ---
> >  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),
> > +        VMSTATE_UINT32(data_size, sPAPREventLogEntry),
> > +        VMSTATE_VARRAY_UINT32_ALLOC(data, sPAPREventLogEntry, data_size,
> > +                                    0, vmstate_info_uint8, uint8_t),
> > +        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)
> >  {
> >      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;
> >  };
> > 
> > -- 
> > 2.9.3
> >

Daniel Henrique Barboza May 17, 2017, 8:31 p.m. UTC | #6

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 <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Signed-off-by: Daniel Henrique Barboza <[e-mail]>
>> 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.



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

David Gibson May 18, 2017, 4:29 a.m. UTC | #7

On Mon, May 15, 2017 at 10:26:46PM -0500, Michael Roth wrote:
> Quoting Daniel Henrique Barboza (2017-05-15 08:10:52)
> > From: Jianjun Duan <[e-mail]>
> > 
> > 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 <[e-mail]>
> > Signed-off-by: Daniel Henrique Barboza <[e-mail]>
> 
> I'd also be a little weary of treating this patch as totally standalone.
> There's pros/cons, but, without migrated DRC state, migrated hotplug
> events won't necessarilly be handled better than the current behavior of
> just dropping them.

True, but alone it should at least stop powerdown messages being lost
on migration, which is also possible AFAICT.

> 
> > ---
> >  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),
> > +        VMSTATE_UINT32(data_size, sPAPREventLogEntry),
> > +        VMSTATE_VARRAY_UINT32_ALLOC(data, sPAPREventLogEntry, data_size,
> > +                                    0, vmstate_info_uint8, uint8_t),
> > +        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)
> >  {
> >      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;
> >  };
> > 
>

David Gibson May 18, 2017, 4:30 a.m. UTC | #8

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 <[e-mail]>
> > > > 
> > > > 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 <[e-mail]>
> > > > Signed-off-by: Daniel Henrique Barboza <[e-mail]>
> > > 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.

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

Daniel Henrique Barboza May 18, 2017, 2:33 p.m. UTC | #9

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 <[e-mail]>
>>>>>
>>>>> 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 <[e-mail]>
>>>>> Signed-off-by: Daniel Henrique Barboza <[e-mail]>
>>>> 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;
>>>>>    };
>>>

Michael Roth May 18, 2017, 4:28 p.m. UTC | #10

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 <[e-mail]>
> >>>>>
> >>>>> 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 <[e-mail]>
> >>>>> Signed-off-by: Daniel Henrique Barboza <[e-mail]>
> >>>> 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;
> >>>>>    };
> >>>
>

Michael Roth May 18, 2017, 4:46 p.m. UTC | #11

Quoting Daniel Henrique Barboza (2017-05-17 15:31:44)
> 
> 
> 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 <[e-mail]>
> >>>
> >>> 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 <[e-mail]>
> >>> Signed-off-by: Daniel Henrique Barboza <[e-mail]>
> >> 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.
> 
> 
> 
> 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?

Published versions of PAPR/LoPAPR are a bit behind on the current
documentation for hotplug (and a few other things). That section in
particular has been update to read:

  10.2.3 Hot Plug Events

  Hot Plug Events, when implemented, are reported through either the
  event-scan RTAS call or a hotplug interrupt.

  An OS that wants to be notified of hotplug events will need to set the
  appropriate arch-vector bit (XXX TBD) look for the
  hot-plug-events node in the /event-sources node of the OF device tree
  (see C.6.12.1.4), enable the interrupts listed in its
  “interrupts” property and provide an interrupt handler to call
  check-exception when one of those interrupts are received.

  When a hotplug event occurs, whether reported by check-exception or
  event-scan, RTAS will directly pass back the Hotplug
  Event Log as described in Table XXX “Platform Event Log, Version 6,
  Hotplug Section” on page XXX.

Published documentation also lacks a description of the actual,
newly-added hotplug event format. A summary of that and most of the
other changes is included in:

  qemu.git/docs/specs/ppc-spapr-hotplug.txt

There's some work to get the changes published in LoPAPR but for now
that's probably the best reference.

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

Daniel Henrique Barboza May 18, 2017, 6:28 p.m. UTC | #12

On 05/18/2017 01:46 PM, Michael Roth wrote:
> Quoting Daniel Henrique Barboza (2017-05-17 15:31:44)
>>
>> 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 <[e-mail]>
>>>>>
>>>>> 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 <[e-mail]>
>>>>> Signed-off-by: Daniel Henrique Barboza <[e-mail]>
>>>> 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.
>>
>>
>>
>> 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?
> Published versions of PAPR/LoPAPR are a bit behind on the current
> documentation for hotplug (and a few other things). That section in
> particular has been update to read:
>
>    10.2.3 Hot Plug Events
>
>    Hot Plug Events, when implemented, are reported through either the
>    event-scan RTAS call or a hotplug interrupt.
>
>    An OS that wants to be notified of hotplug events will need to set the
>    appropriate arch-vector bit (XXX TBD) look for the
>    hot-plug-events node in the /event-sources node of the OF device tree
>    (see C.6.12.1.4), enable the interrupts listed in its
>    “interrupts” property and provide an interrupt handler to call
>    check-exception when one of those interrupts are received.
>
>    When a hotplug event occurs, whether reported by check-exception or
>    event-scan, RTAS will directly pass back the Hotplug
>    Event Log as described in Table XXX “Platform Event Log, Version 6,
>    Hotplug Section” on page XXX.
>
> Published documentation also lacks a description of the actual,
> newly-added hotplug event format. A summary of that and most of the
> other changes is included in:
>
>    qemu.git/docs/specs/ppc-spapr-hotplug.txt
>
> There's some work to get the changes published in LoPAPR but for now
> that's probably the best reference.

Thanks for the info Mike!

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

Patch

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),
+        VMSTATE_UINT32(data_size, sPAPREventLogEntry),
+        VMSTATE_VARRAY_UINT32_ALLOC(data, sPAPREventLogEntry, data_size,
+                                    0, vmstate_info_uint8, uint8_t),
+        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)
 {
     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;
 };