Re: [Xen-devel] [PATCH for-next v2 7/9] x86: switch xen implementation to use hypervisor framework

["Roger Pau Monné" <roger.pau@citrix.com>]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel