Re: [PATCH 15/17] KVM: PPC: Support irq routing and irqfd for in-kernel MPIC

[Scott Wood <scottwood@freescale.com>]

On 04/18/2013 07:15:46 PM, Alexander Graf wrote:
> 
> On 18.04.2013, at 23:39, Scott Wood wrote:
> 
> > On 04/18/2013 09:11:55 AM, Alexander Graf wrote:
> >> +static inline int irqchip_in_kernel(struct kvm *kvm)
> >> +{
> >> +	int ret = 0;
> >> +
> >> +#ifdef CONFIG_KVM_MPIC
> >> +	ret = ret || (kvm->arch.mpic != NULL);
> >> +#endif
> >> +	smp_rmb();
> >> +	return ret;
> >> +}
> >
> > Couldn't we just set a non-irqchip-specific bool?  Though  
> eventually this
> > shouldn't be needed at all -- instead the check would be "does the
> > requested irqchip fd exist and refer to something that exposes an  
> irqchip
> > interface"?
> 
> How would we get the irqchip id?

I was thinking it would come from whatever operation you're trying to  
do, though I see that MSI routing doesn't specify an irqchip. :-P

In that case I guess it would check for whether any MSI handler has  
been registered.

> > The rmb needs documentation.  I don't see a corresponding wmb before
> > writing to kvm->arch.mpic.
> 
> I actually just copied it from the x86 code, wondering what the rmb()  
> is supposed to do here. Do we need this at all?

Well, we don't want to start using the irqchip before it's been fully  
initialized -- but if we want to use barriers to accomplish that rather  
than a lock, there needs to be a wmb on the writing side.

> > How would one create a route for IPIs, timers, etc (we have had  
> customers
> > wanting to assign those to KVM guests)?  What about error  
> interrupts on
> > MPIC 4.2?  How are you defining the "pin" field for MPIC?  Shouldn't
> 
> "pin" is basically what a "source line" is on the MPIC. That's what  
> the equivalent of an IOAPIC interrupt line would be for the MPIC  
> world.
> 
> IPIs, timer interrupts and error interrupts are special vectors. We  
> could probably model them as different KVM_IRQ_ROUTING_ types if we  
> ever need to support mapping those to irqfd.

Seems a bit heavyweight to add several new MPIC-specific routing types  
-- maybe just one new KVM_IRQ_ROUTING type that lets multiple words be  
used to describe the interrupt?

> For simple injection we can always do an ioctl on the MPIC device.

I got complaints for that originally. :-)

> However, I'd be inclined to say that it's rather unlikely you'd want  
> to have a vfio device's interrupt line hooked up to the IPI interrupt  
> of your guest...

Well, as I said, we've done it before for a customer (not with VFIO of  
course, but our old internal-tree device assignment mechanism), so not  
*that* unlikely.  The host was a two-core chip running Linux on only  
one of the cores, and a custom OS on the other core.  They wanted to  
communicate between the custom OS and a KVM guest.  Since host Linux  
was only running on one core, it didn't need the IPIs for itself (and  
beginning with e500mc, the host uses msgsnd instead, so there also the  
host will not need the IPIs for itself).

> > there be an API documentation update for this?
> 
> Hrm. I can certainly add one.

OK.  We've had enough confusion from poor documentation in the device  
tree binding, due to Freescale documentation calling openpic irq 16  
"internal interrupt 0", that we should be clear exactly what it means.

> > We also need to document whach irqchip means, if we want to reserve  
> the
> > ability to use it in the future -- otherwise userspace could fill  
> in any
> > old junk there and we'd need to retain compatibility.  It should  
> probably
> > be the fd of the MPIC.
> 
> It can't, because irqchip is an index into an array. We'd have to add  
> another layer of indirection if we want to find the fd to a certain  
> irqchip. That's why I simply restricted it to always be "0" now.

OK, didn't know how firmly that was baked into the code.  Maybe a  
device attribute for returning the irqchip number -- which would  
accommodate devices that expose multiple irqchips.

> > It looks like you only support having one type of irqchip built into
> > the kernel at a time?  That may be OK for now given the existing
> > restrictions for building KVM, but it should be noted as a
> > limitation/TODO.
> 
> I don't think it's really hard to add support for another irqchip  
> type. But we certainly have harder issues to tackle first before we  
> end up in a situation where in-kernel MPIC and in-kernel XICS would  
> make sense in the same kernel.

Not necessarily hard or hugely important, just looked odd putting  
global non-MPIC-specific functions in the MPIC file.  I tend to prefer  
getting the component separation (at least resaonably) correct from the  
start, so the person who would end up needing to refactor to fit their  
device in doesn't need to mess with MPIC code to do so.  Such a person  
might be unfamiliar with MPIC, have no easy way to test, etc.  Similar  
to the current situation with the IRQ routing code, at least before you  
took it over. :-)

-Scott