| Message ID | 529f544a64bb93b920bf86b1d3f86d93b0a4219b.1589926004.git.anchalag@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix PM hibernation in Xen guests | expand |
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
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 > >
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
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 >
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
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);
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(+)