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

[Marcelo Tosatti <mtosatti@redhat.com>]

On Wed, Feb 20, 2013 at 06:20:51PM -0600, Scott Wood wrote:
> 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? 

I have no idea. Is mapping from one integer linear space (GSIs) 
to real interrupts suitable for you? 

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

Don't think it is.

> It looks like the APIC implementation has
> default routes, where is that documented?)? 

In the code.

> Where does the code live to manage this table, and how APICy is it (looks like the
> answer is "irq_comm.c, and very")? 

Thinking faster than typing? Not sure what you mean.

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

Yes the table is global. It maps GSI ("Global System Interrupt" IIRC)
(integer) to (irqchip,pin) pair.

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

Can't follow this sentence.

> 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

- ACK on 512 bytes not sufficient. Add another ioctl, SET_IRQCHIP2?
- Agree on poor extensibility of interface. Adding a reserved amount
of space as padding and versioning such as has been done so far 
is not acceptable? 
- Debugging: why is reading entire register state not acceptable? Yes,
  its slow.