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

[Scott Wood <scottwood@freescale.com>]

On 02/15/2013 10:51:16 PM, Paul Mackerras wrote:
> On Fri, Feb 15, 2013 at 09:57:06PM -0600, Scott Wood wrote:
> > On 02/15/2013 08:56:14 PM, Paul Mackerras wrote:
> > >I have no particular objection to the device control API per se,  
> but
> > >I have two objections to using it as the primary interface to the  
> XICS
> > >emulation.
> > >
> > >First, I dislike the magical side-effect where creating a device  
> of a
> > >particular type (e.g. MPIC or XICS) automatically attaches it to  
> the
> > >interrupt lines of the vcpus.  I prefer an explicit request to do
> > >in-kernel interrupt control.
> >
> > OK.  This is device-specific behavior, so you could define it
> > differently for XICS than MPIC.  I suppose we could change it for
> > MPIC as well, to leave an opening for the unlikely case where we'd
> > want to model an MPIC that isn't directly connected to the CPUs.
> 
> You can have cascaded MPICs; I once had a powerbook that had one MPIC
> cascaded into another.

OK.

> > How is the explicit request made in this patchset?
> 
> The KVM_CREATE_IRQCHIP_ARGS ioctl says that you want emulation of a
> specific interrupt controller architecture connected to the vcpus'
> external interrupt inputs.  In that sense it's explicit, compared to a
> generic "create device" ioctl that could be for any device.

Hooking up to the CPU's interrupt lines is implicit in creating an MPIC  
(and I'm fine with changing that), not in creating any device.  I don't  
see how it's worse than being implicit in calling  
KVM_CREATE_IRQCHIP_ARGS (which doesn't allow for cascaded irqchips).

> > The standardized interface doesn't make things any easier (as noted
> > above, the caller is already mpic-specific code), and we'd have to
> > come up with a scheme for flattening our interrupt numberspace
> > (rather than introduce new attribute groups for things like IPI and
> > timer interrupts).  It may still be necessary when it comes to
> > irqfd, though...
> 
> With 24 bits of interrupt source number, you don't have to flatten it
> very far.  IPIs are handled separately and don't appear in the
> interrupt source space.

They do need to appear in the interrupt source space if we want to  
inject or irqfd them.  Most users won't want to do that, but we have  
had a customer directly assign IPIs (to communicate with an OS running  
on the other CPU in an AMP setup -- host Linux was non-SMP so wasn't  
using them) and MPIC timers to a guest.

> > >> How do MSIs get injected?
> > >
> > >Just like other interrupts - from the point of view of the  
> interrupt
> > >controller they're edge-triggered interrupt sources.
> >
> > Ah right, I guess this is all set up via hcalls for XICS.
> >
> > With MPIC exposing its registers via the device control api,
> > everything just works -- the PCI device generates a write to the
> > MPIC's memory region, the QEMU MPIC stub sends the write to the
> > kernel as for any other MMIO access (this passthrough is also useful
> > for debugging), the in-kernel MPIC sees the write to the "generate
> > an MSI" register and does its thing.  Compare that to all special
> > the MSI code for APIC... :-)
> 
> You're doing a round trip to userspace for every MPIC register access
> by the guest?  Seriously?

No.  Accesses by the guest get handled in the kernel.  Accesses in  
QEMU, including MSIs generated by virtio, get forwarded to the kernel.

> > >There are plenty of bits free in the 64 bits per source that I have
> > >allowed.  We can accommodate those things.
> >
> > MPIC vector numbers take up 16 of the bits.  The architected
> > interrupt level field is 8 bits, though only a handful of values are
> > actually needed.  Add a couple binary flags, and it gets pretty
> > tight if a third type of interrupt controller starts wanting
> > something new.
> 
> There's enough space for MPIC to have 16 bits of vector and some
> flags.  We don't need to overdesign this.

I view anything other than passing the actual MPIC register values  
around as overdesign here, given that it is communication between  
hw/kvm/mpic.c on the QEMU side and arch/powerpc/kvm/mpic.c on the  
kernel side.

> > interrupts awaiting service (or even in service, if different
> > priorities).  We have both "current task priority" (which is a
> > user-set mask-by-priority register) and the priority of the
> > highest-prio in-service interrupt -- which would "current processor
> > priorty" be?  Etc.
> 
> It would be the current task priority.  I assume MPIC maintains a
> 16-bit map of the interrupt priorities in service, so that would need
> to be added.

We don't maintain such a map in the emulation code.  We have a per-CPU  
bitmap of the actual interrupt sources pending/active, which is another  
attribute that would need to be added in order to support migration on  
MPIC.

-Scott