linux-kernel - Re: [PATCH v4 5/5] KVM: eventfd: add irq bypass consumer management

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 07 Aug 2015 14:09:15 -0600
From:	Alex Williamson <alex.williamson@...hat.com>
To:	Eric Auger <eric.auger@...aro.org>
Cc:	eric.auger@...com, feng.wu@...el.com, pbonzini@...hat.com,
	linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.cs.columbia.edu,
	kvm@...r.kernel.org, avi.kivity@...il.com, mtosatti@...hat.com,
	joro@...tes.org, marc.zyngier@....com, christoffer.dall@...aro.org,
	linux-kernel@...r.kernel.org, patches@...aro.org
Subject: Re: [PATCH v4 5/5] KVM: eventfd: add irq bypass consumer management

On Mon, 2015-08-03 at 19:20 +0200, Eric Auger wrote:
> This patch adds the registration/unregistration of an
> irq_bypass_consumer on irqfd assignment/deassignment.
> 
> Signed-off-by: Eric Auger <eric.auger@...aro.org>
> Signed-off-by: Feng Wu <feng.wu@...el.com>
> 
> ---
> 
> v2 -> v3 (Feng Wu):
> - Use kvm_arch_irq_bypass_start
> - Remove kvm_arch_irq_bypass_update
> - Add member 'struct irq_bypass_producer *producer' in
>   'struct kvm_kernel_irqfd', it is needed by posted interrupt.
> - Remove 'irq_bypass_unregister_consumer' in kvm_irqfd_deassign()
> 
> v1 -> v2:
> - populate of kvm and gsi removed
> - unregister the consumer on irqfd_shutdown
> ---
>  include/linux/kvm_irqfd.h |  2 ++
>  virt/kvm/eventfd.c        | 10 ++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/include/linux/kvm_irqfd.h b/include/linux/kvm_irqfd.h
> index f926b39..0c1de05 100644
> --- a/include/linux/kvm_irqfd.h
> +++ b/include/linux/kvm_irqfd.h
> @@ -64,6 +64,8 @@ struct kvm_kernel_irqfd {
>  	struct list_head list;
>  	poll_table pt;
>  	struct work_struct shutdown;
> +	struct irq_bypass_consumer consumer;
> +	struct irq_bypass_producer *producer;
>  };
>  
>  #endif /* __LINUX_KVM_IRQFD_H */
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index 647ffb8..08855de 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -35,6 +35,7 @@
>  #include <linux/srcu.h>
>  #include <linux/slab.h>
>  #include <linux/seqlock.h>
> +#include <linux/irqbypass.h>
>  #include <trace/events/kvm.h>
>  
>  #include <kvm/iodev.h>
> @@ -140,6 +141,7 @@ irqfd_shutdown(struct work_struct *work)
>  	/*
>  	 * It is now safe to release the object's resources
>  	 */
> +	irq_bypass_unregister_consumer(&irqfd->consumer);
>  	eventfd_ctx_put(irqfd->eventfd);
>  	kfree(irqfd);
>  }
> @@ -380,6 +382,14 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
>  	 */
>  	fdput(f);
>  
> +	irqfd->consumer.token = (void *)irqfd->eventfd;
> +	irqfd->consumer.add_producer = kvm_arch_irq_bypass_add_producer;
> +	irqfd->consumer.del_producer = kvm_arch_irq_bypass_del_producer;
> +	irqfd->consumer.stop = kvm_arch_irq_bypass_stop;
> +	irqfd->consumer.start = kvm_arch_irq_bypass_start;
> +	ret = irq_bypass_register_consumer(&irqfd->consumer);
> +	WARN_ON(ret);

This seems like a lazy way to handle this error.  What is the stack
trace from this WARN_ON going to tell us that we didn't already know?
If we get the WARN_ON, it's probably means that an incompatible producer
registered the token first.  It means we can't do bypass, but it doesn't
tell us anything about whether bypass is or is not enabled.  Wouldn't a
pr_info/debug() suffice for that?  Thanks,

Alex

> +
>  	return 0;
>  
>  fail:



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