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

[Scott Wood <scottwood@freescale.com>]

  On 02/20/2013 07:09:49 AM, 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.

You mean like what we did with SREGS, that got deprecated and replaced  
with ONE_REG?

How is writing documentation not creating new interfaces, if the  
documentation is different from what the interface is currently  
understood to do?
Note that Marcelo seems to view KVM_SET_IRQCHIP as effectively being a  
device reset, which is rather different.

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

So basically, you want me to keep this interface but share the ioctl  
number with an older interface? :-P

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

Three *groups* of properties.  One of the property groups is per  
source, and we can have hundreds of sources.  Another exposes the  
register space, which is 64 KiB (admittedly it's somewhat sparse, but  
there's more than 512 bytes of real data in there).  And we don't  
necessarily want to set *everything*.

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

It's either that, or have the data direction of the "chip" field in  
KVM_GET_IRQCHIP not be entirely in the "get" direction.

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

It should probably be get_user() instead, which is pretty fast in the  
absence of a fault.

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

What if I set the address at a time that isn't init/migration (the  
hardware allows moving it, like a PCI BAR)?  Suddenly it becomes not an  
attribute due to how the caller uses it?

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

Which ioctl would go away?

> You need to show us the benefits of the new interface
> vs existing one, not vice versa.

Well, as I said to Marcello, the real reason why we probably need to  
use the existing interface is irqfd.  That doesn't make the device  
control stuff go away.

> > And I really hope you don't want us to do MSIs the x86 way.
> >
> What is wrong with KVM_SIGNAL_MSI? Except that you'll need code to  
> glue it
> to mpic.

We'll have to write extra code for it compared to the current way where  
it works with *zero* code beyond what is wanted for other purposes such  
as debug and (eventually) migration.  At least it's more direct than  
having to establish a GSI route...

> > In the XICS thread, Paul brought up the possibliity of cascaded
> > MPICs.  It's not relevant to the systems we're trying to model, but
> > if one did want to use the in-kernel irqchip interface for that, it
> > would be really nice to be able to operate on a specific MPIC for
> > injection rather than have to come up with some sort of global
> > identifier (above and beyond the minor flattening we'd need to do to
> > represent a single MPIC's interrupts in a flat numberspace).
> >
> ARM encodes information in irq field of KVM_IRQ_LINE like that:
>   bits:  | 31 ... 24 | 23  ... 16 | 15    ...     0 |
>   field: | irq_type  | vcpu_index |   irq_number    |
> Why will similar approach not work?

Well, it conflicts with the GSI routing stuff, and I don't see an irq  
chip ID field...

But otherwise (or assuming you mean to use such an encoding when  
setting up a GSI route), I didn't say this part couldn't be made to  
work.  It will require new kernel code for managing a GSI table in a  
non-APIC way, and a new callback into the device code, but as I've said  
elsewhere I think we need it for irqfd anyway.  If I use KVM_IRQ_LINE  
for injecting interrupts, do you still object to the rest of it?

> > Could an error return be used for cases where the IRQ was not
> > delivered, in the very unlikely event that we want to implement
> > something similar on MPIC?
> We can, but I do not think it will be good API. This condition is not  
> an
> error.

-EBUSY seems appropriate enough...

> >                            Note again that MPIC's decision to use
> > or not use KVM_IRQ_LINE is only about what MPIC does; it is not
> > inherent in the device control API.
> That's the crux of the problem though. MPIC tries to be different just
> for the sake to be different. Why? The only explanation you provide is
> because current API is "silly", not that you cannot implement MPIC  
> with
> it or it will be unnecessary slow, just "silly".

It's not about "silliness" as that this new thing I added for other  
reasons did the job just as well (again, except when it comes to  
irqfd), and avoided the need for a GSI table, etc.  IRQ injection was  
not the main point of the new interface.

> > >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 see no need for a separate ioctl in terms of the underlying
> > infrastructure for distinguishing "attribute" from "write-only
> > command".  I'm open to improvements on what the ioctl is called.
> > It's basically like setting a register on a device, except I was
> > concerned that if we actually called it a "register" that people
> > would take it too literally and think it's only for the architected
> > register state of the emulated device.
> I agree "attribute" is better name than "register", but injecting
> interrupt is not setting an attribute.

It's a dynamic attribute -- the state of the input line.  Better names  
are welcome.  I don't see this difference as enough to warrant separate  
ioctls.

> > >> >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?
> >
> > The latter.  Two wrongs don't make a right, and adding another
> > inextensible, device-specific API is not the answer to the existing
> > APIs being too inextensible and device/arch-specific.  Some portion
> > will always need to be device-specific because we're controlling the
> > creation and of a specific device, but the glue does not need to be.
> >
> This is not "adding another inextensible, device-specific API" vs  
> "adding
> cool generic extensible API" though. It is "using existing  
> inextensible,
> device-specific API" vs "adding cool generic extensible API".

The "existing inextensible device-specific API" doesn't have support  
for this "specific device".  Something new has to be added one way or  
another.

> > >APIs are easy to add and impossible to remove.
> >
> > That's why I want to get it right this time.
> >
> And what if you'll fail?

That's always a possibility of course.  I don't think that's a good  
reason to avoid trying to move in the right direction.

> What if next architecture will bring new
> developer that will proclaim your new interface "silly" since it does  
> not
> allow for device destruction and do not return file descriptor for  
> newly
> created device that userspace can do select on to wait for a device's
> events or mmap memory for fast userspace/device communication?

The device id that gets returned is arbitrary; you could turn it into  
an fd later with no loss of compatibility.

Device destruction would complicate things and I would not support  
requiring all devices to allow it.  If someone wanted to add it for  
certain devices, at the interface level it would just be a new ioctl.

-Scott