On Tue, Feb 19, 2013 at 4:24 AM, Gleb Natapov <gleb@xxxxxxxxxx> wrote: > On Mon, Feb 18, 2013 at 05:01:40PM -0600, Scott Wood wrote: >> On 02/18/2013 06:21:59 AM, Gleb Natapov wrote: >> >Copying Christoffer since ARM has in kernel irq chip too. >> > >> >On Wed, Feb 13, 2013 at 11:49:15PM -0600, Scott Wood wrote: >> >> Currently, devices that are emulated inside KVM are configured in a >> >> hardcoded manner based on an assumption that any given architecture >> >> only has one way to do it. If there's any need to access device >> >state, >> >> it is done through inflexible one-purpose-only IOCTLs (e.g. >> >> KVM_GET/SET_LAPIC). Defining new IOCTLs for every little thing is >> >> cumbersome and depletes a limited numberspace. >> >> >> >> This API provides a mechanism to instantiate a device of a certain >> >> type, returning an ID that can be used to set/get attributes of the >> >> device. Attributes may include configuration parameters (e.g. >> >> register base address), device state, operational commands, etc. It >> >> is similar to the ONE_REG API, except that it acts on devices rather >> >> than vcpus. >> >You are not only provide different way to create in kernel irq >> >chip you >> >also use an alternate way to trigger interrupt lines. Before going >> >into >> >interface specifics lets think about whether it is really worth it? >> >> Which "it" do you mean here? >> > "It" is adding of a new interface if it will have only one user while > existing one can be adjusted for your needs. If ARM people are on board > I feel much better about it. The question is how on board they are :) > are they willing to make vGIC use it before upstream merge? vGIC is > merged separately from KVM code, so it will not affect merging of KVM > itself. Everything is queued for 3.9, the vgic+timers code is in the arm-soc tree, so this is not going to change right now, but I can see it happening for 3.10 if this proposed interface is accepted. > >> The ability to set/get attributes is needed. Sorry, but "get or set >> one blob of data, up to 512 bytes, for the entire irqchip" is just >> not good enough -- assuming you don't want us to start sticking >> pointers and commands in *that* data. :-) >> > Proposed interface sticks pointers into ioctl data, so why doing the same > for KVM_SET_IRQCHIP/KVM_GET_IRQCHIP makes you smile. For signaling irqs (I > think this is what you mean by "commands") we have KVM_IRQ_LINE. > >> If you mean the way to inject interrupts, it's simpler this way. >> Why go out of our way to inject common glue code into a >> communication path between hw/kvm/mpic.c in QEMU and >> arch/powerpc/kvm/mpic.c in KVM? Or rather, why make that common >> glue be specific to this one function when we could reuse the same >> communication glue used for other things, such as device state? > You will need glue anyway and I do no see how amount of it is much > different one way or the other. Gluing qemu_set_irq() to > ioctl(KVM_IRQ_LINE) or ioctl(KVM_SET_DEVICE_ATTR) is not much different. > > Of course, since the interface you propose is not irq chip specific we > need non irq chip specific way to talk to it. But how do you propose > to model things like KVM_IRQ_LINE_STATUS with KVM_SET_DEVICE_ATTR? > KVM_SET_DEVICE_ATTR needs to return data back and getting data back from > "set" ioctl is strange. Other devices may get other commands that need > response, so if we design generic interface we should take it into > account. I think using KVM_SET_DEVICE_ATTR to inject interrupts is a > misnomer, you do not set internal device attribute, you toggle external > input. My be another ioctl KVM_SEND_DEVICE_COMMAND is needed. > I agree on using KVM_SET_DEVICE_ATTR for injecting interrupts is a bit funky, for the ARM uses we would use this for setting the address used to expose distributor and CPU interfaces to guests, not to inject interrupt, first approximation. >> >> And that's just for regular interrupts. MSIs are vastly simpler on >> MPIC than what x86 does. >> >> >x86 obviously support old way and will have to for some, very >> >long, time. >> >> Sure. >> >> >ARM vGIC code, that is ready to go upstream, uses old way too. So >> >it will >> >be 2 archs against one. >> >> I wasn't aware that that's how it worked. :-P >> > What worked? That vGIC uses existing interface or that non generic > interface used by many arches wins generic one used by only one arch? > >> I was trying to be considerate by not making the entire thing >> gratuitously PPC or MPIC specific, as some others seem inclined to >> do (e.g. see irqfd and APIC). We already had a discussion on ARM's >> "set address" ioctl and rather than extend *that* interface, they >> preferred to just stick something ARM-specific in ASAP with the >> understanding that it would be replaced (or more accurately, kept >> around as a thin wrapper around the new stuff) later. >> > I am not against generic interfaces in general and proposed one in > particular (I have comments about it but this is for other emails), > I am trying to make sure it will be used by more than one user before > committing to it. APIs are easy to add and impossible to remove. > >> >Christoffer do you think the proposed way it >> >better for your needs. Are you willing to make vGIC use it? >> > >> >Scott, what other devices are you planning to support with this >> >interface? >> >> At the moment I do not have plans for other devices, though what >> does it hurt for the capability to be there? >> >> -Scott > > -- > Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
