RE: [RFC PATCH 1/3] arch/x86: add ACRN hypervisor guest

[Zhao, Yakui]

Hi,  Tglx

       Thanks for your nice review. 
>-----Original Message-----
>From: Thomas Gleixner [mailto:tglx@xxxxxxxxxxxxx]
>Sent: Friday, March 22, 2019 11:20 PM
>To: Zhao, Yakui <yakui.zhao@xxxxxxxxx>
>Cc: linux-kernel@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx; Chen, Jason CJ
><jason.cj.chen@xxxxxxxxx>
>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.chen@xxxxxxxxx>
>> Signed-off-by: Zhao Yakui <yakui.zhao@xxxxxxxxx>
>
>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