Hi, Tglx Thanks for your nice review. >-----Original Message----- >From: Thomas Gleixner [mailto:t...@linutronix.de] >Sent: Friday, March 22, 2019 11:20 PM >To: Zhao, Yakui <yakui.z...@intel.com> >Cc: linux-kernel@vger.kernel.org; x...@kernel.org; Chen, Jason CJ ><jason.cj.c...@intel.com> >Subject: Re: [RFC PATCH 1/3] arch/x86: add ACRN hypervisor guest > >Zhao, > >On Thu, 7 Mar 2019, Zhao Yakui wrote: > >> ACRN is one open-source hypervisour, which is maintained by Linux >> foundation. This is to add the para-virtualization support so that it >> allows to enable the Linux guest on acrn-hypervisor. >> >> This adds x86_hyper_acrn into supported hypervisors array, which >> enables ACRN services guest running on ACRN hypervisor. It is > >What is a ACRN services guest?
It should be "ACRN guest". > >> restricted to X86_64. >> >> Signed-off-by: Jason Chen CJ <jason.cj.c...@intel.com> >> Signed-off-by: Zhao Yakui <yakui.z...@intel.com> > >This SOB chain is wrong. If Jason wrote the patch, then there should be a >'From: Jason ..' line at the top of the change log. If you both wrote it then >Jason's SOB wants to be preceeded by a 'Co-developed-by: Jason....' >line. See Documentation/process/ Sure. It will be updated to "Co-developed-by: Jason" in next version. > >> +config ACRN >> + bool "Enable services on ACRN hypervisor" >> + depends on X86_64 && PARAVIRT >> + help >> + This option allows to run Linux as guest in ACRN hypervisor. >> + It is needed if you want to run ACRN services linux on top of >> + ACRN hypervisor. > >This help text does not make any sense to me. "Enable Services on ACRN hypervisor" is a little confusing. In next version it will be changed to "ACRN_GUEST". How about the below: config ACRN_GUEST bool "ACRN Guest Support" depends on X86_64 && PARAVIRT help This option allows to run Linux as guest in ACRN hypervisor. Enabling this will allow the kernel to boot in a paravirtualized environment under the ACRN hypervisor > >> +const struct hypervisor_x86 x86_hyper_acrn = { >> + .name = "ACRN", >> + .detect = acrn_detect, >> + .type = X86_HYPER_ACRN, >> + .init.init_platform = acrn_init_platform, >> + .init.x2apic_available = acrn_x2apic_available, }; >> +EXPORT_SYMBOL(x86_hyper_acrn); > >Whay is this exported? The only user of this is hypervisor.c and that is built >in. >Please remove. You are right. It will be removed and the x86_hyper_acrn will also be added into "__initconst" section. > >Thanks, > > tglx