Re: [RFC PATCH v3 2/4] x86: Add the support of ACRN guest

[Zhao, Yakui]

On 2019å04æ08æ 22:39, Borislav Petkov wrote:
> On Mon, Apr 08, 2019 at 04:12:09PM +0800, Zhao Yakui wrote:
> > ACRN is an open-source hypervisor maintained by Linuxfoundation.
>
> I think tglx wanted to say "by the Linux Foundation" here.

Sure. It will be fixed.
> > This is to add the Linux guest support on acrn-hypervisor.
>
> I think you were told already:
>
> "Please do not use 'This is to add' or 'This adds'. Just say:
>
> Add ...."
>
> So please take your time, work in *all* review feedback instead of
> hurrying the next version out without addressing all review comments.
>
> > Add x86_hyper_acrn into supported hypervisors array, which enables
> > Linux ACRN guest running on ACRN hypervisor. It is restricted to X86_64.
>
> So this all talks about *what* the patch does. But that is visible from
> the diff below and doesn't belong in the commit message.
>
> Rather, your commit message should talk about *why* a change is being
> done.

I will refine the commit log and only talk about "why" a changed is added.

> > Co-developed-by: Jason Chen CJ <jason.cj.chen@xxxxxxxxx>
> > Signed-off-by: Jason Chen CJ <jason.cj.chen@xxxxxxxxx>
> > Signed-off-by: Zhao Yakui <yakui.zhao@xxxxxxxxx>
> > ---
> > v1->v2: Change the CONFIG_ACRN to CONFIG_ACRN_GUEST, which makes it easy to
> > understand.
> >          Remove the export of x86_hyper_acrn.
> >
> > v2->v3: Remove the unnecessary dependency of PARAVIRT
> > ---
> >   arch/x86/Kconfig                  |  8 ++++++++
> >   arch/x86/include/asm/hypervisor.h |  1 +
> >   arch/x86/kernel/cpu/Makefile      |  1 +
> >   arch/x86/kernel/cpu/acrn.c        | 35 +++++++++++++++++++++++++++++++++++
> >   arch/x86/kernel/cpu/hypervisor.c  |  4 ++++
> >   5 files changed, 49 insertions(+)
> >   create mode 100644 arch/x86/kernel/cpu/acrn.c
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 54d1fbc..d77d215 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -845,6 +845,14 @@ config JAILHOUSE_GUEST
> >   	  cell. You can leave this option disabled if you only want to start
> >   	  Jailhouse and run Linux afterwards in the root cell.
> >   
> > +config ACRN_GUEST
> > +	bool "ACRN Guest support"
> > +	depends on X86_64
> > +	help
> > +	  This option allows to run Linux as guest in ACRN hypervisor. Enabling
> > +	  this will allow the kernel to boot in virtualized environment under
> > +	  the ACRN hypervisor.
>
> WARNING: please write a paragraph that describes the config symbol fully
> #47: FILE: arch/x86/Kconfig:848:
> +config ACRN_GUEST
>
> That help text above could use some of the explanation what ACRN is from
> your 0/4 message.

Make sense. I will add some description in patch 0 for the Kconfig 
description.

> > +
> >   endif #HYPERVISOR_GUEST
> >   
> >   source "arch/x86/Kconfig.cpu"
> > diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
> > index 8c5aaba..50a30f6 100644
> > --- a/arch/x86/include/asm/hypervisor.h
> > +++ b/arch/x86/include/asm/hypervisor.h
> > @@ -29,6 +29,7 @@ enum x86_hypervisor_type {
> >   	X86_HYPER_XEN_HVM,
> >   	X86_HYPER_KVM,
> >   	X86_HYPER_JAILHOUSE,
> > +	X86_HYPER_ACRN,
> >   };
> >   
> >   #ifdef CONFIG_HYPERVISOR_GUEST
> > diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
> > index cfd24f9..17a7cdf 100644
> > --- a/arch/x86/kernel/cpu/Makefile
> > +++ b/arch/x86/kernel/cpu/Makefile
> > @@ -44,6 +44,7 @@ obj-$(CONFIG_X86_CPU_RESCTRL)		+= resctrl/
> >   obj-$(CONFIG_X86_LOCAL_APIC)		+= perfctr-watchdog.o
> >   
> >   obj-$(CONFIG_HYPERVISOR_GUEST)		+= vmware.o hypervisor.o mshyperv.o
> > +obj-$(CONFIG_ACRN_GUEST)		+= acrn.o
> >   
> >   ifdef CONFIG_X86_FEATURE_NAMES
> >   quiet_cmd_mkcapflags = MKCAP   $@
> > diff --git a/arch/x86/kernel/cpu/acrn.c b/arch/x86/kernel/cpu/acrn.c
> > new file mode 100644
> > index 0000000..3956567
> > --- /dev/null
> > +++ b/arch/x86/kernel/cpu/acrn.c
> > @@ -0,0 +1,35 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ACRN detection support
> > + *
> > + * Copyright (C) 2019 Intel Corporation. All rights reserved.
> > + *
> > + * Jason Chen CJ <jason.cj.chen@xxxxxxxxx>
> > + * Zhao Yakui <yakui.zhao@xxxxxxxxx>
> > + *
> > + */
> > +
> > +#include <asm/hypervisor.h>
> > +
> > +static uint32_t __init acrn_detect(void)
> > +{
> > +	return hypervisor_cpuid_base("ACRNACRNACRN\0\0", 0);
> > +}
> > +
> > +static void __init acrn_init_platform(void)
> > +{
> > +}
> > +
> > +static bool acrn_x2apic_available(void)
> > +{
> > +	/* do not support x2apic */
>
> Why?
>
> This comment could explain why that choice has been made.

Currently the x2apic is not enabled in the first step.
Next step it needs to check the cpu info reported by ACRN hypervisor to 
determine whether the x2apic should be supported.
How about using the below comments?
" x2apic is not supported now. Later it will check the cpu info to 
determine whether the x2apic can be supported or not."

Thanks