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

[Scott Wood <scottwood@freescale.com>]

On 04/18/2013 09:11:55 AM, Alexander Graf wrote:
> Now that all the irq routing and irqfd pieces are generic, we can  
> expose
> real irqchip support to all of KVM's internal helpers.
> 
> This allows us to use irqfd with the in-kernel MPIC.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/powerpc/include/asm/kvm_host.h |    7 ++
>  arch/powerpc/include/uapi/asm/kvm.h |    1 +
>  arch/powerpc/kvm/Kconfig            |    3 +
>  arch/powerpc/kvm/Makefile           |    1 +
>  arch/powerpc/kvm/irq.h              |   17 ++++++
>  arch/powerpc/kvm/mpic.c             |  106  
> +++++++++++++++++++++++++++++++++++
>  6 files changed, 135 insertions(+), 0 deletions(-)
>  create mode 100644 arch/powerpc/kvm/irq.h
> 
> diff --git a/arch/powerpc/include/asm/kvm_host.h  
> b/arch/powerpc/include/asm/kvm_host.h
> index 36368c9..d5fbb4b 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -44,6 +44,10 @@
>  #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
>  #endif
> 
> +/* These values are internal and can be increased later */
> +#define KVM_NR_IRQCHIPS          1
> +#define KVM_IRQCHIP_NUM_PINS     256
> +
>  #if !defined(CONFIG_KVM_440)
>  #include <linux/mmu_notifier.h>
> 
> @@ -256,6 +260,9 @@ struct kvm_arch {
>  #ifdef CONFIG_PPC_BOOK3S_64
>  	struct list_head spapr_tce_tables;
>  #endif
> +#ifdef CONFIG_KVM_MPIC
> +	void *mpic;
> +#endif
>  };

This can be "struct openpic *mpic" -- we already do this in the vcpu.

> diff --git a/arch/powerpc/kvm/irq.h b/arch/powerpc/kvm/irq.h
> new file mode 100644
> index 0000000..f1e27fd
> --- /dev/null
> +++ b/arch/powerpc/kvm/irq.h
> @@ -0,0 +1,17 @@
> +#ifndef __IRQ_H
> +#define __IRQ_H
> +
> +#include <linux/kvm_host.h>
> +
> +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"?

The rmb needs documentation.  I don't see a corresponding wmb before
writing to kvm->arch.mpic.

> diff --git a/arch/powerpc/kvm/mpic.c b/arch/powerpc/kvm/mpic.c
> index b1ae02d..c8de2f2 100644
> --- a/arch/powerpc/kvm/mpic.c
> +++ b/arch/powerpc/kvm/mpic.c
> @@ -1087,6 +1087,8 @@ static int openpic_cpu_write_internal(void  
> *opaque, gpa_t addr,
>  		}
> 
>  		IRQ_resetbit(&dst->servicing, s_IRQ);
> +		/* Notify listeners that the IRQ is over */
> +		kvm_notify_acked_irq(opp->kvm, 0, s_IRQ);

I don't think we want to call that with the lock held -- it looks like  
it
can call back into kvm_set_irq.  In our old internal version I waited
until the end of the EOI code, and called the notify function with the
lock dropped temporarily.

>  		/* Set up next servicing IRQ */
>  		s_IRQ = IRQ_get_next(opp, &dst->servicing);
>  		/* Check queued interrupts. */
> @@ -1639,14 +1641,42 @@ static void mpic_destroy(struct kvm_device  
> *dev)
>  		unmap_mmio(opp);
>  	}
> 
> +	dev->kvm->arch.mpic = NULL;
>  	kfree(opp);
>  }
> 
> +static int mpic_set_default_irq_routing(struct openpic *opp)
> +{
> +	int i;
> +	struct kvm_irq_routing_entry *routing;
> +
> +	/* XXX be more dynamic if we ever want to support multiple MPIC  
> chips */
> +	routing = kzalloc((sizeof(*routing) * opp->nb_irqs),  
> GFP_KERNEL);
> +	if (!routing)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < opp->nb_irqs; i++) {
> +		routing[i].gsi = i;
> +		routing[i].type = KVM_IRQ_ROUTING_IRQCHIP;
> +		routing[i].u.irqchip.irqchip = 0;
> +		routing[i].u.irqchip.pin = i;
> +	}
> +
> +	kvm_set_irq_routing(opp->kvm, routing, opp->nb_irqs, 0);
> +
> +	kfree(routing);
> +	return 0;
> +}

Do we really want any default routes?  There's no platform notion of GSI
here, so how is userspace to know how the kernel set it up (or what GSIs
are free to be used for new routes) -- other than the "read the code"
answer I got when I asked about x86?  :-P

> +int kvm_set_routing_entry(struct kvm_irq_routing_table *rt,
> +			  struct kvm_kernel_irq_routing_entry *e,
> +			  const struct kvm_irq_routing_entry *ue)
> +{
> +	int r = -EINVAL;
> +
> +	switch (ue->type) {
> +	case KVM_IRQ_ROUTING_IRQCHIP:
> +		e->set = mpic_set_irq;
> +		e->irqchip.irqchip = ue->u.irqchip.irqchip;
> +		e->irqchip.pin = ue->u.irqchip.pin;
> +		if (e->irqchip.pin >= KVM_IRQCHIP_NUM_PINS)
> +			goto out;
> +		rt->chip[ue->u.irqchip.irqchip][e->irqchip.pin] =  
> ue->gsi;
> +		break;
> +	case KVM_IRQ_ROUTING_MSI:
> +		e->set = kvm_set_msi;
> +		e->msi.address_lo = ue->u.msi.address_lo;
> +		e->msi.address_hi = ue->u.msi.address_hi;
> +		e->msi.data = ue->u.msi.data;
> +		break;
> +	default:
> +		goto out;
> +	}
> +
> +	r = 0;
> +out:
> +	return r;
> +}

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
there be an API documentation update for this?

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

BTW, thanks for taking this on -- it would have taken me a while to
digest the existing code.

-Scott