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.
> 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.
> Co-developed-by: Jason Chen CJ <jason.cj.c...@intel.com>
> Signed-off-by: Jason Chen CJ <jason.cj.c...@intel.com>
> Signed-off-by: Zhao Yakui <yakui.z...@intel.com>
> ---
> 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.
> +
> 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.c...@intel.com>
> + * Zhao Yakui <yakui.z...@intel.com>
> + *
> + */
> +
> +#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.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
