Re: [RFC PATCH 1/6] kvm: add device control API

[Scott Wood <scottwood@freescale.com>]

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