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

Christoffer Dall
2013-02-19T15:51:07Z

On Tue, Feb 19, 2013 at 4:24 AM, Gleb Natapov <gleb@redhat.com> wrote:
> 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.

Everything is queued for 3.9, the vgic+timers code is in the arm-soc
tree, so this is not going to change right now, but I can see it
happening for 3.10 if this proposed interface is accepted.

>
>> 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.
>

I agree on using KVM_SET_DEVICE_ATTR for injecting interrupts is a bit
funky, for the ARM uses we would use this for setting the address used
to expose distributor and CPU interfaces to guests, not to inject
interrupt, first approximation.

>>
>> 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.