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

[Scott Wood <scottwood@freescale.com>]

On 02/20/2013 07:09:55 PM, Marcelo Tosatti wrote:
> 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?

I can live with it.

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

Sorry...  The code to manage the table lives in virt/kvm/irq_comm.c.   
That code is very APIC-specific and not currently in a state that  
invites sharing.

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

Suppose, for the sake of irqfd (and maillist tranquility) MPIC uses  
existing KVM_IRQ_LINE and such.  Will there be objection to being able  
to use KVM_GET_DEVICE_ATTR to *get* the irq line status for debugging  
purposes (maybe also useful for migration)?  If there's no objection to  
that, would there be any actual reason, beyond saving a few lines of  
glue code, to make it a read-only attribute?

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

Well, that's what KVM_SET_DEVICE_ATTR is.

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

That's exactly what we did with SREGS, and it got declared a mess and  
replaced with ONE_REG.  I'm trying to learn from my mistakes. :-)

> - Debugging: why is reading entire register state not acceptable? Yes,
>   its slow.

For one, it's more work.  The current way works by simulating a guest  
MMIO access.  No blob format to design, implement, and keep compatible.

-Scott