> -----Original Message-----
> From: Thomas Gleixner [mailto:t...@linutronix.de]
> Sent: Sunday, February 23, 2014 1:40 PM
> To: LKML
> Cc: Ingo Molnar; Peter Zijlstra; x86; KY Srinivasan; Greg Kroah-Hartman;
> linuxdrivers
> Subject: [patch 25/26] x86: hyperv: Cleanup the irq mess
>
> The vmbus/hyperv interrupt handling is another complete trainwreck and
> probably the worst of all currently in tree.
>
> If CONFIG_HYPERV=y then the interrupt delivery to the vmbus happens via
> the direct HYPERVISOR_CALLBACK_VECTOR. So far so good, but:
>
> The driver requests first a normal device interrupt. The only reason
> to do so is to increment the interrupt stats of that device
> interrupt.
>
> We have proper accounting mechanisms for direct vectors, but of
> course it's too much effort to add that 5 lines of code.
>
> Aside of that the alloc_intr_gate() is not protected against
> reallocation which makes module reload impossible.
>
> If CONFIG_HYPERV=n then the vmbus request a regular device interrupt via
> request_irq() and installs it's own private flow handler. Of course this lacks
> any explanation why it can't use the standard flow handler or the existing
> handle_percpu_irq handler.
>
> Solution to the problem is simple to rip out the whole mess and implement it
> correctly.
Thomas,
Thank you for cleaning up this code. When CONFIG_HYPERV== n, none of the VMBUS
code is active.
The special case can go away as you have noted.
Regards,
K. Y
>
> First of all move all that code to arch/x86/kernel/cpu/mshyperv.c
>
> For the CONFIG_HYPERV=y case merily install the
> HYPERVISOR_CALLBACK_VECTOR with proper reallocation protection and
> use the proper direct vector accounting mechanism.
>
> For the CONFIG_HYPERV=n case request the device irq and install the weird
> flow handler.
>
> If the special flow handler can go away, which I assume to be true, then this
> simplifies the code even further.
>
> Signed-off-by: Thomas Gleixner <t...@linutronix.de>
> Cc: x86 <x...@kernel.org>
> Cc: "K. Y. Srinivasan" <k...@microsoft.com>
> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> Cc: linuxdrivers <de...@linuxdriverproject.org>
>
> ---
> arch/x86/include/asm/mshyperv.h | 4 +
> arch/x86/kernel/cpu/mshyperv.c | 97 ++++++++++++++++++++++++------
> ----------
> drivers/hv/vmbus_drv.c | 39 +---------------
> 3 files changed, 66 insertions(+), 74 deletions(-)
>
> Index: tip/arch/x86/include/asm/mshyperv.h
> ==========================================================
> =========
> --- tip.orig/arch/x86/include/asm/mshyperv.h
> +++ tip/arch/x86/include/asm/mshyperv.h
> @@ -2,6 +2,7 @@
> #define _ASM_X86_MSHYPER_H
>
> #include <linux/types.h>
> +#include <linux/interrupt.h>
> #include <asm/hyperv.h>
>
> struct ms_hyperv_info {
> @@ -16,6 +17,7 @@ void hyperv_callback_vector(void); #define
> trace_hyperv_callback_vector hyperv_callback_vector #endif void
> hyperv_vector_handler(struct pt_regs *regs); -void
> hv_register_vmbus_handler(int irq, irq_handler_t handler);
> +int hv_setup_vmbus_irq(int irq, irq_handler_t handler, void *dev_id);
> +void hv_remove_vmbus_irq(int irq, void *dev_id);
>
> #endif
> Index: tip/arch/x86/kernel/cpu/mshyperv.c
> ==========================================================
> =========
> --- tip.orig/arch/x86/kernel/cpu/mshyperv.c
> +++ tip/arch/x86/kernel/cpu/mshyperv.c
> @@ -17,6 +17,7 @@
> #include <linux/hardirq.h>
> #include <linux/efi.h>
> #include <linux/interrupt.h>
> +#include <linux/irq.h>
> #include <asm/processor.h>
> #include <asm/hypervisor.h>
> #include <asm/hyperv.h>
> @@ -30,6 +31,64 @@
> struct ms_hyperv_info ms_hyperv;
> EXPORT_SYMBOL_GPL(ms_hyperv);
>
> +#ifdef CONFIG_HYPERV
> +static irq_handler_t *vmbus_handler;
> +
> +void hyperv_vector_handler(struct pt_regs *regs) {
> + struct pt_regs *old_regs = set_irq_regs(regs);
> +
> + irq_enter();
> + exit_idle();
> +
> + inc_irq_stat(irq_hv_callback_count);
> + if (vmbus_handler)
> + vmbus_handler();
> +
> + irq_exit();
> + set_irq_regs(old_regs);
> +}
> +
> +int hv_setup_vmbus_irq(int irq, irq_handler_t *handler, void *dev_id) {
> + vmbus_handler = handler;
> + /*
> + * Setup the IDT for hypervisor callback. Prevent reallocation
> + * at module reload.
> + */
> + if (!test_bit(HYPERVISOR_CALLBACK_VECTOR, used_vectors))
> + alloc_intr_gate(HYPERVISOR_CALLBACK_VECTOR,
> + hyperv_callback_vector);
> +}
> +
> +void hv_remove_vmbus_irq(unsigned int irq, void *dev_id) {
> + /* We have no way to deallocate the interrupt gate */
> + vmbus_handler = NULL;
> +}
> +#else
> +int hv_setup_vmbus_irq(int irq, irq_handler_t handler, void *dev_id) {
> + int ret = request_irq(irq, handler, 0, "hyperv", dev_id);
> +
> + if (!ret) {
> + /*
> + * Vmbus interrupts can be handled concurrently on
> + * different CPUs. Install the simple percpu flow handler.
> + */
> + irq_set_handler(irq, handle_percpu_simple_irq);
> + }
> + return ret;
> +}
> +
> +void hv_remove_vmbus_irq(int irq, void *dev_id) {
> + free_irq(irq, dev_id);;
> +}
> +#endif
> +EXPORT_SYMBOL_GPL(hv_setup_vmbus_irq);
> +EXPORT_SYMBOL_GPL(hv_remove_vmbus_irq);
> +
> static uint32_t __init ms_hyperv_platform(void) {
> u32 eax;
> @@ -113,41 +172,3 @@ const __refconst struct hypervisor_x86 x
> .init_platform = ms_hyperv_init_platform,
> };
> EXPORT_SYMBOL(x86_hyper_ms_hyperv);
> -
> -#if IS_ENABLED(CONFIG_HYPERV)
> -static int vmbus_irq = -1;
> -static irq_handler_t vmbus_isr;
> -
> -void hv_register_vmbus_handler(int irq, irq_handler_t handler) -{
> - /*
> - * Setup the IDT for hypervisor callback.
> - */
> - alloc_intr_gate(HYPERVISOR_CALLBACK_VECTOR,
> hyperv_callback_vector);
> -
> - vmbus_irq = irq;
> - vmbus_isr = handler;
> -}
> -
> -void hyperv_vector_handler(struct pt_regs *regs) -{
> - struct pt_regs *old_regs = set_irq_regs(regs);
> - struct irq_desc *desc;
> -
> - irq_enter();
> - exit_idle();
> -
> - desc = irq_to_desc(vmbus_irq);
> -
> - if (desc)
> - generic_handle_irq_desc(vmbus_irq, desc);
> -
> - irq_exit();
> - set_irq_regs(old_regs);
> -}
> -#else
> -void hv_register_vmbus_handler(int irq, irq_handler_t handler) -{ -} -#endif
> -EXPORT_SYMBOL_GPL(hv_register_vmbus_handler);
> Index: tip/drivers/hv/vmbus_drv.c
> ==========================================================
> =========
> --- tip.orig/drivers/hv/vmbus_drv.c
> +++ tip/drivers/hv/vmbus_drv.c
> @@ -25,7 +25,6 @@
> #include <linux/init.h>
> #include <linux/module.h>
> #include <linux/device.h>
> -#include <linux/irq.h>
> #include <linux/interrupt.h>
> #include <linux/sysctl.h>
> #include <linux/slab.h>
> @@ -558,9 +557,6 @@ static struct bus_type hv_bus = {
> .dev_groups = vmbus_groups,
> };
>
> -static const char *driver_name = "hyperv";
> -
> -
> struct onmessage_work_context {
> struct work_struct work;
> struct hv_message msg;
> @@ -677,19 +673,6 @@ static irqreturn_t vmbus_isr(int irq, vo }
>
> /*
> - * vmbus interrupt flow handler:
> - * vmbus interrupts can concurrently occur on multiple CPUs and
> - * can be handled concurrently.
> - */
> -
> -static void vmbus_flow_handler(unsigned int irq, struct irq_desc *desc) -{
> - kstat_incr_irqs_this_cpu(irq, desc);
> -
> - desc->action->handler(irq, desc->action->dev_id);
> -}
> -
> -/*
> * vmbus_bus_init -Main vmbus driver initialization routine.
> *
> * Here, we
> @@ -715,26 +698,13 @@ static int vmbus_bus_init(int irq)
> if (ret)
> goto err_cleanup;
>
> - ret = request_irq(irq, vmbus_isr, 0, driver_name, hv_acpi_dev);
> + ret = hv_setup_vmbus_irq(irq, vmbus_isr, hv_acpi_dev);
>
> if (ret != 0) {
> - pr_err("Unable to request IRQ %d\n",
> - irq);
> + pr_err("Unable to request IRQ %d\n", irq);
> goto err_unregister;
> }
>
> - /*
> - * Vmbus interrupts can be handled concurrently on
> - * different CPUs. Establish an appropriate interrupt flow
> - * handler that can support this model.
> - */
> - irq_set_handler(irq, vmbus_flow_handler);
> -
> - /*
> - * Register our interrupt handler.
> - */
> - hv_register_vmbus_handler(irq, vmbus_isr);
> -
> ret = hv_synic_alloc();
> if (ret)
> goto err_alloc;
> @@ -753,7 +723,7 @@ static int vmbus_bus_init(int irq)
>
> err_alloc:
> hv_synic_free();
> - free_irq(irq, hv_acpi_dev);
> + hv_remove_vmbus_irq(irq, hv_acpi_dev);
>
> err_unregister:
> bus_unregister(&hv_bus);
> @@ -978,8 +948,7 @@ cleanup:
>
> static void __exit vmbus_exit(void)
> {
> -
> - free_irq(irq, hv_acpi_dev);
> + hv_remove_vmbus_irq(irq, hv_acpi_dev);
> vmbus_free_channels();
> bus_unregister(&hv_bus);
> hv_cleanup();
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
