On Fri, Jun 05, 2020 at 05:24:37PM -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 6/3/20 6:40 PM, Agarwal, Anchal 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 5/19/20 7:26 PM, Anchal Agarwal wrote:
    > From: Munehisa Kamata [off-list ref]
    >
    > Add Xen PVHVM specific system core callbacks for PM suspend and
    > hibernation support. The callbacks suspend and resume Xen
    > primitives,like shared_info, pvclock and grant table. Note that
    > Xen suspend can handle them in a different manner, but system
    > core callbacks are called from the context.


    I don't think I understand that last sentence.

Looks like it may have cryptic meaning of stating that xen_suspend calls syscore_suspend from xen_suspend
So, if these syscore ops gets called  during xen_suspend do not do anything. Check if the mode is in xen suspend
and return from there. These syscore_ops are specifically for domU hibernation.
I must admit, I may have overlooked lack of explanation of some implicit details in the original commit msg.

    >  So if the callbacks
    > are called from Xen suspend context, return immediately.
    >


    > +
    > +static int xen_syscore_suspend(void)
    > +{
    > +     struct xen_remove_from_physmap xrfp;
    > +     int ret;
    > +
    > +     /* Xen suspend does similar stuffs in its own logic */
    > +     if (xen_suspend_mode_is_xen_suspend())
    > +             return 0;

With your explanation now making this clearer, is this check really
necessary? From what I see we are in XEN_SUSPEND mode when
lock_system_sleep() lock is taken, meaning that we can't initialize
hibernation.

I see. Sounds plausible. I will fix both the code and commit message
for better readability. Thanks for catching this.

quoted

    > +
    > +     xrfp.domid = DOMID_SELF;
    > +     xrfp.gpfn = __pa(HYPERVISOR_shared_info) >> PAGE_SHIFT;
    > +
    > +     ret = HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrfp);
    > +     if (!ret)
    > +             HYPERVISOR_shared_info = &xen_dummy_shared_info;
    > +
    > +     return ret;
    > +}
    > +
    > +static void xen_syscore_resume(void)
    > +{
    > +     /* Xen suspend does similar stuffs in its own logic */
    > +     if (xen_suspend_mode_is_xen_suspend())
    > +             return;
    > +
    > +     /* No need to setup vcpu_info as it's already moved off */
    > +     xen_hvm_map_shared_info();
    > +
    > +     pvclock_resume();
    > +
    > +     gnttab_resume();


    Do you call gnttab_suspend() in pm suspend path?
No, since it does nothing for HVM guests. The unmap_frames is only applicable for PV guests right?

You should call it nevertheless. It will decide whether or not anything
needs to be done.

Will fix it in V2.

-boris

Thanks,
Anchal