RE: [RFC PATCH 2/3] arch/x86/acrn: Use HYPERVISOR_CALLBACK_VECTOR for Acrn upcall vector

[Zhao, Yakui]

>-----Original Message-----
>From: Thomas Gleixner [mailto:tglx@xxxxxxxxxxxxx]
>Sent: Friday, March 22, 2019 11:45 PM
>To: Zhao, Yakui <yakui.zhao@xxxxxxxxx>
>Cc: linux-kernel@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx; Chen, Jason CJ
><jason.cj.chen@xxxxxxxxx>
>Subject: Re: [RFC PATCH 2/3] arch/x86/acrn: Use
>HYPERVISOR_CALLBACK_VECTOR for Acrn upcall vector
>
>On Thu, 7 Mar 2019, Zhao Yakui wrote:
>
>> WHen it works in hypervisor guest mode, 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 as notify vector to kernel.
>> And two APIs are added so that the other module can add/remove the
>> hypervisor callback irq handler.
>
>Which other module?
>

This work is divided into two steps. The first is to add the support of acrn-guest under arch/x86.
The second is to add one driver that allows the acrn-guest to communicate with the acrn-hypervisor.
As the second part has the dependency on the first part,  we hope that the acrn-guest is added firstly and the
required API is also added.

>> Signed-off-by: Jason Chen CJ <jason.cj.chen@xxxxxxxxx>
>> Signed-off-by: Zhao Yakui <yakui.zhao@xxxxxxxxx>
>
>Same SOB issue.

Sure. It will be updated.

>
>> +#if IS_ENABLED(CONFIG_ACRN)
>> +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..2562a82
>> --- /dev/null
>> +++ b/arch/x86/include/asm/acrnhyper.h
>> @@ -0,0 +1,16 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */ #ifndef _ASM_X86_ACRNHYPER_H
>> +#define _ASM_X86_ACRNHYPER_H
>> +
>> +#include <linux/types.h>
>> +#include <asm/io.h>
>> +
>> +#ifdef CONFIG_ACRN
>> +/* ACRN Hypervisor callback */
>> +void acrn_hv_callback_vector(void);
>
>What declares acrn_hv_vector_handler() ?

Acrn_hv_callback_vector is defined in arch/x86/entry/entry_64.S, which will be used as
the parameter of alloc_intr_gate

Acrn_hv_vector_handler is the real ISR handler, which is defined in acrn.c.

>
>>  #include <asm/hypervisor.h>
>> +#include <asm/acrnhyper.h>
>> +#include <asm/irq_vectors.h>
>> +#include <asm/irq_regs.h>
>> +#include <asm/desc.h>
>> +#include <linux/interrupt.h>
>
>First of all the include order is always:
>
> #include <linux/...>
> #include <linux/...>
>
> #include <asm/...>
>
>Aside of that including 'linux/irq.h' should include everything you need. No
>need for gazillion of includes.

OK. The include order will be updated in next version.

>
>>  static bool acrn_x2apic_available(void) @@ -26,6 +33,37 @@ static
>> bool acrn_x2apic_available(void)
>>  	return false;
>>  }
>>
>> +
>
>Stray newline
>

Sure. The extra newline will be removed.

>> +static void (*acrn_intr_handler)(void);
>> +/*
>
>Which should have been above this comment. Glueing stuff together makes it
>unreadable.
>
>> + * Handler for ACRN_HV_CALLBACK.
>
>Err? This handles the HYPERVISOR_CALLBACK_VECTOR
>

I will remove the confusing comment.
In fact this acrn_intr_handle is similar to the vmbus_handler in arch/x86/kernel/cpu/mshyperv.c

>> + */
>> +__visible void __irq_entry acrn_hv_vector_handler(struct pt_regs
>> +*regs) {
>> +	struct pt_regs *old_regs = set_irq_regs(regs);
>> +
>> +	entering_ack_irq();
>> +	inc_irq_stat(irq_hv_callback_count);
>> +
>> +	if (acrn_intr_handler)
>> +		acrn_intr_handler();
>> +
>> +	exiting_irq();
>> +	set_irq_regs(old_regs);
>> +}
>> +
>> +void acrn_setup_intr_irq(void (*handler)(void)) {
>> +	acrn_intr_handler = handler;
>> +}
>> +EXPORT_SYMBOL(acrn_setup_intr_irq);
>> +
>> +void acrn_remove_intr_irq(void)
>> +{
>> +	acrn_intr_handler = NULL;
>> +}
>> +EXPORT_SYMBOL(acrn_remove_intr_irq);
>
>Where is the code which uses these exports? We are not adding exports just
>because or for consumption by out of tree modules.

Understand it.
Is it reasonable that the above two functions are added in the driver patch set?


>
>Thanks,
>
>	tglx