Re: [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability

[Scott Wood <scottwood@freescale.com>]

On 03/13/2013 08:20:44 PM, Paul Mackerras wrote:
> Setting this capability on a vcpu connects that vcpu to an interrupt
> controller device.  The args[0] field of the argument kvm_enable_cap
> struct specifies the overall architecture of the interrupt
> controller.  The args[1] field specifies the CPU number for the vcpu
> from the interrupt controller's point of view.
> 
> Signed-off-by: Paul Mackerras <paulus@samba.org>
> ---
>  arch/powerpc/include/asm/kvm_host.h |    3 +++
>  arch/powerpc/kvm/powerpc.c          |   29  
> +++++++++++++++++++++++++++++
>  include/uapi/linux/kvm.h            |    1 +
>  3 files changed, 33 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/kvm_host.h  
> b/arch/powerpc/include/asm/kvm_host.h
> index f4ba881..dd167e4 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -373,6 +373,9 @@ struct kvmppc_booke_debug_reg {
>  struct kvm_vcpu_arch {
>  	ulong host_stack;
>  	u32 host_pid;
> +
> +	u32 intr_ctrler;
> +

That abbreviation seems a bit awkward, and we should also have a
private-data pointer.

How about:

	u32 irq_arch;
	void *irq_priv;

>  #endif
> +	case KVM_CAP_IRQ_ARCH:
> +		r = -EBUSY;
> +		mutex_lock(&vcpu->kvm->lock);
> +		/* disallow changing once set */
> +		if (!vcpu->arch.intr_ctrler) {
> +			r = 0;
> +			switch (cap->args[0]) {
> +			case 0:		/* no interrupt controller */
> +				break;

s/0/KVM_IRQ_ARCH_NONE/

...at least so that this patch makes it clear where other type ids  
should
be defined.

> +			default:
> +				r = -EINVAL;
> +			}
> +			if (!r) {
> +				/*
> +				 * Make sure any state set up by the  
> interrupt
> +				 * controller init routine is seen  
> before this.
> +				 */
> +				smp_wmb();
> +				vcpu->arch.intr_ctrler = cap->args[0];
> +			}

Do we really need that wmb()?  We're in vcpu context, right?  If the  
vcpu
migrates to another host cpu, that involves rescheduling which already
has a sync.  If the interrupt controller code we call here modifies data
that will be seen from outside the vcpu, it's the responsibility of that
code to use whatever locks, barriers, etc.  are needed (and it's  
unlikely
that vcpu->arch.intr_ctrler will be the relevant thing that it needs to
order with).

This patch should also add a hook at vcpu destruction to call into the
irq code.

-Scott