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

[Gleb Natapov <gleb@redhat.com>]

On Mon, Feb 18, 2013 at 05:01:40PM -0600, Scott Wood wrote:
> On 02/18/2013 06:21:59 AM, 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?
> 
> Which "it" do you mean here?
> 
"It" is adding of a new interface if it will have only one user while
existing one can be adjusted for your needs. If ARM people are on board
I feel much better about it. The question is how on board they are :)
are they willing to make vGIC use it before upstream merge? vGIC is
merged separately from KVM code, so it will not affect merging of KVM
itself.

> 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. For signaling irqs (I
think this is what you mean by "commands") we have KVM_IRQ_LINE.

> If you mean the way to inject interrupts, it's simpler this way.
> Why go out of our way to inject common glue code into a
> communication path between hw/kvm/mpic.c in QEMU and
> arch/powerpc/kvm/mpic.c in KVM?  Or rather, why make that common
> glue be specific to this one function when we could reuse the same
> communication glue used for other things, such as device state?
You will need glue anyway and I do no see how amount of it is much
different one way or the other. Gluing qemu_set_irq() to
ioctl(KVM_IRQ_LINE) or ioctl(KVM_SET_DEVICE_ATTR) is not much different.

Of course, since the interface you propose is not irq chip specific we
need non irq chip specific way to talk to it. But how do you propose
to model things like KVM_IRQ_LINE_STATUS with KVM_SET_DEVICE_ATTR?
KVM_SET_DEVICE_ATTR needs to return data back and getting data back from
"set" ioctl is strange. Other devices may get other commands that need
response, so if we design generic interface we should take it into
account. I think using KVM_SET_DEVICE_ATTR to inject interrupts is a
misnomer, you do not set internal device attribute, you toggle external
input. My be another ioctl KVM_SEND_DEVICE_COMMAND is needed.

> 
> And that's just for regular interrupts.  MSIs are vastly simpler on
> MPIC than what x86 does.
> 
> >x86 obviously support old way and will have to for some, very
> >long, time.
> 
> Sure.
> 
> >ARM vGIC code, that is ready to go upstream, uses old way too. So
> >it will
> >be 2 archs against one.
> 
> I wasn't aware that that's how it worked. :-P
> 
What worked? That vGIC uses existing interface or that non generic
interface used by many arches wins generic one used by only one arch?

> I was trying to be considerate by not making the entire thing
> gratuitously PPC or MPIC specific, as some others seem inclined to
> do (e.g. see irqfd and APIC).  We already had a discussion on ARM's
> "set address" ioctl and rather than extend *that* interface, they
> preferred to just stick something ARM-specific in ASAP with the
> understanding that it would be replaced (or more accurately, kept
> around as a thin wrapper around the new stuff) later.
> 
I am not against generic interfaces in general and proposed one in
particular (I have comments about it but this is for other emails),
I am trying to make sure it will be used by more than one user before
committing to it. APIs are easy to add and impossible to remove.

> >Christoffer do you think the proposed way it
> >better for your needs. Are you willing to make vGIC use it?
> >
> >Scott, what other devices are you planning to support with this
> >interface?
> 
> At the moment I do not have plans for other devices, though what
> does it hurt for the capability to be there?
> 
> -Scott

--
			Gleb.