Re: [PATCH v4 05/10] ARM: KVM: Inject IRQs and FIQs from userspace

[Avi Kivity <avi@redhat.com>]

On 08/06/2011 01:39 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 x86 starts out with a default configuration and allows updating it 
via KVM_SET_GSI_ROUTING.  You may need this in the future if you decide 
to implement an irq controller in the kernel.

> +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);

Please reuse trace_kvm_set_irq().  You can decode vcpu/type in a 
trace-cmd plugin.

> +
> +	if (irq_level->level) {
> +		vcpu->arch.virt_irq |= mask;
> +		vcpu->arch.wait_for_interrupts = 0;
> +	} else
> +		vcpu->arch.virt_irq&= ~mask;
> +

This seems to be non-smp-safe?  Do you need atomic ops and barriers 
here?  And a wakeup?

Unlike KVM_INTERRUPT, KVM_IRQ_LINE is designed to be used asynchronously 
wrt the vcpu.

> +	return 0;
> +}
> +
>   long kvm_arch_vcpu_ioctl(struct file *filp,
>   			 unsigned int ioctl, unsigned long arg)
>   {
> @@ -312,8 +349,21 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
>   long kvm_arch_vm_ioctl(struct file *filp,
>   		       unsigned int ioctl, unsigned long arg)
>   {
> -	printk(KERN_ERR "kvm_arch_vm_ioctl: Unsupported ioctl (%d)\n", ioctl);
> -	return -EINVAL;
> +	struct kvm *kvm = filp->private_data;
> +	void __user *argp = (void __user *)arg;
> +
> +	switch (ioctl) {
> +	case KVM_IRQ_LINE: {
> +		struct kvm_irq_level irq_event;
> +
> +		if (copy_from_user(&irq_event, argp, sizeof irq_event))
> +			return -EFAULT;
> +		return kvm_arch_vm_ioctl_irq_line(kvm,&irq_event);
> +	}
> +	default:
> +		kvm_err(-EINVAL, "Unsupported ioctl (%d)", ioctl);

Please remove for the final code, we don't want a user spamming the 
kernel log.

> +		return -EINVAL;
> +	}
>   }
>
>

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