Message-ID: <559D3597.2080706@linaro.org>
Date: Wed, 08 Jul 2015 16:37:11 +0200
From: Eric Auger <eric.auger@linaro.org>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0
MIME-Version: 1.0
To: Alex Williamson <alex.williamson@redhat.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org
CC: eric.auger@st.com, joro@8bytes.org, avi.kivity@gmail.com,
        pbonzini@redhat.com, feng.wu@intel.com
Subject: Re: [RFC PATCH] irq: IRQ bypass manager
References: <20150707212725.9250.21077.stgit@gimli.home>
In-Reply-To: <20150707212725.9250.21077.stgit@gimli.home>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 14670
Lines: 439

Hi Alex,
On 07/07/2015 11:40 PM, Alex Williamson wrote:
> When a physical I/O device is assigned to a virtual machine through
> facilities like VFIO and KVM, the interrupt for the device generally
> bounces through the host system before being injected into the VM.
> However, hardware technologies exist that often allow the host to be
> bypassed for some of these scenarios.  Intel Posted Interrupts allow
> the specified physical edge interrupts to be directly injected into a
> guest when delivered to a physical processor while the vCPU is
> running.  ARM IRQ Forwarding allows the hypervisor to handle level
> triggered device interrupts as edge interrupts, by giving the guest
> control of de-asserting and unmasking the interrupt line.
> 
> The IRQ bypass manager here is meant to provide the shim to connect
> interrupt producers, generally the host physical device driver, with
> interrupt consumers, generally the hypervisor, in order to configure
> these bypass mechanism.  To do this, we base the connection on a
> shared, opaque token.  For KVM-VFIO this is expected to be an
> eventfd_ctx since this is the connection we already use to connect an
> eventfd to an irqfd on the in-kernel path.  When a producer and
> consumer with matching tokens is found, callbacks via both registered
> participants allow the bypass facilities to be automatically enabled.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> Cc: Eric Auger <eric.auger@linaro.org>
> ---
> 
> This is the current draft of the IRQ bypass manager, I've made the
> following changes:
> 
>  - Incorporated Eric's extensions (I would welcome Sign-offs from all
>    involved in the development, especially Eric - I've gone ahead and
>    added Linaro copyright for the contributions so far)
Signed-off-by: Eric Auger <eric.auger@linaro.org>
>  - Module support with module reference tracking
>  - might_sleep() as suggested by Paolo
>  - kerneldoc as suggested by Paolo
>  - Renamed file s/bypass/irqbypass/ because a module named "bypass"
>    is strange
> 
> Issues:
Currently, for IRQ forwarding, I use an active boolean on producer side
- on connect, this "active" boolean is set by cons->add_consumer and
used by prod->add_producer
- conversely on destruction it is set by cons->del_producer and used by
prod->del_consumer

For me this reflects the state of the IRQ. On connect, if the IRQ is
active of VFIO masked it is unsafe to connect - I still don't know how
to handle that case - . On disconnect, if the IRQ is active at interrupt
controller level I need to mask at VFIO level.

I introduced this boolean in the IRQ forwarding series(irq: bypass:
Extend skeleton for ARM forwarding control).

I know this is quite specific to IRQ forwarding stuff though.

Any suggestion of a more elegant implementation?

>  - The update() callback is defined but not used
I let Feng comment
>  - We can't have *all* the callbacks be optional.  I assume add/del
>    are required
yes to me add/del are mandated. Others are optional.
>  - Naming consistency, stop is to start as suspend is to resume, not
>    stop/resume
let's use start/stop then
>  - Callback descriptions including why we need separate stop/start
>    hooks when it seems like the callee could reasonably assume such
>    around the add/del callbacks
why we need to separate start/top:
on connect: I need to intertwine actions from producer/consumers for
forwarding
1) producer: disable IRQ
2) consumer: halt guest
3) producer: compute active state, set non automasked handler
4) consumer: program gic/irqchip, normally depending on active state
5) consumer: start guest
6) producer: enable IRQ

each action is private to a consumer/producer
action 3) only can be done after 1) and 2) to snapshot the active state

On disconnect, I need
1) producer: disable IRQ
2) consumer: halt guest
3) consumer: test active state, program gic/irqchip
4) producer: set automasked handled and mask the IRQ depending on active
state
5) consumer: start guest
6) producer: enable IRQ

again action 3) can only be done after 1) and 2) to snapshot the active
state
So rationale behind having start/stop is that I need a more complex
sequence than just 2 private functions. Then I thought that when setting
up/tearing down the link it could be natural to have start/stop
functions at source & sink.

start: start the IRQ consumption/production at sink/source
stop: stop the IRQ consumption/production at sink/source

>  - Need functional prototypes for both PI and forwarding
this is functional for forwarding. tested on calxeda midway but
dependencies mostly are RFC's.

Best Regards

Eric
> 
>  include/linux/irqbypass.h |   75 ++++++++++++++++
>  kernel/irq/Kconfig        |    3 +
>  kernel/irq/Makefile       |    1 
>  kernel/irq/irqbypass.c    |  206 +++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 285 insertions(+)
>  create mode 100644 include/linux/irqbypass.h
>  create mode 100644 kernel/irq/irqbypass.c
> 
> diff --git a/include/linux/irqbypass.h b/include/linux/irqbypass.h
> new file mode 100644
> index 0000000..cc7ce45
> --- /dev/null
> +++ b/include/linux/irqbypass.h
> @@ -0,0 +1,75 @@
> +/*
> + * IRQ offload/bypass manager
> + *
> + * Copyright (C) 2015 Red Hat, Inc.
> + * Copyright (c) 2015 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef IRQBYPASS_H
> +#define IRQBYPASS_H
> +
> +#include <linux/list.h>
> +
> +struct irq_bypass_consumer;
> +
> +/**
> + * struct irq_bypass_producer - IRQ bypass producer definition
> + * @node: IRQ bypass manager private list management
> + * @token: opaque token to match between producer and consumer
> + * @irq: Linux IRQ number for the producer device
> + * @stop:
> + * @resume:
> + * @add_consumer:
> + * @del_consumer:
> + *
> + * The IRQ bypass producer structure represents an interrupt source for
> + * participation in possible host bypass, for instance an interrupt vector
> + * for a physical device assigned to a VM.
> + */
> +struct irq_bypass_producer {
> +	struct list_head node;
> +	void *token;
> +	int irq; /* linux irq */
> +	void (*stop)(struct irq_bypass_producer *);
> +	void (*resume)(struct irq_bypass_producer *);
> +	void (*add_consumer)(struct irq_bypass_producer *,
> +			     struct irq_bypass_consumer *);
> +	void (*del_consumer)(struct irq_bypass_producer *,
> +			     struct irq_bypass_consumer *);
> +};
> +
> +/**
> + * struct irq_bypass_consumer - IRQ bypass consumer definition
> + * @node: IRQ bypass manager private list management
> + * @token: opaque token to match between producer and consumer
> + * @stop:
> + * @resume:
> + * @add_consumer:
> + * @del_consumer:
> + * @update:
> + *
> + * The IRQ bypass consumer structure represents an interrupt sink for
> + * participation in possible host bypass, for instance a hypervisor may
> + * support offloads to allow bypassing the host entirely or offload
> + * portions of the interrupt handling to the VM.
> + */
> +struct irq_bypass_consumer {
> +	struct list_head node;
> +	void *token;
> +	void (*stop)(struct irq_bypass_consumer *);
> +	void (*resume)(struct irq_bypass_consumer *);
> +	void (*add_producer)(struct irq_bypass_consumer *,
> +			     struct irq_bypass_producer *);
> +	void (*del_producer)(struct irq_bypass_consumer *,
> +			     struct irq_bypass_producer *);
> +	void (*update)(struct irq_bypass_consumer *);
> +};
> +
> +int irq_bypass_register_producer(struct irq_bypass_producer *);
> +void irq_bypass_unregister_producer(struct irq_bypass_producer *);
> +int irq_bypass_register_consumer(struct irq_bypass_consumer *);
> +void irq_bypass_unregister_consumer(struct irq_bypass_consumer *);
> +#endif /* IRQBYPASS_H */
> diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig
> index 9a76e3b..de5afe3 100644
> --- a/kernel/irq/Kconfig
> +++ b/kernel/irq/Kconfig
> @@ -100,4 +100,7 @@ config SPARSE_IRQ
>  
>  	  If you don't know what to do here, say N.
>  
> +config IRQ_BYPASS_MANAGER
> +	tristate
> +
>  endmenu
> diff --git a/kernel/irq/Makefile b/kernel/irq/Makefile
> index d121235..a6cfec7 100644
> --- a/kernel/irq/Makefile
> +++ b/kernel/irq/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_PROC_FS) += proc.o
>  obj-$(CONFIG_GENERIC_PENDING_IRQ) += migration.o
>  obj-$(CONFIG_PM_SLEEP) += pm.o
>  obj-$(CONFIG_GENERIC_MSI_IRQ) += msi.o
> +obj-$(CONFIG_IRQ_BYPASS_MANAGER) += irqbypass.o
> diff --git a/kernel/irq/irqbypass.c b/kernel/irq/irqbypass.c
> new file mode 100644
> index 0000000..3fd828d
> --- /dev/null
> +++ b/kernel/irq/irqbypass.c
> @@ -0,0 +1,206 @@
> +/*
> + * IRQ offload/bypass manager
> + *
> + * Copyright (C) 2015 Red Hat, Inc.
> + * Copyright (c) 2015 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Various virtualization hardware acceleration techniques allow bypassing
> + * or offloading interrupts received from devices around the host kernel.
> + * Posted Interrupts on Intel VT-d systems can allow interrupts to be
> + * received directly by a virtual machine.  ARM IRQ Forwarding can allow
> + * level triggered device interrupts to be de-asserted directly by the VM.
> + * This manager allows interrupt producers and consumers to find each other
> + * to enable this sort of bypass.
> + */
> +
> +#include <linux/irqbypass.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("IRQ bypass manager utility module");
> +
> +static LIST_HEAD(producers);
> +static LIST_HEAD(consumers);
> +static DEFINE_MUTEX(lock);
> +
> +/* @lock must be held when calling connect */
> +static void __connect(struct irq_bypass_producer *prod,
> +		      struct irq_bypass_consumer *cons)
> +{
> +	if (prod->stop)
> +		prod->stop(prod);
> +	if (cons->stop)
> +		cons->stop(cons);
> +	if (prod->add_consumer)
> +		prod->add_consumer(prod, cons);
> +	if (cons->add_producer)
> +		cons->add_producer(cons, prod);
> +	if (cons->resume)
> +		cons->resume(cons);
> +	if (prod->resume)
> +		prod->resume(prod);
> +}
> +
> +/* @lock must be held when calling disconnect */
> +static void __disconnect(struct irq_bypass_producer *prod,
> +			 struct irq_bypass_consumer *cons)
> +{
> +	if (prod->stop)
> +		prod->stop(prod);
> +	if (cons->stop)
> +		cons->stop(cons);
> +	if (cons->del_producer)
> +		cons->del_producer(cons, prod);
> +	if (prod->del_consumer)
> +		prod->del_consumer(prod, cons);
> +	if (cons->resume)
> +		cons->resume(cons);
> +	if (prod->resume)
> +		prod->resume(prod);
> +}
> +
> +/**
> + * irq_bypass_register_producer - register IRQ bypass producer
> + * @producer: pointer to producer structure
> + *
> + * Add the provided IRQ producer to the list of producers and connect
> + * with any matching tokens found on the IRQ consumers list.
> + */
> +int irq_bypass_register_producer(struct irq_bypass_producer *producer)
> +{
> +	struct irq_bypass_producer *tmp;
> +	struct irq_bypass_consumer *consumer;
> +
> +	might_sleep();
> +
> +	if (!try_module_get(THIS_MODULE))
> +		return -ENODEV;
> +
> +	mutex_lock(&lock);
> +
> +	list_for_each_entry(tmp, &producers, node) {
> +		if (tmp->token == producer->token) {
> +			mutex_unlock(&lock);
> +			module_put(THIS_MODULE);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	list_add(&producer->node, &producers);
> +
> +	list_for_each_entry(consumer, &consumers, node) {
> +		if (consumer->token == producer->token) {
> +			__connect(producer, consumer);
> +			break;
> +		}
> +	}
> +
> +	mutex_unlock(&lock);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(irq_bypass_register_producer);
> +
> +/**
> + * irq_bypass_unregister_producer - unregister IRQ bypass producer
> + * @producer: pointer to producer structure
> + *
> + * Remove a previously registered IRQ producer from the list of producers
> + * and disconnected from any connected IRQ consumers.
> + */
> +void irq_bypass_unregister_producer(struct irq_bypass_producer *producer)
> +{
> +	struct irq_bypass_consumer *consumer;
> +
> +	might_sleep();
> +
> +	mutex_lock(&lock);
> +
> +	list_for_each_entry(consumer, &consumers, node) {
> +		if (consumer->token == producer->token) {
> +			__disconnect(producer, consumer);
> +			break;
> +		}
> +	}
> +
> +	list_del(&producer->node);
> +
> +	mutex_unlock(&lock);
> +	module_put(THIS_MODULE);
> +}
> +EXPORT_SYMBOL_GPL(irq_bypass_unregister_producer);
> +
> +/**
> + * irq_bypass_register_consumer - register IRQ bypass consumer
> + * @consumer: pointer to consumer structure
> + *
> + * Add the provided IRQ consumer to the list of consumers and connect
> + * with any matching tokens found on the IRQ producer list.
> + */
> +int irq_bypass_register_consumer(struct irq_bypass_consumer *consumer)
> +{
> +	struct irq_bypass_consumer *tmp;
> +	struct irq_bypass_producer *producer;
> +
> +	might_sleep();
> +
> +	if (!try_module_get(THIS_MODULE))
> +		return -ENODEV;
> +
> +	mutex_lock(&lock);
> +
> +	list_for_each_entry(tmp, &consumers, node) {
> +		if (tmp->token == consumer->token) {
> +			mutex_unlock(&lock);
> +			module_put(THIS_MODULE);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	list_add(&consumer->node, &consumers);
> +
> +	list_for_each_entry(producer, &producers, node) {
> +		if (producer->token == consumer->token) {
> +			__connect(producer, consumer);
> +			break;
> +		}
> +	}
> +
> +	mutex_unlock(&lock);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(irq_bypass_register_consumer);
> +
> +/**
> + * irq_bypass_unregister_consumer - unregister IRQ bypass consumer
> + * @consumer: pointer to consumer structure
> + *
> + * Remove a previously registered IRQ consumer from the list of consumers
> + * and disconnected from any connected IRQ producers.
> + */
> +void irq_bypass_unregister_consumer(struct irq_bypass_consumer *consumer)
> +{
> +	struct irq_bypass_producer *producer;
> +
> +	might_sleep();
> +
> +	mutex_lock(&lock);
> +
> +	list_for_each_entry(producer, &producers, node) {
> +		if (producer->token == consumer->token) {
> +			__disconnect(producer, consumer);
> +			break;
> +		}
> +	}
> +
> +	list_del(&consumer->node);
> +
> +	mutex_unlock(&lock);
> +	module_put(THIS_MODULE);
> +}
> +EXPORT_SYMBOL_GPL(irq_bypass_unregister_consumer);
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/