Patchwork [v5,0/8] In-kernel XICS interrupt controller emulation

login
register
mail settings
Submitter Paul Mackerras
Date April 18, 2013, 6:29 a.m.
Message ID <20130418062926.GA25033@drongo>
Download mbox | patch
Permalink /patch/237445/
State New
Headers show

Comments

Paul Mackerras - April 18, 2013, 6:29 a.m.
This is a repost of my patch series implementing in-kernel emulation
of the XICS interrupt controller architecture defined in PAPR (Power
Architecture Platform Requirements, the document that defines IBM's
pSeries platform architecture).  This version of the patch series uses
the latest device API as posted by Scott Wood, that is, i.e., the
version where the core device code provides the file descriptor and
ioctl handler.  I have structured the series so that the API is added
by the last two patches, so as to be able to accommodate any future
revisions to the device API with minimal changes.

The series is based on Alex Graf's kvm-ppc-queue branch with Scott
Wood's recent patch series applied on top, together with the patch
below to allow it to build with CONFIG_KVM_MPIC=n.

The API defined here uses KVM_CREATE_DEVICE to create the XICS,
KVM_DEVICE_SET_ATTR/KVM_DEVICE_GET_ATTR to manipulate the interrupt
sources (for initialization and migration), a new KVM_CAP_IRQ_XICS
capability to connect vcpus to the XICS, a new identifier
KVM_REG_PPC_ICP_STATE for the one-reg interface to get and set
per-vcpu state, and the existing KVM_IRQ_LINE ioctl to assert and
deassert interrupt sources.

This version also cleans up some checkpatch.pl errors and clarifies
the lifetime rules for the various objects.  There are two checkpatch
warnings for long lines, but they are long because they have long
strings in them, and if I break the strings over two lines then
checkpatch warns about that.

Paul.

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Graf - April 26, 2013, 2:30 p.m.
On 18.04.2013, at 08:29, Paul Mackerras wrote:

> This is a repost of my patch series implementing in-kernel emulation
> of the XICS interrupt controller architecture defined in PAPR (Power
> Architecture Platform Requirements, the document that defines IBM's
> pSeries platform architecture).  This version of the patch series uses
> the latest device API as posted by Scott Wood, that is, i.e., the
> version where the core device code provides the file descriptor and
> ioctl handler.  I have structured the series so that the API is added
> by the last two patches, so as to be able to accommodate any future
> revisions to the device API with minimal changes.
> 
> The series is based on Alex Graf's kvm-ppc-queue branch with Scott
> Wood's recent patch series applied on top, together with the patch
> below to allow it to build with CONFIG_KVM_MPIC=n.
> 
> The API defined here uses KVM_CREATE_DEVICE to create the XICS,
> KVM_DEVICE_SET_ATTR/KVM_DEVICE_GET_ATTR to manipulate the interrupt
> sources (for initialization and migration), a new KVM_CAP_IRQ_XICS
> capability to connect vcpus to the XICS, a new identifier
> KVM_REG_PPC_ICP_STATE for the one-reg interface to get and set
> per-vcpu state, and the existing KVM_IRQ_LINE ioctl to assert and
> deassert interrupt sources.
> 
> This version also cleans up some checkpatch.pl errors and clarifies
> the lifetime rules for the various objects.  There are two checkpatch
> warnings for long lines, but they are long because they have long
> strings in them, and if I break the strings over two lines then
> checkpatch warns about that.

Very nice patch set. I've applie 1-7 of it to kvm-ppc-queue. So they will hopefully make it to 3.10.

Please check for 8/8 whether

  a) You want to have a released kernel version without irq routing (irqfd) support. It makes user space's life harder, because you need to maintain backwards compatibility.

  b) Please rebase on top of the current state of things, especially the changed lifecycle assumptions. Devices should now just live until the vm gets destroyed. It gives me way less headaches.


Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood - April 26, 2013, 5:13 p.m.
On 04/26/2013 09:30:37 AM, Alexander Graf wrote:
> 
> On 18.04.2013, at 08:29, Paul Mackerras wrote:
> 
> > This is a repost of my patch series implementing in-kernel emulation
> > of the XICS interrupt controller architecture defined in PAPR (Power
> > Architecture Platform Requirements, the document that defines IBM's
> > pSeries platform architecture).  This version of the patch series  
> uses
> > the latest device API as posted by Scott Wood, that is, i.e., the
> > version where the core device code provides the file descriptor and
> > ioctl handler.  I have structured the series so that the API is  
> added
> > by the last two patches, so as to be able to accommodate any future
> > revisions to the device API with minimal changes.
> >
> > The series is based on Alex Graf's kvm-ppc-queue branch with Scott
> > Wood's recent patch series applied on top, together with the patch
> > below to allow it to build with CONFIG_KVM_MPIC=n.
> >
> > The API defined here uses KVM_CREATE_DEVICE to create the XICS,
> > KVM_DEVICE_SET_ATTR/KVM_DEVICE_GET_ATTR to manipulate the interrupt
> > sources (for initialization and migration), a new KVM_CAP_IRQ_XICS
> > capability to connect vcpus to the XICS, a new identifier
> > KVM_REG_PPC_ICP_STATE for the one-reg interface to get and set
> > per-vcpu state, and the existing KVM_IRQ_LINE ioctl to assert and
> > deassert interrupt sources.
> >
> > This version also cleans up some checkpatch.pl errors and clarifies
> > the lifetime rules for the various objects.  There are two  
> checkpatch
> > warnings for long lines, but they are long because they have long
> > strings in them, and if I break the strings over two lines then
> > checkpatch warns about that.
> 
> Very nice patch set. I've applie 1-7 of it to kvm-ppc-queue. So they  
> will hopefully make it to 3.10.
> 
> Please check for 8/8 whether
> 
>   a) You want to have a released kernel version without irq routing  
> (irqfd) support. It makes user space's life harder, because you need  
> to maintain backwards compatibility.
> 
>   b) Please rebase on top of the current state of things, especially  
> the changed lifecycle assumptions. Devices should now just live until  
> the vm gets destroyed. It gives me way less headaches.

Also please note that we no longer hold kvm->lock during device  
creation, so your EEXIST check looks racy.

-Scott
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Mackerras - April 26, 2013, 11:52 p.m.
On Fri, Apr 26, 2013 at 12:13:33PM -0500, Scott Wood wrote:

> Also please note that we no longer hold kvm->lock during device
> creation, so your EEXIST check looks racy.

Huh?  Patch 2/8 adds this code:

+       mutex_lock(&kvm->lock);
+       if (kvm->arch.xics)
+               ret = -EEXIST;
+       else
+               kvm->arch.xics = xics;
+       mutex_unlock(&kvm->lock);

How is that racy?

Paul.
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood - April 27, 2013, 12:08 a.m.
On 04/26/2013 06:52:47 PM, Paul Mackerras wrote:
> On Fri, Apr 26, 2013 at 12:13:33PM -0500, Scott Wood wrote:
> 
> > Also please note that we no longer hold kvm->lock during device
> > creation, so your EEXIST check looks racy.
> 
> Huh?  Patch 2/8 adds this code:
> 
> +       mutex_lock(&kvm->lock);
> +       if (kvm->arch.xics)
> +               ret = -EEXIST;
> +       else
> +               kvm->arch.xics = xics;
> +       mutex_unlock(&kvm->lock);
> 
> How is that racy?

Sorry, it's not -- I must have been looking at the previous version of  
the patchset by mistake. :-(

-Scott

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Mackerras - April 27, 2013, 8:36 a.m.
On Fri, Apr 26, 2013 at 04:30:37PM +0200, Alexander Graf wrote:
> Very nice patch set. I've applie 1-7 of it to kvm-ppc-queue. So they will hopefully make it to 3.10.
> 
> Please check for 8/8 whether
> 
>   a) You want to have a released kernel version without irq routing (irqfd) support. It makes user space's life harder, because you need to maintain backwards compatibility.

If we do a version without irq routing and later add it, old userspace
should still work, since KVM_IRQ_LINE still works, right?  New
userspace on an old kernel can test the KVM_CAP_IRQ_ROUTING capability
to see if it can use irqfd.  So on the whole, I would like to get it
in, provided of course I can get it tested and sent out before the
window closes.

>   b) Please rebase on top of the current state of things, especially the changed lifecycle assumptions. Devices should now just live until the vm gets destroyed. It gives me way less headaches.

OK, I've done that, but I need to grab David Gibson to get a qemu that
knows about the new API so I can test it.

Regards,
Paul.
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Graf - April 28, 2013, 9:50 a.m.
On 27.04.2013, at 10:36, Paul Mackerras wrote:

> On Fri, Apr 26, 2013 at 04:30:37PM +0200, Alexander Graf wrote:
>> Very nice patch set. I've applie 1-7 of it to kvm-ppc-queue. So they will hopefully make it to 3.10.
>> 
>> Please check for 8/8 whether
>> 
>>  a) You want to have a released kernel version without irq routing (irqfd) support. It makes user space's life harder, because you need to maintain backwards compatibility.
> 
> If we do a version without irq routing and later add it, old userspace
> should still work, since KVM_IRQ_LINE still works, right?  New
> userspace on an old kernel can test the KVM_CAP_IRQ_ROUTING capability
> to see if it can use irqfd.  So on the whole, I would like to get it

User space would have to know about 2 different ways of setting interrupts, one through IRQ_LINE and one through whatever XICS device specific ioctl you create :).

This really depends on how quickly you think you could have irqfd support working. If you think it's ready for 3.11, I'd be inclined to say that it's easier for everyone if we don't have to provide any backwards compatibility at all. Then QEMU doesn't need to learn about the irq line setting path that is only ever available on 3.10.

> in, provided of course I can get it tested and sent out before the
> window closes.
> 
>>  b) Please rebase on top of the current state of things, especially the changed lifecycle assumptions. Devices should now just live until the vm gets destroyed. It gives me way less headaches.
> 
> OK, I've done that, but I need to grab David Gibson to get a qemu that
> knows about the new API so I can test it.

Yeah, please sync with him on this whole thing too. Someone will have to write the compat code ;).


Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Mackerras - April 28, 2013, 12:05 p.m.
On Sun, Apr 28, 2013 at 11:50:49AM +0200, Alexander Graf wrote:
> 
> On 27.04.2013, at 10:36, Paul Mackerras wrote:
> 
> > On Fri, Apr 26, 2013 at 04:30:37PM +0200, Alexander Graf wrote:
> >> Very nice patch set. I've applie 1-7 of it to kvm-ppc-queue. So they will hopefully make it to 3.10.
> >> 
> >> Please check for 8/8 whether
> >> 
> >>  a) You want to have a released kernel version without irq routing (irqfd) support. It makes user space's life harder, because you need to maintain backwards compatibility.
> > 
> > If we do a version without irq routing and later add it, old userspace
> > should still work, since KVM_IRQ_LINE still works, right?  New
> > userspace on an old kernel can test the KVM_CAP_IRQ_ROUTING capability
> > to see if it can use irqfd.  So on the whole, I would like to get it
> 
> User space would have to know about 2 different ways of setting interrupts, one through IRQ_LINE and one through whatever XICS device specific ioctl you create :).

Ummm, no, userspace uses KVM_IRQ_LINE either way.  Since I don't
define CONFIG_HAVE_KVM_IRQCHIP or CONFIG_HAVE_KVM_IRQ_ROUTING, we
don't compile in virt/kvm/irqchip.c and thus we don't get a definition
of kvm_set_irq() (note that in your tree as it stands, it's only by
grace of gcc's optimizations that we don't get a link error when
CONFIG_KVM_MPIC=n, since we have the code in
arch/powerpc/kvm/powerpc.c that calls kvm_set_irq() unconditionally).

So I added a definition of kvm_set_irq() to book3s_xics.c in the patch
I posted recently, meaning that userspace can in fact use KVM_IRQ_LINE
even though I don't yet support irqfd or irq routing.

> > OK, I've done that, but I need to grab David Gibson to get a qemu that
> > knows about the new API so I can test it.
> 
> Yeah, please sync with him on this whole thing too. Someone will have to write the compat code ;).

I have a version of qemu from him that uses KVM_CREATE_DEVICE to
create the XICS and KVM_IRQ_LINE to control the interrupt inputs, and
it all works just fine.

Regards,
Paul.
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Graf - April 30, 2013, 9:59 a.m.
On 28.04.2013, at 14:05, Paul Mackerras wrote:

> On Sun, Apr 28, 2013 at 11:50:49AM +0200, Alexander Graf wrote:
>> 
>> On 27.04.2013, at 10:36, Paul Mackerras wrote:
>> 
>>> On Fri, Apr 26, 2013 at 04:30:37PM +0200, Alexander Graf wrote:
>>>> Very nice patch set. I've applie 1-7 of it to kvm-ppc-queue. So they will hopefully make it to 3.10.
>>>> 
>>>> Please check for 8/8 whether
>>>> 
>>>> a) You want to have a released kernel version without irq routing (irqfd) support. It makes user space's life harder, because you need to maintain backwards compatibility.
>>> 
>>> If we do a version without irq routing and later add it, old userspace
>>> should still work, since KVM_IRQ_LINE still works, right?  New
>>> userspace on an old kernel can test the KVM_CAP_IRQ_ROUTING capability
>>> to see if it can use irqfd.  So on the whole, I would like to get it
>> 
>> User space would have to know about 2 different ways of setting interrupts, one through IRQ_LINE and one through whatever XICS device specific ioctl you create :).
> 
> Ummm, no, userspace uses KVM_IRQ_LINE either way.  Since I don't
> define CONFIG_HAVE_KVM_IRQCHIP or CONFIG_HAVE_KVM_IRQ_ROUTING, we
> don't compile in virt/kvm/irqchip.c and thus we don't get a definition
> of kvm_set_irq() (note that in your tree as it stands, it's only by
> grace of gcc's optimizations that we don't get a link error when
> CONFIG_KVM_MPIC=n, since we have the code in
> arch/powerpc/kvm/powerpc.c that calls kvm_set_irq() unconditionally).
> 
> So I added a definition of kvm_set_irq() to book3s_xics.c in the patch
> I posted recently, meaning that userspace can in fact use KVM_IRQ_LINE
> even though I don't yet support irqfd or irq routing.

That means you'll have to make your default map compatible with what you expose there now.

> 
>>> OK, I've done that, but I need to grab David Gibson to get a qemu that
>>> knows about the new API so I can test it.
>> 
>> Yeah, please sync with him on this whole thing too. Someone will have to write the compat code ;).
> 
> I have a version of qemu from him that uses KVM_CREATE_DEVICE to
> create the XICS and KVM_IRQ_LINE to control the interrupt inputs, and
> it all works just fine.

Please get him to ack the approach of getting 2 interfaces later. Of course what you wrote now works. I'm not concerned about that. I'm concerned about tomorrow :).


Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 1d3888b..bd41eea 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -466,9 +466,11 @@  void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu)
 	kvmppc_remove_vcpu_debugfs(vcpu);
 
 	switch (vcpu->arch.irq_type) {
+#ifdef CONFIG_KVM_MPIC
 	case KVMPPC_IRQ_MPIC:
 		kvmppc_mpic_disconnect_vcpu(vcpu->arch.mpic, vcpu);
 		break;
+#endif
 	}
 
 	kvmppc_core_vcpu_free(vcpu);