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:
> > 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 <kamatam@xxxxxxxxxx>
> > >
> > > 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.
>
> > > +
> > > + 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
>
