Re: [PATCH 3/9] KVM: PPC: Book3S: Add kernel emulation for the XICS interrupt controller

Scott Wood
2013-02-21T00:20:51Z

On 02/20/2013 01:58:54 PM, Marcelo Tosatti wrote:
> This is probably a stupid question, but why the
> KVM_SET_IRQCHIP/KVM_SET_GSI_ROUTING interface is not appropriate for
> your purposes?
>
> x86 sets up a default GSI->IRQCHIP PIN mapping on creation (during
> KVM_SET_IRQCHIP), but it can be modified with KVM_SET_GSI_ROUTING.

To start, the whole IRQ routing stuff is poorly documented.

Am I supposed to make up GSI numbers and use the routing thing to
associate them with real interrupts? Are there constraints on what
sort of GSI numbers I can choose (I now see in the code that
KVM_MAX_IRQ_ROUTES is returned from the capability check, but where is
that documented? It looks like the APIC implementation has default
routes, where is that documented?)? Where does the code live to manage
this table, and how APICy is it (looks like the answer is "irq_comm.c,
and very")? I suppose I could write another implementation of the
table management code for MPIC, though the placement of "irqchip"
inside the route entry, rather than as an argument to KVM_IRQ_LINE,
suggests the table is supposed to be global, not in the individual
interrupt controller.

It looks like I'm going to have to do this anyway for irqfd, though
that doesn't make the other uses of the device control api go away.
Even KVM_DEV_MPIC_GRP_IRQ_ACTIVE would still be useful for reading the
status for debugging (reading device registers doesn't quite do it,
since the "active" bit won't show up if the interrupt is masked). At
that point, is it more offensive to make it read-only even though it
would be trivial to make it read/write (which would allow users who
don't need it to bypass the routing API), or to make it read/write and
live with there being more than one way to do something?

KVM_SET_IRQCHIP is not suitable because we have more than 512 bytes of
state, and because it doesn't allow debugging access to device
registers (e.g. inspecting from the QEMU command line), and because
it's hard to add new pieces of state if we realize we left something
out. It reminds be of GET/SET_SREGS. With that, I did what you seem
to want here, which is to adapt the existing interfaces, using feature
flags to control optional state. It ended up being a mess, and ONE_REG
was introduced as a replacement. The device control API is the
equivalent of ONE_REG for things other than vcpus.

-Scott