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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
