Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode

[Anchal Agarwal <anchalag@xxxxxxxxxx>]

On Sat, Jul 18, 2020 at 09:47:04PM -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.
> 
> 
> 
> (Roger, question for you at the very end)
> 
> On 7/17/20 3:10 PM, Anchal Agarwal wrote:
> > 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:
> >>> On Mon, Jul 13, 2020 at 11:52:01AM -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/2/20 2:21 PM, Anchal Agarwal wrote:
> >>>>> +
> >>>>> +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?
> 
> 
> 
> I think I understand now what you are trying to say --- it's whether we
> are going to use xen_suspend() routine, right? If that's the case then
> sure, you can use "xen_suspend" term. (I'd probably still change
> xen_is_xen_suspend() to is_xen_suspend())
>
I think so too. Will change it.
> 
> >>>>> +{
> >>>>> +     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?
> 
> 
> I don't know what would happen if ARM guest tries to handle hibernation
> callbacks. The only ones that you are introducing are in block and net
> fronts and that's arch-independent.
> 
> 
> You do add a bunch of x86-specific code though (syscore ops), would
> something similar be needed for ARM?
> 
> 
I don't expect this to work out of the box on ARM. To start with something
similar will be needed for ARM too.
We may still want to keep the driver code as-is.

I understand the concern here wrt ARM, however, currently the support is only
proposed for x86 guests here and similar work could be carried out for ARM.
Also, if regular hibernation works correctly on arm, then all is needed is to
fix Xen side of things.

I am not sure what could be done to achieve any assurances on arm side as far as
this series is concerned.
> >>>> 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?
> 
> 
> I suspect dom0 will not do well now as far as hibernation goes, in which
> case you are not breaking anything.
> 
> 
> Roger?
> 
> 
> -boris

Thanks,
Anchal
> 
> 
>