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

[David Gibson]

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

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

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

signature.asc
Description: PGP signature