| Message ID | 20130418062926.GA25033@drongo (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);