Re: [Xen-devel] [PATCH for-next v3 5/9] x86: introduce hypervisor framework

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.10.2019 17:57, Wei Liu wrote:
> --- /dev/null
> +++ b/xen/arch/x86/guest/hypervisor.c
> @@ -0,0 +1,45 @@
> +/******************************************************************************
> + * arch/x86/guest/hypervisor.c
> + *
> + * Support for detecting and running under a hypervisor.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Copyright (c) 2019 Microsoft.
> + */
> +
> +#include <xen/types.h>
> +
> +#include <asm/cache.h>
> +#include <asm/guest/hypervisor.h>
> +
> +static struct hypervisor_ops *hops __read_mostly;

The __read_mostly wants to again go between type and identifier.

> +bool hypervisor_probe(void)
> +{
> +    if ( hops )
> +        return true;
> +
> +    return false;

I assume this isn't simply "return hops" because more is going to be
added here?

> --- /dev/null
> +++ b/xen/include/asm-x86/guest/hypervisor.h
> @@ -0,0 +1,61 @@
> +/******************************************************************************
> + * asm-x86/guest/hypervisor.h
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; If not, see 
> <http://www.gnu.org/licenses/>.
> + *
> + * Copyright (c) 2019 Microsoft.
> + */
> +
> +#ifndef __X86_HYPERVISOR_H__
> +#define __X86_HYPERVISOR_H__
> +
> +#ifdef CONFIG_GUEST
> +
> +struct hypervisor_ops {
> +    /* Name of the hypervisor */
> +    const char *name;
> +    /* Main setup routine */
> +    void (*setup)(void);
> +    /* AP setup */
> +    void (*ap_setup)(void);
> +    /* Resume from suspension */
> +    void (*resume)(void);

None of these fields look to be used, despite ...

> +};
> +
> +bool hypervisor_probe(void);
> +void hypervisor_setup(void);
> +void hypervisor_ap_setup(void);
> +void hypervisor_resume(void);

... the latter three of these being the apparent wrappers for
them. Perhaps just a side effect of how exactly this series
was split up, so not strictly a request to change anything.

> @@ -49,15 +46,6 @@ DECLARE_PER_CPU(struct vcpu_info *, vcpu_info);
>  
>  static inline void probe_hypervisor(void) {}
>  
> -static inline void hypervisor_setup(void)
> -{
> -    ASSERT_UNREACHABLE();
> -}
> -static inline void hypervisor_ap_setup(void)
> -{
> -    ASSERT_UNREACHABLE();
> -}

Why did the ASSERT_UNREACHABLE() get lost?

Jan

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