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

Gleb Natapov
2013-02-24T15:46:17Z

On Thu, Feb 21, 2013 at 08:17:54PM -0600, Scott Wood wrote:
> 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
>
If Alex prefers ONE_REG interface this is his call :)

> >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...
>
I have nothing against telling in-kernel device to reset provided there
is a way to do so, which current interface lacks. Reset in userspase has
its advantage too: bugs are easier to fix, there may be different kind
of resets (hard/soft).

> >> >> 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.
Yes, but this is not different from the interface you propose.

>
> >> >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.
>
The register may be write-only from OS point of view, but its internal
state may still need to be transfered on migration. This brings us back
to the point that state and commands should have different accessors.

> 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.
The interface suppose to be generic, we are not talking about MPIC
specifically here. Regarding migration "never say never" :)

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

>
> >> >> 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.
>
I think they want too. May be they have a plan to enhance irqfd. S390
people do something about it now. Haven't look at proposed patches yet.

> >> 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.
>
I noticed the thread (there was in-kernel irqchip there to compensate for
PPC), but haven't read it before sending the email otherwise I would
have added here that you guys need to agree on common interface.

Now I looked closer into proposed interface. I am not sure why Paul
decided to implement KVM_CREATE_IRQCHIP_ARGS instead of using
KVM_CREATE_IRQCHIP. He supports only one type with his patches and I am
not sure if he is planning add something else. KVM_IRQCHIP_SET_SOURCES
looks like it tires to reimplement irq routing.

> 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.
Since it is not going to switch it is not a good example. On x86 probably
interrupt remapping device will have to have some kernel component that
may take advantage of the new interface, but I haven't thought about
interrupt remapping implementation enough to be sure.

> 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.
I am not sure why special device is needed for such filtering. If MMIO
is not handled by the kernel it is forwarded to a userspace.

>
> >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).
I think read/write will be simpler.

>
> 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?
>
How MSI is delivered with MPIC? From device point of view sending an MSI
is doing write at address X of data Y. How PPC with MPIC translates this
into actual interrupt?

> >> >> 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?
>
It is a hack because purpose of the interface becomes unclear. If you
see it called in a code you have no idea what semantics is expected.
For instance getting/setting of a state should be done when vcpus are
not running, but commands may be sent while they are. This also
encourage bugs when incorrect attribute is used during migration or vice
versa. In short interface does not help you, it requires you to read
documentation of each device very carefully. Reset is one thing that
will be implemented as a command ioctl. Moving address map depends. If
an address is a part of device's sate and will be migrated as such then it
is device attribute, otherwise use command to move it.

--
Gleb.