From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58432) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VFQC4-0007Or-CB for qemu-devel@nongnu.org; Fri, 30 Aug 2013 11:01:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VFQBz-000610-B5 for qemu-devel@nongnu.org; Fri, 30 Aug 2013 11:01:00 -0400 Received: from smtp02.citrix.com ([66.165.176.63]:15774) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VFQBz-00060o-4Q for qemu-devel@nongnu.org; Fri, 30 Aug 2013 11:00:55 -0400 Message-ID: <5220B3A3.4000100@citrix.com> Date: Fri, 30 Aug 2013 16:00:51 +0100 From: Anthony PERARD MIME-Version: 1.0 References: In-Reply-To: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V2] qemu-xen: HVM domain S3 bugfix List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Liu, Jinsong" Cc: George Dunlap , "qemu-devel@nongnu.org" , "xen-devel@lists.xen.org" , Ian Campbell , Stefano Stabellini On 29/08/13 09:25, Liu, Jinsong wrote: > Currently HVM S3 has a bug coming from the difference between > qemu-traditioanl and qemu-xen. For qemu-traditional, the way > to resume from hvm s3 is via 'xl trigger' command. However, > for qemu-xen, the way to resume from hvm s3 inherited from > standard qemu, i.e. via QMP, and it doesn't work under Xen. > > The root cause is, for qemu-xen, 'xl trigger' command didn't reset > devices, while QMP didn't unpause hvm domain though they did qemu > system reset. > > We have 2 patches to fix the HVM S3 bug: qemu-xen patch and xl patch. > This patch is the qemu-xen patch. It registers a wakeup later notify, > so that when 'xl trigger' command invokes QMP system_wakeup and after > qemu system reset, it hypercalls to hypervisor to unpause domain, then > hvm S3 resumes successfully. > > Signed-off-by: Liu Jinsong > --- > vl.c | 13 +++++++++++++ > xen-all.c | 9 +++++++++ > 2 files changed, 22 insertions(+), 0 deletions(-) > > diff --git a/vl.c b/vl.c > index 5314f55..aeebd83 100644 > --- a/vl.c > +++ b/vl.c > @@ -1478,6 +1478,8 @@ static NotifierList suspend_notifiers = > NOTIFIER_LIST_INITIALIZER(suspend_notifiers); > static NotifierList wakeup_notifiers = > NOTIFIER_LIST_INITIALIZER(wakeup_notifiers); > +static NotifierList wakeup_later_notifiers = > + NOTIFIER_LIST_INITIALIZER(wakeup_later_notifiers); Maybe late_wakeup_notifiers would be a better description for this list. > static uint32_t wakeup_reason_mask = ~0; > static RunState vmstop_requested = RUN_STATE_MAX; > > @@ -1625,6 +1627,11 @@ static void qemu_system_suspend(void) > monitor_protocol_event(QEVENT_SUSPEND, NULL); > } > > +static void qemu_system_wakeup(void) > +{ > + notifier_list_notify(&wakeup_later_notifiers, NULL); > +} > + I don't think it's useful to have this function with only the list_notify. You could call directly list_notify in the main_loop_should_exit() function bellow. > void qemu_system_suspend_request(void) > { > if (runstate_check(RUN_STATE_SUSPENDED)) { > @@ -1668,6 +1675,11 @@ void qemu_register_wakeup_notifier(Notifier *notifier) > notifier_list_add(&wakeup_notifiers, notifier); > } > > +void qemu_register_wakeup_later_notifier(Notifier *notifier) > +{ > + notifier_list_add(&wakeup_later_notifiers, notifier); > +} > + > void qemu_system_killed(int signal, pid_t pid) > { > shutdown_signal = signal; > @@ -1744,6 +1756,7 @@ static bool main_loop_should_exit(void) > cpu_synchronize_all_states(); > qemu_system_reset(VMRESET_SILENT); > resume_all_vcpus(); > + qemu_system_wakeup(); > monitor_protocol_event(QEVENT_WAKEUP, NULL); > } > if (qemu_powerdown_requested()) { > diff --git a/xen-all.c b/xen-all.c > index 15be8ed..3353f63 100644 > --- a/xen-all.c > +++ b/xen-all.c > @@ -97,6 +97,7 @@ typedef struct XenIOState { > > Notifier exit; > Notifier suspend; > + Notifier wakeup_later; > } XenIOState; > > /* Xen specific function for piix pci */ > @@ -139,6 +140,11 @@ static void xen_suspend_notifier(Notifier *notifier, void *data) > xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 3); > } > > +static void xen_wakeup_later_notifier(Notifier *notifier, void *data) > +{ > + xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 0); > +} > + > /* Xen Interrupt Controller */ > > static void xen_set_irq(void *opaque, int irq, int level) > @@ -1106,6 +1112,9 @@ int xen_hvm_init(void) > state->suspend.notify = xen_suspend_notifier; > qemu_register_suspend_notifier(&state->suspend); > > + state->wakeup_later.notify = xen_wakeup_later_notifier; > + qemu_register_wakeup_later_notifier(&state->wakeup_later); > + > xc_get_hvm_param(xen_xc, xen_domid, HVM_PARAM_IOREQ_PFN, &ioreq_pfn); > DPRINTF("shared page at pfn %lx\n", ioreq_pfn); > state->shared_page = xc_map_foreign_range(xen_xc, xen_domid, XC_PAGE_SIZE, > Could you break this path into two? One which add the notifier list and a second one with the Xen specific part? Thanks, -- Anthony PERARD