Message-ID: <55A3BBD1.40305@linaro.org>
Date: Mon, 13 Jul 2015 15:23:29 +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> <559D3597.2080706@linaro.org>
In-Reply-To: <559D3597.2080706@linaro.org>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 15406
Lines: 451

Hi Alex,
On 07/08/2015 04:37 PM, Eric Auger wrote:
> 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>
Given the amount of code I wrote compared to you, I think you can simply
drop my Signed-off ;-)
>>  - 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?

Would you accept to introduce this active boolean back? Any other
suggestion?

Best Regards

Eric
> 
>>  - 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/