| Message ID | 20190930150044.5734-8-liuwe@microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Port Xen to Hyper-V | expand |
On Mon, Sep 30, 2019 at 04:00:41PM +0100, Wei Liu wrote: > Take the chance to change probe_hypervisor to hypervisor_probe. The implementation LGTM. > > Signed-off-by: Wei Liu <liuwe@microsoft.com> > --- > xen/arch/x86/guest/hypervisor.c | 31 +++++++++++++++++++++++++++++-- > xen/arch/x86/guest/xen/pvh-boot.c | 2 +- > xen/arch/x86/guest/xen/xen.c | 26 ++++++++++++++------------ > xen/arch/x86/setup.c | 2 +- > xen/include/asm-x86/guest/xen.h | 6 ++++-- > 5 files changed, 49 insertions(+), 18 deletions(-) > > diff --git a/xen/arch/x86/guest/hypervisor.c b/xen/arch/x86/guest/hypervisor.c > index 89b9ae4de0..30453b6a7a 100644 > --- a/xen/arch/x86/guest/hypervisor.c > +++ b/xen/arch/x86/guest/hypervisor.c > @@ -22,7 +22,7 @@ > #include <xen/types.h> > > #include <asm/cache.h> > -#include <asm/guest/hypervisor.h> > +#include <asm/guest.h> > > static struct hypervisor_ops *hops __read_mostly; > > @@ -31,7 +31,34 @@ bool hypervisor_probe(void) > if ( hops ) > return true; > > - return false; > + /* Too early to use cpu_has_hypervisor */ > + if ( !(cpuid_ecx(1) & cpufeat_mask(X86_FEATURE_HYPERVISOR)) ) > + return false; > + > +#ifdef CONFIG_XEN_GUEST > + if ( xen_probe() ) > + hops = &xen_hypervisor_ops; > +#endif I think you likely want something like: if ( xen_probe() ) { hops = &xen_hypervisor_ops; return true; } if ( hyperv_probe() ) { .... return true; } return false; In order to return after a successful probe, or else you lose cycles probing for hypervisors when a match has been found, and also in the Xen case you risk detecting the HyperV support in Xen and thus using that instead of the Xen one. Long term if we gain more guests support I would likely want to see hypervisor_ops turning into an array and gaining a probe function so that this can be done in a loop instead of having this function. > + > + return !!hops; > +} > + > +void 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(); > } > > /* > 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 9895025d02..a9d321e5eb 100644 > --- a/xen/arch/x86/guest/xen/xen.c > +++ b/xen/arch/x86/guest/xen/xen.c > @@ -67,24 +67,19 @@ static void __init find_xen_leaves(void) > } > } > > -void __init probe_hypervisor(void) > +bool __init xen_probe(void) > { > - if ( xen_guest ) > - return; > - > - /* Too early to use cpu_has_hypervisor */ > - if ( !(cpuid_ecx(1) & cpufeat_mask(X86_FEATURE_HYPERVISOR)) ) > - return; > - > find_xen_leaves(); > > if ( !xen_cpuid_base ) > - return; > + return false; > > /* Fill the hypercall page. */ > wrmsrl(cpuid_ebx(xen_cpuid_base + 2), __pa(hypercall_page)); > > xen_guest = true; > + > + return true; > } > > static void map_shared_info(void) > @@ -249,7 +244,7 @@ static void init_evtchn(void) > } > } > > -void __init hypervisor_setup(void) > +void __init xen_setup(void) > { > init_memmap(); > > @@ -277,7 +272,7 @@ void __init hypervisor_setup(void) > init_evtchn(); > } > > -void hypervisor_ap_setup(void) > +void xen_ap_setup(void) > { > set_vcpu_id(); > map_vcpuinfo(); > @@ -307,7 +302,7 @@ static void ap_resume(void *unused) > init_evtchn(); > } > > -void hypervisor_resume(void) > +void xen_resume(void) I think xen_{setup/ap_setup/resume} can be made static now? Thanks, Roger.
On Mon, Oct 21, 2019 at 11:56:36AM +0200, Roger Pau Monné wrote: [...] > > static struct hypervisor_ops *hops __read_mostly; > > > > @@ -31,7 +31,34 @@ bool hypervisor_probe(void) > > if ( hops ) > > return true; > > > > - return false; > > + /* Too early to use cpu_has_hypervisor */ > > + if ( !(cpuid_ecx(1) & cpufeat_mask(X86_FEATURE_HYPERVISOR)) ) > > + return false; > > + > > +#ifdef CONFIG_XEN_GUEST > > + if ( xen_probe() ) > > + hops = &xen_hypervisor_ops; > > +#endif > > I think you likely want something like: > > if ( xen_probe() ) > { > hops = &xen_hypervisor_ops; > return true; > } > if ( hyperv_probe() ) > { > .... > return true; > } > > return false; > > In order to return after a successful probe, or else you lose cycles > probing for hypervisors when a match has been found, and also in the > Xen case you risk detecting the HyperV support in Xen and thus using > that instead of the Xen one. > Good point. > Long term if we gain more guests support I would likely want to see > hypervisor_ops turning into an array and gaining a probe function so > that this can be done in a loop instead of having this function. > That was my plan from the get-go but Xen lacked appropriate infrastructure, hence I resorted to something akin to HVM hooks. [...] > > -void __init hypervisor_setup(void) > > +void __init xen_setup(void) > > { > > init_memmap(); > > > > @@ -277,7 +272,7 @@ void __init hypervisor_setup(void) > > init_evtchn(); > > } > > > > -void hypervisor_ap_setup(void) > > +void xen_ap_setup(void) > > { > > set_vcpu_id(); > > map_vcpuinfo(); > > @@ -307,7 +302,7 @@ static void ap_resume(void *unused) > > init_evtchn(); > > } > > > > -void hypervisor_resume(void) > > +void xen_resume(void) > > I think xen_{setup/ap_setup/resume} can be made static now? Indeed. I will fix this. Wei. > > Thanks, Roger.
diff --git a/xen/arch/x86/guest/hypervisor.c b/xen/arch/x86/guest/hypervisor.c index 89b9ae4de0..30453b6a7a 100644 --- a/xen/arch/x86/guest/hypervisor.c +++ b/xen/arch/x86/guest/hypervisor.c @@ -22,7 +22,7 @@ #include <xen/types.h> #include <asm/cache.h> -#include <asm/guest/hypervisor.h> +#include <asm/guest.h> static struct hypervisor_ops *hops __read_mostly; @@ -31,7 +31,34 @@ bool hypervisor_probe(void) if ( hops ) return true; - return false; + /* Too early to use cpu_has_hypervisor */ + if ( !(cpuid_ecx(1) & cpufeat_mask(X86_FEATURE_HYPERVISOR)) ) + return false; + +#ifdef CONFIG_XEN_GUEST + if ( xen_probe() ) + hops = &xen_hypervisor_ops; +#endif + + return !!hops; +} + +void 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(); } /* 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 9895025d02..a9d321e5eb 100644 --- a/xen/arch/x86/guest/xen/xen.c +++ b/xen/arch/x86/guest/xen/xen.c @@ -67,24 +67,19 @@ static void __init find_xen_leaves(void) } } -void __init probe_hypervisor(void) +bool __init xen_probe(void) { - if ( xen_guest ) - return; - - /* Too early to use cpu_has_hypervisor */ - if ( !(cpuid_ecx(1) & cpufeat_mask(X86_FEATURE_HYPERVISOR)) ) - return; - find_xen_leaves(); if ( !xen_cpuid_base ) - return; + return false; /* Fill the hypercall page. */ wrmsrl(cpuid_ebx(xen_cpuid_base + 2), __pa(hypercall_page)); xen_guest = true; + + return true; } static void map_shared_info(void) @@ -249,7 +244,7 @@ static void init_evtchn(void) } } -void __init hypervisor_setup(void) +void __init xen_setup(void) { init_memmap(); @@ -277,7 +272,7 @@ void __init hypervisor_setup(void) init_evtchn(); } -void hypervisor_ap_setup(void) +void xen_ap_setup(void) { set_vcpu_id(); map_vcpuinfo(); @@ -307,7 +302,7 @@ static void ap_resume(void *unused) init_evtchn(); } -void hypervisor_resume(void) +void xen_resume(void) { /* Reset shared info page. */ map_shared_info(); @@ -330,6 +325,13 @@ void hypervisor_resume(void) pv_console_init(); } +struct hypervisor_ops xen_hypervisor_ops = { + .name = "Xen", + .setup = xen_setup, + .ap_setup = xen_ap_setup, + .resume = xen_resume, +}; + /* * Local variables: * mode: C diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index dec60d0301..0ee11b15a6 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 8221fc1325..7401c80a1a 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)) @@ -31,8 +32,9 @@ extern bool xen_guest; extern bool pv_console; extern uint32_t xen_cpuid_base; +extern struct hypervisor_ops xen_hypervisor_ops; -void probe_hypervisor(void); +bool xen_probe(void); int xen_alloc_unused_page(mfn_t *mfn); int xen_free_unused_page(mfn_t mfn); @@ -44,7 +46,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 bool xen_probe(void) { return false; } #endif /* CONFIG_XEN_GUEST */ #endif /* __X86_GUEST_XEN_H__ */
Take the chance to change probe_hypervisor to hypervisor_probe. Signed-off-by: Wei Liu <liuwe@microsoft.com> --- xen/arch/x86/guest/hypervisor.c | 31 +++++++++++++++++++++++++++++-- xen/arch/x86/guest/xen/pvh-boot.c | 2 +- xen/arch/x86/guest/xen/xen.c | 26 ++++++++++++++------------ xen/arch/x86/setup.c | 2 +- xen/include/asm-x86/guest/xen.h | 6 ++++-- 5 files changed, 49 insertions(+), 18 deletions(-)