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

[Marcelo Tosatti <mtosatti@redhat.com>]

On Wed, Feb 20, 2013 at 06:28:24PM -0300, 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?
> 
> > > 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>

I suppose the atomic-test-and-set type operations introduced to ONE_REG ioctls
are one example of such complications.

Avi, can you remind us what are the drawbacks of having separate
userspace/kernel device emulation?