From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Subject: Re: [RFC PATCH 1/6] kvm: add device control API Date: Wed, 20 Feb 2013 17:53:20 -0600 Message-ID: <1361404400.31212.12@snotra> References: <20130220212824.GC4700@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; delsp=Yes; format=Flowed Content-Transfer-Encoding: 8BIT Cc: Gleb Natapov , Alexander Graf , , , Christoffer Dall To: Marcelo Tosatti Return-path: In-Reply-To: <20130220212824.GC4700@amt.cnet> (from mtosatti@redhat.com on Wed Feb 20 15:28:24 2013) Content-Disposition: inline Sender: kvm-ppc-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On 02/20/2013 03:28:24 PM, Marcelo Tosatti wrote: > 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? That's best answered by looking at patch 6/6 and discussing the actual attributes that are defined so far. The need to set the base address of the registers is straightforward. When ARM added their special ioctl for this, we discussed it being eventually replaced with a more general API (in fact, that was the reason they put "ARM" in the name). Access to device registers was originally intended for debugging, and eventually migration. It turned out to also be very useful for injecting MSIs -- nothing special needed to be done. It Just Works(tm) the same way it does in hardware, with an MMIO write from a PCI device to a specific MPIC register. Again, irqfd may complicate this, but there's no need for QEMU-generated MSIs to have to take a circuitous path. Finally, there's the interrupt source attribute. Even if we get rid of that, we'll need MPIC-specific documentation on how to flatten the IRQ numberspace, and if we ever have a cascaded PIC situation things could get ugly since there's no way to identify a specific IRQ controller in KVM_IRQ_LINE. > 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). MPIC emulation is entirely in the kernel with this patchset -- though the register that lets you reset cores will likely need to be kicked back to QEMU. > It is then not necessary to set device attributes on a live guest and > deal with the complications associated with that. Which complications? -Scott