Re: [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace

[Avi Kivity <avi@redhat.com>]

On 12/11/2011 12:24 PM, Christoffer Dall wrote:
> Userspace can inject IRQs and FIQs through the KVM_IRQ_LINE VM ioctl.
> This ioctl is used since the sematics are in fact two lines that can be
> either raised or lowered on the VCPU - the IRQ and FIQ lines.
>
> KVM needs to know which VCPU it must operate on and whether the FIQ or
> IRQ line is raised/lowered. Hence both pieces of information is packed
> in the kvm_irq_level->irq field. The irq fild value will be:
>   IRQ: vcpu_index * 2
>   FIQ: (vcpu_index * 2) + 1
>
> This is documented in Documentation/kvm/api.txt.
>
> The effect of the ioctl is simply to simply raise/lower the
> corresponding virt_irq field on the VCPU struct, which will cause the
> world-switch code to raise/lower virtual interrupts when running the
> guest on next switch. The wait_for_interrupt flag is also cleared for
> raised IRQs causing an idle VCPU to become active again.
>
> Note: The custom trace_kvm_irq_line is used despite a generic definition of
> trace_kvm_set_irq, since the trace-Kvm_set_irq depends on the x86-specific
> define of __HAVE_IOAPIC. Either the trace event should be created
> regardless of this define or it should depend on another ifdef clause,
> common for both x86 and ARM. However, since the arguments don't really
> match those used in ARM, I am yet to be convinced why this is necessary.

Why don't they match?  The assignment of lines to actual pins differs,
but essentially it's the same thing (otherwise we'd use a different ioctl).

>  
>  Capability: KVM_CAP_IRQCHIP
> -Architectures: x86, ia64
> +Architectures: x86, ia64, arm
>  Type: vm ioctl
>  Parameters: struct kvm_irq_level
>  Returns: 0 on success, -1 on error
> @@ -582,6 +582,14 @@ Requires that an interrupt controller model has been previously created with
>  KVM_CREATE_IRQCHIP.  Note that edge-triggered interrupts require the level
>  to be set to 1 and then back to 0.
>  
> +KVM_CREATE_IRQCHIP (except for ARM).  Note that edge-triggered interrupts
> +require the level to be set to 1 and then back to 0.

Need to replace the previous line beginning with KVM_CREATE_IRQCHIP, not
duplicate it.

> +
> +ARM uses two types of interrupt lines per CPU, ie. IRQ and FIQ. The value of the
> +irq field should be (VCPU_INDEX * 2) for IRQs and ((VCPU_INDEX * 2) + 1) for
> +FIQs. Level is used to raise/lower the line. See arch/arm/include/asm/kvm.h for
> +convenience macros.

Userspace only includes <linux/kvm.h>; also please name the macros here.

>  
> +static int kvm_arch_vm_ioctl_irq_line(struct kvm *kvm,
> +				      struct kvm_irq_level *irq_level)
> +{
> +	u32 mask;
> +	unsigned int vcpu_idx;
> +	struct kvm_vcpu *vcpu;
> +
> +	vcpu_idx = irq_level->irq / 2;
> +	if (vcpu_idx >= KVM_MAX_VCPUS)
> +		return -EINVAL;
> +
> +	vcpu = kvm_get_vcpu(kvm, vcpu_idx);
> +	if (!vcpu)
> +		return -EINVAL;
> +
> +	switch (irq_level->irq % 2) {
> +	case KVM_ARM_IRQ_LINE:
> +		mask = HCR_VI;
> +		break;
> +	case KVM_ARM_FIQ_LINE:
> +		mask = HCR_VF;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	trace_kvm_irq_line(irq_level->irq % 2, irq_level->level, vcpu_idx);
> +
> +	if (irq_level->level) {
> +		vcpu->arch.virt_irq |= mask;

racy - this is a vm ioctl, and so can (and usually is) invoked from
outside the vcpu thread.

> +		vcpu->arch.wait_for_interrupts = 0;

Need an actual wakeup here (see x86's kvm_vcpu_kick() - should really be
common code; it takes care of both the 'vcpu sleeping and needs a
wakeup' and 'vcpu is in guest mode and needs to go to the host to
evaluate interrupt state').

> +	} else
> +		vcpu->arch.virt_irq &= ~mask;
> +
> +	return 0;
> +}
> +
>

-- 
error compiling committee.c: too many arguments to function