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

[Thomas Gleixner]

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?

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

Same SOB issue.

> +#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() ?

>  #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.

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

Stray newline

> +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

> + */
> +__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.

Thanks,

	tglx