Re: [Xen-devel] [PATCH for-next RFC 5/8] x86: factor out hypervisor agnostic code

[Wei Liu <wl@xxxxxxx>]

On Wed, Sep 25, 2019 at 12:39:11PM +0200, Roger Pau Monné wrote:
> On Mon, Sep 23, 2019 at 11:09:28AM +0100, Wei Liu wrote:
> > The only implementation there is Xen.
> > 
> > No functional change.
> > 
> > Signed-off-by: Wei Liu <liuwe@xxxxxxxxxxxxx>
> > ---
> >  xen/arch/x86/guest/Makefile            |   2 +
> >  xen/arch/x86/guest/hypervisor.c        | 112 +++++++++++++++++++++++++
> >  xen/arch/x86/guest/xen/xen.c           |  81 +-----------------
> >  xen/include/asm-x86/guest.h            |   1 +
> >  xen/include/asm-x86/guest/hypervisor.h |  58 +++++++++++++
> >  xen/include/asm-x86/guest/xen.h        |  21 ++---
> >  6 files changed, 182 insertions(+), 93 deletions(-)
> >  create mode 100644 xen/arch/x86/guest/hypervisor.c
> >  create mode 100644 xen/include/asm-x86/guest/hypervisor.h
> > 
> > diff --git a/xen/arch/x86/guest/Makefile b/xen/arch/x86/guest/Makefile
> > index 6806f04947..f63d64bbee 100644
> > --- a/xen/arch/x86/guest/Makefile
> > +++ b/xen/arch/x86/guest/Makefile
> > @@ -1 +1,3 @@
> > +obj-y += hypervisor.o
> > +
> >  subdir-$(CONFIG_XEN_GUEST) += xen
> > diff --git a/xen/arch/x86/guest/hypervisor.c 
> > b/xen/arch/x86/guest/hypervisor.c
> > new file mode 100644
> > index 0000000000..b0a724bf13
> > --- /dev/null
> > +++ b/xen/arch/x86/guest/hypervisor.c
> > @@ -0,0 +1,112 @@
> > +/******************************************************************************
> > + * 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) 2017 Citrix Systems Ltd.
> > + */
> > +
> > +#include <xen/init.h>
> > +#include <xen/mm.h>
> > +#include <xen/rangeset.h>
> > +
> > +#include <asm/guest.h>
> > +#include <asm/processor.h>
> > +
> > +static struct rangeset *mem;
> > +
> > +void __init probe_hypervisor(void)
> 
> IMO would be nice to take the opportunity to rename this to
> hypervisor_probe, to match with the rest of the hypervisor_ functions.

No problem.

> 
> > diff --git a/xen/include/asm-x86/guest/xen.h 
> > b/xen/include/asm-x86/guest/xen.h
> > index b015ed1883..d031f1f70d 100644
> > --- a/xen/include/asm-x86/guest/xen.h
> > +++ b/xen/include/asm-x86/guest/xen.h
> > @@ -32,12 +32,10 @@ extern bool xen_guest;
> >  extern bool pv_console;
> >  extern uint32_t xen_cpuid_base;
> >  
> > -void probe_hypervisor(void);
> > -void hypervisor_setup(void);
> > -void hypervisor_ap_setup(void);
> > -int hypervisor_alloc_unused_page(mfn_t *mfn);
> > -int hypervisor_free_unused_page(mfn_t mfn);
> > -void hypervisor_resume(void);
> > +void probe_xen(void);
> > +void xen_setup(void);
> > +void xen_ap_setup(void);
> > +void xen_resume(void);
> >  
> >  DECLARE_PER_CPU(unsigned int, vcpu_id);
> >  DECLARE_PER_CPU(struct vcpu_info *, vcpu_info);
> > @@ -47,16 +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 void hypervisor_setup(void)
> > -{
> > -    ASSERT_UNREACHABLE();
> > -}
> > -static inline void hypervisor_ap_setup(void)
> > -{
> > -    ASSERT_UNREACHABLE();
> > -}
> > +static inline void probe_xen(void) {}
> 
> Why do you need this?
> 
> AFAICT probe_xen is used in the same way as the rest of the xen_*
> functions, and hence I wonder why you need a stub for it?
> 
> I guess this is a forward-looking change for when probe_xen will be
> called unconditionally to check for Xen support?

Yes when probing hypervisor support you will need to unconditionally
call these functions.

Wei.

> 
> Thanks, Roger.

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