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

[Scott Wood <scottwood@freescale.com>]

  On 02/21/2013 02:22:09 AM, Gleb Natapov wrote:
> On Wed, Feb 20, 2013 at 08:05:12PM -0600, Scott Wood wrote:
> >  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?
> >
> SREGS is implemented by ppc. I see ONE_REG as addition to REGS
> interface. You can access all register at once or you can access them
> one by one. If there is a need we can add MULTIPLE_REGS that will get
> list of requested REGS.

http://www.spinics.net/lists/kvm-ppc/msg04876.html
http://www.spinics.net/lists/kvm-ppc/msg05842.html

> The interface is not over generic i.e it does
> not try to replace KVM_RUN by writing special register.

Sigh.

> >                                                    And we
> > don't necessarily want to set *everything*.
> What are those cases? You do need to on reset/migration.

Why do we want to set all the registers on reset, rather than tell the  
in-kernel device to reset?  The default state came from the kernel in  
the first place on irqchip creation...

> > >>                                        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.
> >
> Still do not follow. Example?

struct kvm_irqchip has "chip_id", "pad", and "chip".  "pad" is  
insufficient to communicate attribute type plus a pointer.  So if we  
want to provide a pointer for the kernel to write the attribute into,  
it has to read from memory that the ioctl definition suggests should  
only be written to.

> > >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's the interface for guest to move it?

Some non-MPIC registers called CCSRBARH, CCSRBARL, and CCSRBAR.

> Why it goes via userspace?

Because the mechanism in question doesn't just move MPIC.  It moves a  
big block of a bunch of different devices all at once.

> You can move APIC base too, but this does not involve userspace. But
> even if you do go via userspace, it is just a guest asking to change  
> device
> configuration, so using SET_ATTR to set new configuration is fine.
> 
> > >> 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?
> >
> Those that you propose in your new interface.

No, they wouldn't.  At most one MPIC attribute group would go away  
(though as I've noted it would still be useful to be able to "get"  
those attributes for debugging).

> > >> 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...
> If just writing a register cause MSI to be send how do you distinguish
> between write that should send MSI and write that is done on migration
> to transfer current value?

It is a write-only command register.  The registers that contain the  
state are elsewhere.

Again, we do not currently support migration on MPIC.  It is a very low  
priority for embedded.  We do not wish to rule it out entirely, but it  
most likely would require adding more state accesors.

> We had that problem with MSRs on x86. We had
> to, eventually, add a flag that tells us the reason of MSR access.

The equivalent to that flag would be using the right kind of accessor  
for what you want to do (simulated guest access versus backdoor state  
access).

> > >> 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...
> It does :( Can't say I am happy about it, but I skipped the discussion
> about the interface back then and it is too late to complain now.  
> Since,
> as you notices, irqfd interfaces with irq routing I wonder what's ARM
> plan about it. But if you choose to go ARM way the format is ARM  
> specific,
> so you can use your own encoding and put irq chip information there.

Well, we do want to support irqfd, so I don't think we'll be following  
ARM here.

> > 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?
> The rest of what, proposed interface? There are two separate  
> discussions
> happening here interleaved. First is "do we need to introduce new  
> generic
> interface for device creation when existing one, albeit not ideal,  
> can be
> used" and I am OK with that as long as ARM moves to it for 3.10,  
> although
> I would prefer to have some example of what this interface will be  
> used
> for besides irq chips otherwise it will be just another way to create
> irqchip.

We need a new way to create irqchips anyway, even if it's just what the  
XICS patchset adds (KVM_CREATE_IRQCHIP_ARGS, which is similar to  
KVM_CREATE_DEVICE except it doesn't return an identifier for operating  
on a specific device).  And of course we want to sort this out before  
either patchset gets merged, so we don't end up adding both methods.  I  
suspect the XICS patchset flew under your radar because it has "PPC:"  
in front of it and the subject line doesn't mention the ioctl, but I'm  
not the only one that felt the need for a few new ioctls.

As for other types of devices, x86 has i8254.c emulated in-kernel -- I  
know that's not going to switch to the new interface, but it could have  
if it existed back then.  I can also see creating an in-kernel  
emulation device for doing MMIO filtering on some piece of embedded  
hardware that guests need to access with reasonable performance, but  
the hardware desginers screwed up the protection slightly (e.g. put  
other things in the same 4K page).  We've done such filtering before in  
our standalone hypervisor; the question is whether it happens to  
anything with enough performance requirements to be done in the kernel.

> Second one is "how the interface should look like". And here I
> think that strong distinction is needed between setting the attributes
> and sending commands with side effects for reasons explained all over
> this ml thread.

OK, so let's just call them "commands".  I like the split into "read"  
and "write" commands, especially when most of the commands naturally  
come in such pairs, but if you don't like that part it can be reduced  
to a single read/write command (and then we'd define separate set/get  
commands where appropriate).

Note that the XICS patchset also involves device commands.  It does it  
by passing any unknown vm ioctl to the irqchip (XICS implements  
KVM_IRQCHIP_GET_SOURCES and KVM_IRQCHIP_SET_SOURCES in addition to  
KVM_IRQ_LINE).  Obviously both ways can work; I've given my reasons  
elsewhere in the thread for preferring something that doesn't require a  
new ioctl for every device command.

> > 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.
> Having generic interface for device creation and then make some  
> devices
> special by allowing them to be used with KVM_IRQ_LINE makes little
> sense,

Well, we'd want to document which devices tie into which generic  
interfaces, and which device is selected if multiple such devices are  
created (if that is allowed for a particular device class).

For KVM_IRQ_LINE, we could perhaps use the device id as the irqchip id  
in kvm_irq_routing_irqchip (or, have an attribute/command that  
retrieves the id to be used there).  Unfortunately there is no irqchip  
field in kvm_irq_routing_msi, though since it's basically a command to  
write to an arbitrary MMIO address, maybe it could just be implemented  
that way?

> > >> 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.
> As long as you use the same attribute for migration and interrupt  
> injection
> purpose I do. If you use separate attributes for migration and  
> interrupt
> injection then not having separate ioctl is just a hack.

Why is it a hack?  Is it also a hack to not use a separate ioctl to  
reset the device, to move its address map, etc?

-Scott