On Wed, Feb 20, 2013 at 03:09:49PM +0200, Gleb Natapov wrote: > On Tue, Feb 19, 2013 at 03:16:37PM -0600, Scott Wood wrote: > > On 02/19/2013 06:24:18 AM, Gleb Natapov wrote: > > >On Mon, Feb 18, 2013 at 05:01:40PM -0600, Scott Wood wrote: > > >> 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. > > > > There's a difference between putting a pointer in an ioctl control > > structure that is specifically documented as being that way (as in > > ONE_REG), versus taking an ioctl that claims to be setting/getting a > > blob of state and embedding pointers in it. It would be like > > sticking a pointer in the attribute payload of this API, which I > > think is something to be discouraged. > If documentation is what differentiate for you between silly and smart > then write documentation instead of new interfaces. > > KVM_SET_IRQCHIP/KVM_GET_IRQCHIP is defined to operate on blob of data on > x86, nothing prevent you from adding MPIC specifics to the interface, > Add mpic state into kvm_irqchip structure and if 512 bytes is not enough > for you to transfer the state put pointers there and _document_ them. > But with 512 bytes you can transfer properties inline, so you probably > do not need pointer there anyway. I see you have three properties 2 of > them 32bit and one 64bit. > > > It'd also be using > > KVM_SET_IRQCHIP to read data, which is the sort of thing you object > > to later on regarding KVM_IRQ_LINE_STATUS. > > > Do not see why. > > > Then there's the silliness of transporting 512 bytes just to read a > > descriptor for transporting something else. > > > Yes, agree. But is this enough of a reason to introduce entirely new > interface? Is it on performance critical path? Doubt it, unless you > abuse the interface to send interrupts, but then isn't it silty to > do copy_from_user() twice to inject an interrupt like proposed interface > does? > > > >For signaling irqs (I think this is what you mean by "commands") > > >we have KVM_IRQ_LINE. > > > > It's one type of command. Another is setting the address. Another > > is writing to registers that have side effects (this is how MSI > > injection is done on MPIC, just as in real hardware). > > > Setting the address is setting an attribute. Sending MSI is a command. > Things you set/get during init/migration are attributes. Things you do > to cause side-effects are commands.
Yes, it would be good to restrict what can be done via the interface (to avoid abstracting away problems). At a first glance, i would say it should allow for "initial configuration of device state", such as registers etc. Why exactly you need to set attributes Scott? > > What is the benefit of KVM_IRQ_LINE over what MPIC does? What real > > (non-glue/wrapper) code can become common? > > > No new ioctl with exactly same result (well actually even faster since > less copying is done). You need to show us the benefits of the new interface > vs existing one, not vice versa. Also related to this question is the point of avoiding the implementation of devices to be spread across userspace and the kernel (this is one point Avi brought up often). If the device emulation is implemented entirely in the kernel, all is necessary are initial values of device registers (and retrieval of those values later for savevm/migration). It is then not necessary to set device attributes on a live guest and deal with the complications associated with that. <snip> -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
