On Wed, Jul 15, 2020 at 05:18:08PM -0400, Boris Ostrovsky wrote:

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.



On 7/15/20 4:49 PM, Anchal Agarwal wrote:

quoted

On Mon, Jul 13, 2020 at 11:52:01AM -0400, Boris Ostrovsky wrote:

quoted

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.



On 7/2/20 2:21 PM, Anchal Agarwal wrote:

quoted

+
+bool xen_is_xen_suspend(void)

Weren't you going to call this pv suspend? (And also --- is this suspend
or hibernation? Your commit messages and cover letter talk about fixing
hibernation).

This is for hibernation is for pvhvm/hvm/pv-on-hvm guests as you may call it.
The method is just there to check if "xen suspend" is in progress.
I do not see "xen_suspend" differentiating between pv or hvm
domain until later in the code hence, I abstracted it to xen_is_xen_suspend.

I meant "pv suspend" in the sense that this is paravirtual suspend, not
suspend for paravirtual guests. Just like pv drivers are for both pv and
hvm guests.


And then --- should it be pv suspend or pv hibernation?

Ok so I think I am lot confused by this question. Here is what this
function for, function xen_is_xen_suspend() just tells us whether 
the guest is in "SHUTDOWN_SUSPEND" state or not. This check is needed
for correct invocation of syscore_ops callbacks registered for guest's
hibernation and for xenbus to invoke respective callbacks[suspend/resume
vs freeze/thaw/restore].
Since "shutting_down" state is defined static and is not directly available
to other parts of the code, the function solves the purpose.

I am having hard time understanding why this should be called pv
suspend/hibernation unless you are suggesting something else?
Am I missing your point here? 

quoted

quoted

quoted

+{
+     return suspend_mode == XEN_SUSPEND;
+}
+

+static int xen_setup_pm_notifier(void)
+{
+     if (!xen_hvm_domain())
+             return -ENODEV;

I forgot --- what did we decide about non-x86 (i.e. ARM)?

It would be great to support that however, its  out of
scope for this patch set.
I’ll be happy to discuss it separately.

I wasn't implying that this *should* work on ARM but rather whether this
will break ARM somehow (because xen_hvm_domain() is true there).

Ok makes sense. TBH, I haven't tested this part of code on ARM and the series
was only support x86 guests hibernation.
Moreover, this notifier is there to distinguish between 2 PM
events PM SUSPEND and PM hibernation. Now since we only care about PM
HIBERNATION I may just remove this code and rely on "SHUTDOWN_SUSPEND" state.
However, I may have to fix other patches in the series where this check may
appear and cater it only for x86 right?

quoted

quoted

And PVH dom0.

That's another good use case to make it work with however, I still
think that should be tested/worked upon separately as the feature itself
(PVH Dom0) is very new.

Same question here --- will this break PVH dom0?

I haven't tested it as a part of this series. Is that a blocker here?

Thanks,
Anchal

-boris