Messages in this thread | | | From | "Zhao, Yakui" <> | Subject | RE: [RFC PATCH 2/3] arch/x86/acrn: Use HYPERVISOR_CALLBACK_VECTOR for Acrn upcall vector | Date | Mon, 25 Mar 2019 03:57:48 +0000 |
| |
>-----Original Message----- >From: Thomas Gleixner [mailto:tglx@linutronix.de] >Sent: Friday, March 22, 2019 11:45 PM >To: Zhao, Yakui <yakui.zhao@intel.com> >Cc: linux-kernel@vger.kernel.org; x86@kernel.org; Chen, Jason CJ ><jason.cj.chen@intel.com> >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@intel.com> >> Signed-off-by: Zhao Yakui <yakui.zhao@intel.com> > >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
| |