|
| From: | Daniel Henrique Barboza |
| Subject: | Re: [Qemu-ppc] [PATCH v10] migration: spapr: migrate pending_events of spapr state |
| Date: | Thu, 18 May 2017 11:33:58 -0300 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 |
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 <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.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; };
| [Prev in Thread] | Current Thread | [Next in Thread] |