| Message ID | 20191121185049.16666-7-liuwe@microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Port Xen to Hyper-V | expand |
> -----Original Message----- > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Wei > Liu > Sent: 21 November 2019 19:51 > To: Xen Development List <xen-devel@lists.xenproject.org> > Cc: Wei Liu <liuwe@microsoft.com>; Wei Liu <wl@xen.org>; Andrew Cooper > <andrew.cooper3@citrix.com>; Michael Kelley <mikelley@microsoft.com>; Jan > Beulich <jbeulich@suse.com>; Roger Pau Monné <roger.pau@citrix.com> > Subject: [Xen-devel] [PATCH v4 6/8] x86: switch xen guest implementation > to use hypervisor framework > > Signed-off-by: Wei Liu <liuwe@microsoft.com> [snip] > diff --git a/xen/include/asm-x86/guest/xen.h b/xen/include/asm- > x86/guest/xen.h > index 01dc3ee6f6..db90b550a7 100644 > --- a/xen/include/asm-x86/guest/xen.h > +++ b/xen/include/asm-x86/guest/xen.h > @@ -23,6 +23,7 @@ > > #include <asm/e820.h> > #include <asm/fixmap.h> > +#include <asm/guest/hypervisor.h> > > #define XEN_shared_info ((struct shared_info > *)fix_to_virt(FIX_XEN_SHARED_INFO)) > > @@ -32,7 +33,7 @@ extern bool xen_guest; > extern bool pv_console; > extern uint32_t xen_cpuid_base; > > -void probe_hypervisor(void); > +const struct hypervisor_ops *xen_probe(void); > int xg_alloc_unused_page(mfn_t *mfn); > int xg_free_unused_page(mfn_t mfn); > > @@ -44,7 +45,7 @@ DECLARE_PER_CPU(struct vcpu_info *, vcpu_info); > #define xen_guest 0 > #define pv_console 0 Nit: These should be #defined to false rather than 0. The rest LGTM so with those fixed, Reviewed-by: Paul Durrant <pdurrant@amazon.com> > > -static inline void probe_hypervisor(void) {} > +static inline const struct hypervisor_ops *xen_probe(void) { return NULL; > } > > #endif /* CONFIG_XEN_GUEST */ > #endif /* __X86_GUEST_XEN_H__ */ > -- > 2.20.1 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel
On Fri, Nov 22, 2019 at 11:02:30AM +0000, Durrant, Paul wrote: > > -----Original Message----- > > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Wei > > Liu > > Sent: 21 November 2019 19:51 > > To: Xen Development List <xen-devel@lists.xenproject.org> > > Cc: Wei Liu <liuwe@microsoft.com>; Wei Liu <wl@xen.org>; Andrew Cooper > > <andrew.cooper3@citrix.com>; Michael Kelley <mikelley@microsoft.com>; Jan > > Beulich <jbeulich@suse.com>; Roger Pau Monné <roger.pau@citrix.com> > > Subject: [Xen-devel] [PATCH v4 6/8] x86: switch xen guest implementation > > to use hypervisor framework > > > > Signed-off-by: Wei Liu <liuwe@microsoft.com> > [snip] > > diff --git a/xen/include/asm-x86/guest/xen.h b/xen/include/asm- > > x86/guest/xen.h > > index 01dc3ee6f6..db90b550a7 100644 > > --- a/xen/include/asm-x86/guest/xen.h > > +++ b/xen/include/asm-x86/guest/xen.h > > @@ -23,6 +23,7 @@ > > > > #include <asm/e820.h> > > #include <asm/fixmap.h> > > +#include <asm/guest/hypervisor.h> > > > > #define XEN_shared_info ((struct shared_info > > *)fix_to_virt(FIX_XEN_SHARED_INFO)) > > > > @@ -32,7 +33,7 @@ extern bool xen_guest; > > extern bool pv_console; > > extern uint32_t xen_cpuid_base; > > > > -void probe_hypervisor(void); > > +const struct hypervisor_ops *xen_probe(void); > > int xg_alloc_unused_page(mfn_t *mfn); > > int xg_free_unused_page(mfn_t mfn); > > > > @@ -44,7 +45,7 @@ DECLARE_PER_CPU(struct vcpu_info *, vcpu_info); > > #define xen_guest 0 > > #define pv_console 0 > > Nit: These should be #defined to false rather than 0. The rest LGTM so with those fixed, They were part of existing code so I didn't touch them. Wei.
On 21.11.2019 19:50, Wei Liu wrote: > +void __init hypervisor_setup(void) > +{ > + if ( hops && hops->setup ) > + hops->setup(); > +} > + > +void hypervisor_ap_setup(void) > +{ > + if ( hops && hops->ap_setup ) > + hops->ap_setup(); > +} > + > +void hypervisor_resume(void) > +{ > + if ( hops && hops->resume ) > + hops->resume(); > +} I assume these don't get executed frequently enough for alternatives indirect call patching to matter? Down the road, if any more frequently executed hooks should appear, we will want to switch to that model though. This has the added benefit of there then being only one runtime instance of struct hypervisor_ops; all per-hypervisor ones would become __initconstrel. > @@ -326,6 +310,31 @@ void hypervisor_resume(void) > pv_console_init(); > } > > +static const struct hypervisor_ops xg_ops = { With this new xg_ prefix, ... > + .name = "Xen", > + .setup = xen_setup, > + .ap_setup = xen_ap_setup, > + .resume = xen_resume, ... I'd like to suggest to use it for these ones too (if they need to have a prefix in the first place, which I doubt, but which I know others view differently). > +const struct hypervisor_ops * __init xen_probe(void) For this one the call is a little more difficult, but for consistency I think xg_ would be slightly better here as well. Nit: Strictly speaking there's a stray blank after *. We normally have ones only ahead of *, but not after. Jan
On Fri, Nov 29, 2019 at 02:50:18PM +0100, Jan Beulich wrote: > On 21.11.2019 19:50, Wei Liu wrote: > > +void __init hypervisor_setup(void) > > +{ > > + if ( hops && hops->setup ) > > + hops->setup(); > > +} > > + > > +void hypervisor_ap_setup(void) > > +{ > > + if ( hops && hops->ap_setup ) > > + hops->ap_setup(); > > +} > > + > > +void hypervisor_resume(void) > > +{ > > + if ( hops && hops->resume ) > > + hops->resume(); > > +} > > I assume these don't get executed frequently enough for > alternatives indirect call patching to matter? Down the road, if > any more frequently executed hooks should appear, we will want > to switch to that model though. This has the added benefit of > there then being only one runtime instance of struct > hypervisor_ops; all per-hypervisor ones would become > __initconstrel. > All the routines listed here are run very infrequently. I agree that when more frequently executed hooks appear we will want to look into alternatives. > > @@ -326,6 +310,31 @@ void hypervisor_resume(void) > > pv_console_init(); > > } > > > > +static const struct hypervisor_ops xg_ops = { > > With this new xg_ prefix, ... > > > + .name = "Xen", > > + .setup = xen_setup, > > + .ap_setup = xen_ap_setup, > > + .resume = xen_resume, > > ... I'd like to suggest to use it for these ones too (if they > need to have a prefix in the first place, which I doubt, but > which I know others view differently). > Since they are only visible to this file I think not having a prefix should be fine. > > +const struct hypervisor_ops * __init xen_probe(void) > > For this one the call is a little more difficult, but for > consistency I think xg_ would be slightly better here as well. No problem. Changed it to xg_probe. > > Nit: Strictly speaking there's a stray blank after *. We > normally have ones only ahead of *, but not after. Fixed. Wei. > > Jan
diff --git a/xen/arch/x86/guest/hypervisor.c b/xen/arch/x86/guest/hypervisor.c index 103feba5d8..a067cacecb 100644 --- a/xen/arch/x86/guest/hypervisor.c +++ b/xen/arch/x86/guest/hypervisor.c @@ -19,18 +19,48 @@ * Copyright (c) 2019 Microsoft. */ +#include <xen/init.h> #include <xen/types.h> #include <asm/cache.h> -#include <asm/guest/hypervisor.h> +#include <asm/guest.h> static const struct hypervisor_ops __read_mostly *hops; const struct hypervisor_ops *hypervisor_probe(void) { + if ( hops ) + goto out; + + if ( !cpu_has_hypervisor ) + goto out; + + hops = xen_probe(); + if ( hops ) + goto out; + + out: return hops; } +void __init hypervisor_setup(void) +{ + if ( hops && hops->setup ) + hops->setup(); +} + +void hypervisor_ap_setup(void) +{ + if ( hops && hops->ap_setup ) + hops->ap_setup(); +} + +void hypervisor_resume(void) +{ + if ( hops && hops->resume ) + hops->resume(); +} + /* * Local variables: * mode: C diff --git a/xen/arch/x86/guest/xen/pvh-boot.c b/xen/arch/x86/guest/xen/pvh-boot.c index ca8e156f7d..498625eae0 100644 --- a/xen/arch/x86/guest/xen/pvh-boot.c +++ b/xen/arch/x86/guest/xen/pvh-boot.c @@ -103,7 +103,7 @@ void __init pvh_init(multiboot_info_t **mbi, module_t **mod) { convert_pvh_info(mbi, mod); - probe_hypervisor(); + hypervisor_probe(); ASSERT(xen_guest); get_memory_map(); diff --git a/xen/arch/x86/guest/xen/xen.c b/xen/arch/x86/guest/xen/xen.c index 0f5b5267c5..8cfe059887 100644 --- a/xen/arch/x86/guest/xen/xen.c +++ b/xen/arch/x86/guest/xen/xen.c @@ -67,22 +67,6 @@ static void __init find_xen_leaves(void) } } -void __init probe_hypervisor(void) -{ - if ( xen_guest || !cpu_has_hypervisor ) - return; - - find_xen_leaves(); - - if ( !xen_cpuid_base ) - return; - - /* Fill the hypercall page. */ - wrmsrl(cpuid_ebx(xen_cpuid_base + 2), __pa(hypercall_page)); - - xen_guest = true; -} - static void map_shared_info(void) { mfn_t mfn; @@ -245,7 +229,7 @@ static void init_evtchn(void) } } -void __init hypervisor_setup(void) +static void __init xen_setup(void) { init_memmap(); @@ -273,7 +257,7 @@ void __init hypervisor_setup(void) init_evtchn(); } -void hypervisor_ap_setup(void) +static void xen_ap_setup(void) { set_vcpu_id(); map_vcpuinfo(); @@ -303,7 +287,7 @@ static void ap_resume(void *unused) init_evtchn(); } -void hypervisor_resume(void) +static void xen_resume(void) { /* Reset shared info page. */ map_shared_info(); @@ -326,6 +310,31 @@ void hypervisor_resume(void) pv_console_init(); } +static const struct hypervisor_ops xg_ops = { + .name = "Xen", + .setup = xen_setup, + .ap_setup = xen_ap_setup, + .resume = xen_resume, +}; + +const struct hypervisor_ops * __init xen_probe(void) +{ + if ( xen_guest ) + return &xg_ops; + + find_xen_leaves(); + + if ( !xen_cpuid_base ) + return NULL; + + /* Fill the hypercall page. */ + wrmsrl(cpuid_ebx(xen_cpuid_base + 2), __pa(hypercall_page)); + + xen_guest = true; + + return &xg_ops; +} + /* * Local variables: * mode: C diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 00ee87bde5..19606d909b 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -763,7 +763,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) * allocing any xenheap structures wanted in lower memory. */ kexec_early_calculations(); - probe_hypervisor(); + hypervisor_probe(); parse_video_info(); diff --git a/xen/include/asm-x86/guest/xen.h b/xen/include/asm-x86/guest/xen.h index 01dc3ee6f6..db90b550a7 100644 --- a/xen/include/asm-x86/guest/xen.h +++ b/xen/include/asm-x86/guest/xen.h @@ -23,6 +23,7 @@ #include <asm/e820.h> #include <asm/fixmap.h> +#include <asm/guest/hypervisor.h> #define XEN_shared_info ((struct shared_info *)fix_to_virt(FIX_XEN_SHARED_INFO)) @@ -32,7 +33,7 @@ extern bool xen_guest; extern bool pv_console; extern uint32_t xen_cpuid_base; -void probe_hypervisor(void); +const struct hypervisor_ops *xen_probe(void); int xg_alloc_unused_page(mfn_t *mfn); int xg_free_unused_page(mfn_t mfn); @@ -44,7 +45,7 @@ DECLARE_PER_CPU(struct vcpu_info *, vcpu_info); #define xen_guest 0 #define pv_console 0 -static inline void probe_hypervisor(void) {} +static inline const struct hypervisor_ops *xen_probe(void) { return NULL; } #endif /* CONFIG_XEN_GUEST */ #endif /* __X86_GUEST_XEN_H__ */
Signed-off-by: Wei Liu <liuwe@microsoft.com> --- Changes in v4: 1. xen_probe returns address of ops directly. 2. Add __init to hypervisor_setup. 3. Drop Paul's review tag. --- xen/arch/x86/guest/hypervisor.c | 32 ++++++++++++++++++++- xen/arch/x86/guest/xen/pvh-boot.c | 2 +- xen/arch/x86/guest/xen/xen.c | 47 ++++++++++++++++++------------- xen/arch/x86/setup.c | 2 +- xen/include/asm-x86/guest/xen.h | 5 ++-- 5 files changed, 64 insertions(+), 24 deletions(-)