Message-ID: <1340982580.1207.332.camel@bling.home>
Subject: Re: [PATCH v2 5/6] kvm: KVM_EOIFD, an eventfd for EOIs
From: Alex Williamson <alex.williamson@redhat.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: avi@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        jan.kiszka@siemens.com
Date: Fri, 29 Jun 2012 09:09:40 -0600
In-Reply-To: <20120628192927.GB14787@redhat.com>
References: <20120627044758.23698.249.stgit@bling.home>
	 <20120627050952.23698.37235.stgit@bling.home>
	 <20120628192927.GB14787@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3463
Lines: 71

On Thu, 2012-06-28 at 22:29 +0300, Michael S. Tsirkin wrote:
> On Tue, Jun 26, 2012 at 11:10:08PM -0600, Alex Williamson wrote:
> > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> > index b216709..87a2558 100644
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -1987,6 +1987,30 @@ interrupts with those injected through KVM_IRQ_LINE.  IRQFDs created
> >  with KVM_IRQFD_FLAG_LEVEL must also set this flag when de-assiging.
> >  KVM_IRQFD_FLAG_LEVEL support is indicated by KVM_CAP_IRQFD_LEVEL.
> >  
> > +4.77 KVM_EOIFD
> > +
> > +Capability: KVM_CAP_EOIFD
> > +Architectures: x86
> > +Type: vm ioctl
> > +Parameters: struct kvm_eoifd (in)
> > +Returns: 0 on success, -1 on error
> > +
> > +KVM_EOIFD allows userspace to receive EOI notification through an
> > +eventfd for level triggered irqchip interrupts.  Behavior for edge
> > +triggered interrupts is undefined.  kvm_eoifd.fd specifies the eventfd
> > +used for notification and kvm_eoifd.gsi specifies the irchip pin,
> > +similar to KVM_IRQFD.  KVM_EOIFD_FLAG_DEASSIGN is used to deassign
> > +a previously enabled eoifd and should also set fd and gsi to match.
> > +
> > +The KVM_EOIFD_FLAG_LEVEL_IRQFD flag indicates that the EOI is for
> > +a level triggered EOI and the kvm_eoifd structure includes
> > +kvm_eoifd.irqfd, which must be previously configured using KVM_IRQFD
> > +with the KVM_IRQFD_FLAG_LEVEL flag.  This allows both EOI notification
> > +through kvm_eoifd.fd as well as automatically de-asserting level
> > +irqfds on EOI.  Both KVM_EOIFD_FLAG_DEASSIGN and
> > +KVM_EOIFD_FLAG_LEVEL_IRQFD should be used to de-assign an eoifd
> > +initially setup with KVM_EOIFD_FLAG_LEVEL_IRQFD.
> > +
> 
> OK so thinking about this some more, does the below makes sense:
> it is enough to have LEVEL either in IRQFD or in EOIFD but not both.
> I weakly prefer it in EOIFD: when you bind it to irqfd you
> also assign a source id to that irqfd.
> 
> We also can make is a bit clearer: rename KVM_EOIFD_FLAG_LEVEL_IRQFD
> to KVM_EOIFD_FLAG_LEVEL_CLEAR which makes it explicit that we clear
> on EOI and that it is for a level interrupt. The fact that it needs
> an irqfd is less important IMHO.
> Make this flag mandatory for now, we'll see later how to handle
> vcpu filtering and edge.
> 
> How does it sound?

Honestly, I don't like it at all.  We're designing the interface
assuming that we can't modify KVM_IRQFD flags.  Let's do as Avi suggests
and test whether KVM_IRQFD is a beyond broken first.  Given that we do
make use of 1 bit in the flags for de-assert, I'm optimistic that nobody
is leaving random garbage in the rest of it.

Your idea may work, but I really don't like that KVM_EOIFD can reach
across and change the behavior of KVM_IRQFD.  That's not an intuitive
interface.  The LEVEL_IRQFD flag is specifying that the struct kvm_eoifd
includes an irqfd for a level triggered interrupt.  AFAICT, there's no
reason to specify that except to clear it since the EOI notification is
not per source ID.  However, it's still valid to ask for EOI
notification without binding it to a level irqfd, in which case it
really is just EOI notification.  Thanks,

Alex

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