On 25.02.2013, at 14:09, Gleb Natapov wrote: > On Mon, Feb 25, 2013 at 12:11:19PM +1100, Paul Mackerras wrote: >> On Mon, Feb 18, 2013 at 02:21:59PM +0200, 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? x86 >>> obviously support old way and will have to for some, very long, time. >>> ARM vGIC code, that is ready to go upstream, uses old way too. So it will >>> be 2 archs against one. Christoffer do you think the proposed way it >>> better for your needs. Are you willing to make vGIC use it? >> >> In fact there have been two distinct interrupt controller emulations >> for PPC posted recently, Scott's and mine, with quite different >> interfaces. In contrast to Scott's device control API, where the >> operations you would do for different interrupt controllers are quite >> different, mine attempted to define a much more abstract interface for >> interrupt controller emulations, with the idea that an interrupt >> controller subsystem connects a set of interrupt sources, each with >> some state, to a set of interrupt delivery mechanisms, one per vcpu, >> also with some state. >> > I agree with Scott that it is futile to try and come up with generic > irqchip configuration interface and I doubt it is needed from QEMU > or other userspace pov. I looked at proposed KVM_IRQCHIP_SET_SOURCES > interface and while it is possible to pass some information about > pic/ioapic using it there will be a lot of information that will not > fit there. For one there is global irqchips related state and proposed > interface only talk about interrupt sources. Another is that using > generic interface will require us to have a code that translate irqchip > representation into this generic one and back for no apparent gain. > Currently pic/ioapic state is very similar to what HW specification > defines and it is not going to change. > > Looking at your implementation of KVM_IRQCHIP_SET_SOURCES I wounder how > well it work for migration though. The interface suppose to transfer > irqchips state as is, but I see things like that in your code: > > mutex_lock(&ics->lock); > irqp->server = val & KVM_IRQ_SERVER_MASK; > irqp->priority = val >> KVM_IRQ_PRIORITY_SHIFT; > irqp->resend = 0; > irqp->masked_pending = 0; > irqp->asserted = 0; > > Why it is safe to initialize those values to default values during > migration? Wouldn't it be simpler and more correct to just transfer > the whole content of irqp from src to dst? > >> Thus my interface had: >> >> - a KVM_CREATE_IRQCHIP_ARGS ioctl, with an argument structure that >> indicates the overall architecture of the interrupt subsystem, >> >> - KVM_IRQCHIP_SET_SOURCES and KVM_IRQCHIP_GET_SOURCES ioctls, that >> return or modify the state of some set of interrupt sources >> >> - a KVM_REG_PPC_ICP_STATE identifier in the ONE_REG register >> identifier space, that is used to save and restore the per-vcpu >> interrupt presentation state. >> >> Alternatively, I could modify my code to use the existing >> KVM_CREATE_IRQCHIP, KVM_GET_IRQCHIP, and KVM_SET_IRQCHIP ioctls. If I >> were to do that I would want to rename the 'pad' field in struct >> kvm_irqchip to 'nr' and use it with 'chip_id' to identify a range of >> interrupt sources to be affected. I'd also want to keep the ONE_REG >> identifier for the per-vcpu state. > It is preferable to the interface you propose since I do not think your > interface fits other interrupt controllers well. You can put nr field > into dummy[] payload, or replace pad with "union {pad, nr}". > >> >> Or I could change over to using Scott's device control API, though I >> have some objections to doing that. >> >> What is your advice about which interface to use? >> > The ideal situation would be if you and Scott agree on the details about > the interface. If you don't like something about Scott's interface we > can discuss it and shape it to something you agree with or even like. > I already asked Scott to introduce command interface. Scott does not > care about migration, you do, so you can make sure that interface works > for that.
I agree. I really want to see one style used for both devices. If we come to the conclusion that we need to go old-style ioctl payload style, then fine. I obviously prefer fine-grained individual state accessors. But really, even if the "generic interface" only generalizes MPIC and XICS it's already a win for me :). Alex -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html