Re: [RFC PATCH v3 3/4] x86: Use HYPERVISOR_CALLBACK_VECTOR for acrn_guest upcall vector

["Zhao, Yakui" <yakui.zhao@intel.com>]

On 2019年04月08日 23:00, Borislav Petkov wrote:
> You can prefix your subject now like this:
> 
> x86/acrn: Use ...

Thanks for suggestion.
It will be updated in next version.

> 
> On Mon, Apr 08, 2019 at 04:12:10PM +0800, Zhao Yakui wrote:
>> Linux kernel uses the HYPERVISOR_CALLBACK_VECTOR for hypervisor upcall
>> vector. And it is already used for Xen and HyperV.
>> After Acrn hypervisor is detected, it will also use this defined vector
>> to notify kernel.
>>
>> Co-developed-by: Jason Chen CJ <jason.cj.chen@intel.com>
>> Signed-off-by: Jason Chen CJ <jason.cj.chen@intel.com>
>> Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
>> ---
>> V1->V2: Remove the unused API definition of acrn_setup_intr_handler and
>> acrn_remove_intr_handler.
>>          Adjust the order of header file
>>          Add the declaration of acrn_hv_vector_handler and tracing
>> definition of acrn_hv_callback_vector.
>> ---
>>   arch/x86/Kconfig                 |  1 +
>>   arch/x86/entry/entry_64.S        |  5 +++++
>>   arch/x86/include/asm/acrnhyper.h | 19 +++++++++++++++++++
>>   arch/x86/kernel/cpu/acrn.c       | 22 ++++++++++++++++++++++
>>   4 files changed, 47 insertions(+)
>>   create mode 100644 arch/x86/include/asm/acrnhyper.h
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index d77d215..ae4d38b 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -848,6 +848,7 @@ config JAILHOUSE_GUEST
>>   config ACRN_GUEST
>>   	bool "ACRN Guest support"
>>   	depends on X86_64
>> +	select X86_HV_CALLBACK_VECTOR
>>   	help
>>   	  This option allows to run Linux as guest in ACRN hypervisor. Enabling
>>   	  this will allow the kernel to boot in virtualized environment under
>> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
>> index 1f0efdb..d1b8ad3 100644
>> --- a/arch/x86/entry/entry_64.S
>> +++ b/arch/x86/entry/entry_64.S
>> @@ -1129,6 +1129,11 @@ apicinterrupt3 HYPERV_STIMER0_VECTOR \
>>   	hv_stimer0_callback_vector hv_stimer0_vector_handler
>>   #endif /* CONFIG_HYPERV */
>>   
>> +#if IS_ENABLED(CONFIG_ACRN_GUEST)
>> +apicinterrupt3 HYPERVISOR_CALLBACK_VECTOR \
>> +	acrn_hv_callback_vector acrn_hv_vector_handler
>> +#endif
>> +
>>   idtentry debug			do_debug		has_error_code=0	paranoid=1 shift_ist=DEBUG_STACK
>>   idtentry int3			do_int3			has_error_code=0
>>   idtentry stack_segment		do_stack_segment	has_error_code=1
>> diff --git a/arch/x86/include/asm/acrnhyper.h b/arch/x86/include/asm/acrnhyper.h
>> new file mode 100644
>> index 0000000..9f9c239
>> --- /dev/null
>> +++ b/arch/x86/include/asm/acrnhyper.h
> 
> Simply
> 
> 	.../acrn.h
> 
> I'd say.

OK. The simplifed file name(acrn.h) will be used in next version.

> 
>> @@ -0,0 +1,19 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_X86_ACRNHYPER_H
>> +#define _ASM_X86_ACRNHYPER_H
>> +
>> +#include <linux/types.h>
>> +#include <linux/atomic.h>
>> +#include <linux/nmi.h>
>> +#include <asm/io.h>
>> +
>> +#ifdef CONFIG_ACRN_GUEST
> 
> Why is that ifdef needed?

It is used to avoid that one function declaration has no definition when 
asm/acrnhyper.h is included and ACRN_GUEST is not enabled.

> 
>> +void acrn_hv_callback_vector(void);
>> +#ifdef CONFIG_TRACING
>> +#define trace_acrn_hv_callback_vector acrn_hv_callback_vector
>> +#endif
>> +
>> +void acrn_hv_vector_handler(struct pt_regs *regs);
> 
> end marker:
> 
> #endif /* _ASM_X86_ACRNHYPER_H */

It will be added in next version.

> 
>> diff --git a/arch/x86/kernel/cpu/acrn.c b/arch/x86/kernel/cpu/acrn.c
>> index 3956567..7a233b5 100644
>> --- a/arch/x86/kernel/cpu/acrn.c
>> +++ b/arch/x86/kernel/cpu/acrn.c
>> @@ -9,7 +9,11 @@
>>    *
>>    */
>>   
>> +#include <linux/interrupt.h>
>>   #include <asm/hypervisor.h>
>> +#include <asm/acrnhyper.h>
>> +#include <asm/desc.h>
>> +#include <asm/irq_regs.h>
> 
> What's the sorting order here? Length or alphabetic? Or none? :)

Sorry that I don't consider the oder.
The required new header file is appended one by one. Of course linux/ 
path and asm / are appended separately.
Do you have any suggestion about the header order?

> 
> linux/ path includes still need to come first, of course.
>