Re: [PATCH v8 10/15] ARM: KVM: Inject IRQs and FIQs from userspace

[Avi Kivity <avi@redhat.com>]

On 06/15/2012 10:08 PM, Christoffer Dall wrote:
> From: Christoffer Dall <cdall@cs.columbia.edu>
> 
> 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 << 1
>   FIQ: (vcpu_index << 1) | 1
> 
> This is documented in Documentation/kvm/api.txt.
> 
> The effect of the ioctl is simply to simply raise/lower the
> corresponding irq_line 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 or FIQs causing an idle VCPU to become active again. CPUs
> in guest mode are kicked to make sure they refresh their interrupt status.

>  
> +static int kvm_arch_vm_ioctl_irq_line(struct kvm *kvm,
> +				      struct kvm_irq_level *irq_level)
> +{
> +	int mask;
> +	unsigned int vcpu_idx;
> +	struct kvm_vcpu *vcpu;
> +	unsigned long old, new, *ptr;
> +
> +	vcpu_idx = irq_level->irq >> 1;
> +	if (vcpu_idx >= KVM_MAX_VCPUS)
> +		return -EINVAL;
> +
> +	vcpu = kvm_get_vcpu(kvm, vcpu_idx);
> +	if (!vcpu)
> +		return -EINVAL;
> +
> +	if ((irq_level->irq & 1) == KVM_ARM_IRQ_LINE)
> +		mask = HCR_VI;
> +	else /* KVM_ARM_FIQ_LINE */
> +		mask = HCR_VF;
> +
> +	trace_kvm_set_irq(irq_level->irq, irq_level->level, 0);
> +
> +	ptr = (unsigned long *)&vcpu->arch.irq_lines;
> +	do {
> +		old = ACCESS_ONCE(*ptr);
> +		if (irq_level->level)
> +			new = old | mask;
> +		else
> +			new = old & ~mask;
> +
> +		if (new == old)
> +			return 0; /* no change */
> +	} while (cmpxchg(ptr, old, new) != old);

Isn't this a complicated


   if (level)
       set_bit()
   else
       clear_bit()

?

> +
> +	/*
> +	 * The vcpu irq_lines field was updated, wake up sleeping VCPUs and
> +	 * trigger a world-switch round on the running physical CPU to set the
> +	 * virtual IRQ/FIQ fields in the HCR appropriately.
> +	 */
> +	kvm_vcpu_kick(vcpu);

No need to wake when the line is asserted so you can make this
conditional on level.

> +
> +	return 0;
> +}
> +
>  long kvm_arch_vcpu_ioctl(struct file *filp,
>  			 unsigned int ioctl, unsigned long arg)
>  {
> @@ -298,7 +345,20 @@ 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)
>  {
> -	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:
> +		return -EINVAL;
> +	}
>  }

Should be in common code guarded by the define introduced previously.


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