Message-ID: <a1e7acec-b437-427f-ba76-5a264df689ef@amd.com>
Date: Wed, 3 Dec 2025 17:33:49 -0500
From: Jason Andryuk <jason.andryuk@....com>
To: Marek Marczykowski-Górecki
	<marmarek@...isiblethingslab.com>
CC: Juergen Gross <jgross@...e.com>, Stefano Stabellini
	<sstabellini@...nel.org>, Oleksandr Tyshchenko
	<oleksandr_tyshchenko@...m.com>, Yann Sionneau <yann.sionneau@...es.tech>,
	<xen-devel@...ts.xenproject.org>, <linux-kernel@...r.kernel.org>, "Daniel
 Smith" <dpsmith@...rtussolutions.com>, Christopher Clark
	<christopher.w.clark@...il.com>
Subject: Re: [PATCH 1/2] xenbus: Use .freeze/.thaw to handle xenbus devices

On 2025-12-01 17:16, Marek Marczykowski-Górecki wrote:
> On Mon, Dec 01, 2025 at 01:20:40PM -0500, Jason Andryuk wrote:
>> On 2025-11-29 21:03, Marek Marczykowski-Górecki wrote:
>>> On Wed, Nov 19, 2025 at 05:47:29PM -0500, Jason Andryuk wrote:
>>>> The goal is to fix s2idle and S3 for Xen PV devices.
>>>
>>> Can you give a little more context of this? We do have working S3 in
>>> qubes with no need for such change. We trigger it via the toolstack (libxl_domain_suspend_only()).
>>> Are you talking about guest-initiated suspend here?
>>
>> This is intended to help domU s2idle/S3 and resume.  I guess that is what
>> you mean by guest-initiated?  The domU can use 'echo mem > /sys/power/state'
>> to enter s2idle/S3.  We also have the domU react to the ACPI sleep button
>> from `xl trigger $dom sleep`.
> 
> Ok, so this is indeed a different path than we use in Qubes OS.
> 
>> AIUI, libxl_domain_suspend_only() triggers xenstore writes which Linux
>> drivers/xen/manage.c:do_suspend() acts on.  `xl save/suspend/migrate` all
>> use this path.
>>
>> The terminology gets confusing.  Xen uses "suspend" for
>> save/suspend/migrate, but the Linux power management codes uses
>> freeze/thaw/restore.  AIUI, Linux's PMSG_SUSPEND/.suspend is for runtime
>> power management.
> 
> Indeed it gets confusing...
> 
>> When you call libxl_domain_suspend_only()/libxl_domain_resume(), you pass
>> suspend_cancel==1.
>>   *  1. (fast=1) Resume the guest without resetting the domain
>>         environment.
>>   *     The guests's call to SCHEDOP_shutdown(SHUTDOWN_suspend) will
>>         return 1.
>>
>> That ends up in Linux do_suspend() as si.cancelled = 1, which calls
>> PMSG_THAW -> .thaw -> xenbus_dev_cancel() which is a no-op.  So it does not
>> change the PV devices.
>>
>> We needed guest user space to perform actions before entering s2idle.
>> libxl_domain_suspend_only() triggers the Linux kernel path which does not
>> notify user space.  The ACPI power buttons let user space perform actions
>> (lock and blank the screen) before entering the idle state.
> 
> I see. In our case, we have our own userspace hook that gets called
> before (if relevant - in most cases it isn't).
> 
>>> We also have kinda working (host) s2idle. You may want to take a look at this
>>> work (some/most of it was posted upstream, but not all got
>>> committed/reviewed):
>>> https://github.com/QubesOS/qubes-issues/issues/6411#issuecomment-1538089344
>>> https://github.com/QubesOS/qubes-linux-kernel/pull/910 (some patches
>>> changed since that PR, see the current main too).
>>
>> This would not affect host s2idle - it changes PV frontend devices.
>>
>> Do you libxl_domain_suspend_only() all domUs and then put dom0 into s0ix?
> 
> Yes, exactly.
> 
>>>> A domain resuming
>>>> from s3 or s2idle disconnects its PV devices during resume.  The
>>>> backends are not expecting this and do not reconnect.
>>>>
>>>> b3e96c0c7562 ("xen: use freeze/restore/thaw PM events for suspend/
>>>> resume/chkpt") changed xen_suspend()/do_suspend() from
>>>> PMSG_SUSPEND/PMSG_RESUME to PMSG_FREEZE/PMSG_THAW/PMSG_RESTORE, but the
>>>> suspend/resume callbacks remained.
>>>>
>>>> .freeze/restore are used with hiberation where Linux restarts in a new
>>>> place in the future.  .suspend/resume are useful for runtime power
>>>> management for the duration of a boot.
>>>>
>>>> The current behavior of the callbacks works for an xl save/restore or
>>>> live migration where the domain is restored/migrated to a new location
>>>> and connecting to a not-already-connected backend.
>>>>
>>>> Change xenbus_pm_ops to use .freeze/thaw/restore and drop the
>>>> .suspend/resume hook.  This matches the use in drivers/xen/manage.c for
>>>> save/restore and live migration.  With .suspend/resume empty, PV devices
>>>> are left connected during s2idle and s3, so PV devices are not changed
>>>> and work after resume.
>>>
>>> Is that intended? While it might work for suspend by a chance(*), I'm
>>> pretty sure not disconnecting + re-reconnecting PV devices across
>>> save/restore/live migration will break them.
>>
>> save/restore/live migration keep using .freeze/thaw/restore, which
>> disconnects and reconnects today.  Nothing changes there as
>> xen_suspend()/do_suspend() call the power management code with
>> PMSG_FREEZE/PMSG_THAW/PMSG_RESTORE.
>>
>> This patches makes .suspend/resume no-ops for PMSG_SUSPEND/PMSG_RESUME. When
>> a domU goes into s2idle/S3, the backend state remains connected. With this
>> patch, when the domU wakes up, the frontends do nothing and remain
>> connected.
> 
> This explanation makes sense.
> 
>>> (*) and even that I'm not sure - with driver domains, depending on
>>> suspend order this feels like might result in a deadlock...
>>
>> I'm not sure.  I don't think this patch changes anything with respect to
>> them.
>>
>> Thanks for testing.
>>
>> Maybe the commit messages should change to highlight this is for domU PV
>> devices?  struct xen_bus_type xenbus_backend does not define dev_pm_ops.
> 
> Good idea.
> 
>> Regards,
>> Jason
>>
>>>> Signed-off-by: Jason Andryuk <jason.andryuk@....com>
>>>> ---
>>>>    drivers/xen/xenbus/xenbus_probe_frontend.c | 4 +---
>>>>    1 file changed, 1 insertion(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c b/drivers/xen/xenbus/xenbus_probe_frontend.c
>>>> index 6d1819269cbe..199917b6f77c 100644
>>>> --- a/drivers/xen/xenbus/xenbus_probe_frontend.c
>>>> +++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
>>>> @@ -148,11 +148,9 @@ static void xenbus_frontend_dev_shutdown(struct device *_dev)
>>>>    }
>>>>    static const struct dev_pm_ops xenbus_pm_ops = {
>>>> -	.suspend	= xenbus_dev_suspend,
>>>> -	.resume		= xenbus_frontend_dev_resume,
>>>>    	.freeze		= xenbus_dev_suspend,
>>>>    	.thaw		= xenbus_dev_cancel,
>>>> -	.restore	= xenbus_dev_resume,
>>>> +	.restore	= xenbus_frontend_dev_resume,

I was double checking before sending a v2, and I have questions about 
this.  I purposely switched the .restore callback since 
xenbus_frontend_dev_resume() handles the extra case.  It was added in:

commit 2abb274629614bef4044a0b98ada42e977feadfd
Author: Aurelien Chartier <aurelien.chartier@...rix.com>
Date:   Tue May 28 18:09:56 2013 +0100

     xenbus: delay xenbus frontend resume if xenstored is not running

     If the xenbus frontend is located in a domain running xenstored, 
the device
     resume is hanging because it is happening before the process 
resume. This
     patch adds extra logic to the resume code to check if we are the domain
     running xenstored and delay the resume if needed.

It was after b3e96c0c7562, so .freeze/thaw/restore were already present 
for domU xen_suspend() handling.  So the .resume handler should have 
been used.

This is for the "domain running xenstored", so dom0.  So maybe this was 
called for a dom0 S3 suspend/resume?  But as stated above this is patch 
changes PV frontends.  Maybe the change was for Xenclient/OpenXT - 
OpenXT has netfront in dom0 connected to the network driver domain.  But 
without netback changes, I don't think that would work today?  As it is, 
S3 in OpenXT has been disabled for years as broken.

Ok, yes, dom0 S3:
https://lore.kernel.org/xen-devel/1367413042-13987-1-git-send-email-aurelien.chartier@citrix.com/

 > This patch series fixes the S3 resume of dom0 or a Xenstore stub
 > domain running a frontend over xenbus (xen-netfront in my use case).
 >
 > As device resume is happening before process resume, the xenbus
 > frontend resume is hanging if xenstored is not running, thus causing
 > a deadlock. This patch series is fixing that issue by bypassing the
 > xenbus frontend resume when we are running in dom0 or a Xenstore stub
 > domain.

I don't think setting .restore = xenbus_frontend_dev_resume will break 
anything for a domU.  It handles a case which doesn't trigger for a 
domU.  Removing .resume, a frontend in dom0 will not be touched, so that 
at least cannot hang.

Does this all sound okay?  Does anyone think I am missing anything

Regards,
Jason

Powered by blists - more mailing lists