[03/12] x86/xen: Introduce new function to map HYPERVISOR_shared_info on Resume
diff mbox series

Message ID 529f544a64bb93b920bf86b1d3f86d93b0a4219b.1589926004.git.anchalag@amazon.com
State Superseded
Headers show
Series
  • Fix PM hibernation in Xen guests
Related show

Commit Message

Agarwal, Anchal May 19, 2020, 11:25 p.m. UTC
Introduce a small function which re-uses shared page's PA allocated
during guest initialization time in reserve_shared_info() and not
allocate new page during resume flow.
It also  does the mapping of shared_info_page by calling
xen_hvm_init_shared_info() to use the function.

Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
---
 arch/x86/xen/enlighten_hvm.c | 7 +++++++
 arch/x86/xen/xen-ops.h       | 1 +
 2 files changed, 8 insertions(+)

Comments

Boris Ostrovsky May 30, 2020, 11:02 p.m. UTC | #1
On 5/19/20 7:25 PM, Anchal Agarwal wrote:
> Introduce a small function which re-uses shared page's PA allocated
> during guest initialization time in reserve_shared_info() and not
> allocate new page during resume flow.
> It also  does the mapping of shared_info_page by calling
> xen_hvm_init_shared_info() to use the function.
>
> Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
> ---
>  arch/x86/xen/enlighten_hvm.c | 7 +++++++
>  arch/x86/xen/xen-ops.h       | 1 +
>  2 files changed, 8 insertions(+)
>
> diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
> index e138f7de52d2..75b1ec7a0fcd 100644
> --- a/arch/x86/xen/enlighten_hvm.c
> +++ b/arch/x86/xen/enlighten_hvm.c
> @@ -27,6 +27,13 @@
>  
>  static unsigned long shared_info_pfn;
>  
> +void xen_hvm_map_shared_info(void)
> +{
> +	xen_hvm_init_shared_info();
> +	if (shared_info_pfn)
> +		HYPERVISOR_shared_info = __va(PFN_PHYS(shared_info_pfn));
> +}
> +


AFAICT it is only called once so I don't see a need for new routine.


And is it possible for shared_info_pfn to be NULL in resume path (which
is where this is called)?


-boris
Agarwal, Anchal June 4, 2020, 11:03 p.m. UTC | #2
On Sat, May 30, 2020 at 07:02:01PM -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 5/19/20 7:25 PM, Anchal Agarwal wrote:
> > Introduce a small function which re-uses shared page's PA allocated
> > during guest initialization time in reserve_shared_info() and not
> > allocate new page during resume flow.
> > It also  does the mapping of shared_info_page by calling
> > xen_hvm_init_shared_info() to use the function.
> >
> > Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
> > ---
> >  arch/x86/xen/enlighten_hvm.c | 7 +++++++
> >  arch/x86/xen/xen-ops.h       | 1 +
> >  2 files changed, 8 insertions(+)
> >
> > diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
> > index e138f7de52d2..75b1ec7a0fcd 100644
> > --- a/arch/x86/xen/enlighten_hvm.c
> > +++ b/arch/x86/xen/enlighten_hvm.c
> > @@ -27,6 +27,13 @@
> >
> >  static unsigned long shared_info_pfn;
> >
> > +void xen_hvm_map_shared_info(void)
> > +{
> > +     xen_hvm_init_shared_info();
> > +     if (shared_info_pfn)
> > +             HYPERVISOR_shared_info = __va(PFN_PHYS(shared_info_pfn));
> > +}
> > +
> 
> 
> AFAICT it is only called once so I don't see a need for new routine.
> 
> 
HYPERVISOR_shared_info can only be mapped in this scope without refactoring
much of the code.
> And is it possible for shared_info_pfn to be NULL in resume path (which
> is where this is called)?
> 
> 
I don't think it should be, still a sanity check but I don't think its needed there
because hibernation will fail in any case if thats the case. 
However, HYPERVISOR_shared_info does needs to be re-mapped on resume as its been
marked to dummy address on suspend. Its also safe in case va changes.
Does the answer your question?
> -boris

-Anchal
> 
>
Boris Ostrovsky June 5, 2020, 9:39 p.m. UTC | #3
On 6/4/20 7:03 PM, Anchal Agarwal wrote:
> On Sat, May 30, 2020 at 07:02:01PM -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 5/19/20 7:25 PM, Anchal Agarwal wrote:
>>> Introduce a small function which re-uses shared page's PA allocated
>>> during guest initialization time in reserve_shared_info() and not
>>> allocate new page during resume flow.
>>> It also  does the mapping of shared_info_page by calling
>>> xen_hvm_init_shared_info() to use the function.
>>>
>>> Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
>>> ---
>>>  arch/x86/xen/enlighten_hvm.c | 7 +++++++
>>>  arch/x86/xen/xen-ops.h       | 1 +
>>>  2 files changed, 8 insertions(+)
>>>
>>> diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
>>> index e138f7de52d2..75b1ec7a0fcd 100644
>>> --- a/arch/x86/xen/enlighten_hvm.c
>>> +++ b/arch/x86/xen/enlighten_hvm.c
>>> @@ -27,6 +27,13 @@
>>>
>>>  static unsigned long shared_info_pfn;
>>>
>>> +void xen_hvm_map_shared_info(void)
>>> +{
>>> +     xen_hvm_init_shared_info();
>>> +     if (shared_info_pfn)
>>> +             HYPERVISOR_shared_info = __va(PFN_PHYS(shared_info_pfn));
>>> +}
>>> +
>>
>> AFAICT it is only called once so I don't see a need for new routine.
>>
>>
> HYPERVISOR_shared_info can only be mapped in this scope without refactoring
> much of the code.


Refactoring what? All am suggesting is

--- a/arch/x86/xen/suspend.c
+++ b/arch/x86/xen/suspend.c
@@ -124,7 +124,9 @@ static void xen_syscore_resume(void)
                return;
 
        /* No need to setup vcpu_info as it's already moved off */
-       xen_hvm_map_shared_info();
+       xen_hvm_init_shared_info();
+       if (shared_info_pfn)
+               HYPERVISOR_shared_info = __va(PFN_PHYS(shared_info_pfn));
 
        pvclock_resume();

>> And is it possible for shared_info_pfn to be NULL in resume path (which
>> is where this is called)?
>>
>>
> I don't think it should be, still a sanity check but I don't think its needed there
> because hibernation will fail in any case if thats the case. 


If shared_info_pfn is NULL you'd have problems long before hibernation
started. We set it in xen_hvm_guest_init() and never touch again.


In fact, I'd argue that it should be __ro_after_init.


> However, HYPERVISOR_shared_info does needs to be re-mapped on resume as its been
> marked to dummy address on suspend. Its also safe in case va changes.
> Does the answer your question?


I wasn't arguing whether HYPERVISOR_shared_info needs to be set, I was
only saying that shared_info_pfn doesn't need to be tested.


-boris
Agarwal, Anchal June 8, 2020, 4:52 p.m. UTC | #4
On Fri, Jun 05, 2020 at 05:39:54PM -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/4/20 7:03 PM, Anchal Agarwal wrote:
> > On Sat, May 30, 2020 at 07:02:01PM -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 5/19/20 7:25 PM, Anchal Agarwal wrote:
> >>> Introduce a small function which re-uses shared page's PA allocated
> >>> during guest initialization time in reserve_shared_info() and not
> >>> allocate new page during resume flow.
> >>> It also  does the mapping of shared_info_page by calling
> >>> xen_hvm_init_shared_info() to use the function.
> >>>
> >>> Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
> >>> ---
> >>>  arch/x86/xen/enlighten_hvm.c | 7 +++++++
> >>>  arch/x86/xen/xen-ops.h       | 1 +
> >>>  2 files changed, 8 insertions(+)
> >>>
> >>> diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
> >>> index e138f7de52d2..75b1ec7a0fcd 100644
> >>> --- a/arch/x86/xen/enlighten_hvm.c
> >>> +++ b/arch/x86/xen/enlighten_hvm.c
> >>> @@ -27,6 +27,13 @@
> >>>
> >>>  static unsigned long shared_info_pfn;
> >>>
> >>> +void xen_hvm_map_shared_info(void)
> >>> +{
> >>> +     xen_hvm_init_shared_info();
> >>> +     if (shared_info_pfn)
> >>> +             HYPERVISOR_shared_info = __va(PFN_PHYS(shared_info_pfn));
> >>> +}
> >>> +
> >>
> >> AFAICT it is only called once so I don't see a need for new routine.
> >>
> >>
> > HYPERVISOR_shared_info can only be mapped in this scope without refactoring
> > much of the code.
> 
> 
> Refactoring what? All am suggesting is
>
shared_info_pfn does not seem to be in scope here, it's scope is limited
to enlighten_hvm.c. That's the reason I introduced a new function there.

> --- a/arch/x86/xen/suspend.c
> +++ b/arch/x86/xen/suspend.c
> @@ -124,7 +124,9 @@ static void xen_syscore_resume(void)
>                 return;
> 
>         /* No need to setup vcpu_info as it's already moved off */
> -       xen_hvm_map_shared_info();
> +       xen_hvm_init_shared_info();
> +       if (shared_info_pfn)
> +               HYPERVISOR_shared_info = __va(PFN_PHYS(shared_info_pfn));
> 
>         pvclock_resume();
> 
> >> And is it possible for shared_info_pfn to be NULL in resume path (which
> >> is where this is called)?
> >>
> >>
> > I don't think it should be, still a sanity check but I don't think its needed there
> > because hibernation will fail in any case if thats the case.
> 
> 
> If shared_info_pfn is NULL you'd have problems long before hibernation
> started. We set it in xen_hvm_guest_init() and never touch again.
> 
> 
> In fact, I'd argue that it should be __ro_after_init.
> 
> 
I agree, and I should have mentioned that I will remove that check and its not
necessary as this gets mapped way early in the boot process.
> > However, HYPERVISOR_shared_info does needs to be re-mapped on resume as its been
> > marked to dummy address on suspend. Its also safe in case va changes.
> > Does the answer your question?
> 
> 
> I wasn't arguing whether HYPERVISOR_shared_info needs to be set, I was
> only saying that shared_info_pfn doesn't need to be tested.
> 
Got it. :)
> 
> -boris
> 
Thanks,
Anchal
>
Boris Ostrovsky June 8, 2020, 6:49 p.m. UTC | #5
On 6/8/20 12:52 PM, Anchal Agarwal wrote:
>
>>>>> +void xen_hvm_map_shared_info(void)
>>>>> +{
>>>>> +     xen_hvm_init_shared_info();
>>>>> +     if (shared_info_pfn)
>>>>> +             HYPERVISOR_shared_info = __va(PFN_PHYS(shared_info_pfn));
>>>>> +}
>>>>> +
>>>> AFAICT it is only called once so I don't see a need for new routine.
>>>>
>>>>
>>> HYPERVISOR_shared_info can only be mapped in this scope without refactoring
>>> much of the code.
>>
>> Refactoring what? All am suggesting is
>>
> shared_info_pfn does not seem to be in scope here, it's scope is limited
> to enlighten_hvm.c. That's the reason I introduced a new function there.


OK, that's a good point.


-boris

Patch
diff mbox series

diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
index e138f7de52d2..75b1ec7a0fcd 100644
--- a/arch/x86/xen/enlighten_hvm.c
+++ b/arch/x86/xen/enlighten_hvm.c
@@ -27,6 +27,13 @@ 
 
 static unsigned long shared_info_pfn;
 
+void xen_hvm_map_shared_info(void)
+{
+	xen_hvm_init_shared_info();
+	if (shared_info_pfn)
+		HYPERVISOR_shared_info = __va(PFN_PHYS(shared_info_pfn));
+}
+
 void xen_hvm_init_shared_info(void)
 {
 	struct xen_add_to_physmap xatp;
diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
index 45a441c33d6d..d84c357994bd 100644
--- a/arch/x86/xen/xen-ops.h
+++ b/arch/x86/xen/xen-ops.h
@@ -56,6 +56,7 @@  void xen_enable_syscall(void);
 void xen_vcpu_restore(void);
 
 void xen_callback_vector(void);
+void xen_hvm_map_shared_info(void);
 void xen_hvm_init_shared_info(void);
 void xen_unplug_emulated_devices(void);