| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1f575bbe5adf469b9a2482686d06fc98@BY2PR03MB299.namprd03.prod.outlook.com>
Date: Fri, 28 Feb 2014 02:50:52 +0000
From: KY Srinivasan <kys@...rosoft.com>
To: Thomas Gleixner <tglx@...utronix.de>
CC: LKML <linux-kernel@...r.kernel.org>, Ingo Molnar <mingo@...e.hu>,
"Peter Zijlstra" <peterz@...radead.org>, x86 <x86@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linuxdrivers <devel@...uxdriverproject.org>
Subject: RE: [patch 25/26] x86: hyperv: Cleanup the irq mess
> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@...utronix.de]
> Sent: Tuesday, February 25, 2014 11:10 AM
> To: KY Srinivasan
> Cc: LKML; Ingo Molnar; Peter Zijlstra; x86; Greg Kroah-Hartman; linuxdrivers
> Subject: RE: [patch 25/26] x86: hyperv: Cleanup the irq mess
>
> On Tue, 25 Feb 2014, KY Srinivasan wrote:
> > > -----Original Message-----
> > > From: Thomas Gleixner [mailto:tglx@...utronix.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.
>
> So, if CONFIG_HYPERV=n then you do not need the request_irq() fallback at
> all, right? Somehow I missed the HYPERV dependency of the VMBUS stuff
>
> That makes stuff even simpler as we can get rid of those extra cases including
> the extra flow handler.
>
> New patch below.
Thanks Thomas.
Acked-by: K. Y. Srinivasan <kys@...rosoft.com>
>
> Thanks,
>
> tglx
> ---
>
> arch/x86/include/asm/mshyperv.h | 4 +-
> arch/x86/kernel/cpu/mshyperv.c | 78 ++++++++++++++++++++-------------
> -------
> drivers/hv/vmbus_drv.c | 39 ++------------------
> 3 files changed, 47 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,45 @@
> 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;
> +}
> +EXPORT_SYMBOL_GPL(hv_setup_vmbus_irq);
> +EXPORT_SYMBOL_GPL(hv_remove_vmbus_irq);
> +#endif
> +
> static uint32_t __init ms_hyperv_platform(void) {
> u32 eax;
> @@ -113,41 +153,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 majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists