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

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 7860 bytes --]

Just found two, maybe three nits while browsing by:

On 2011-12-11 11:24, 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.
> 
> Signed-off-by: Christoffer Dall <c.dall@virtualopensystems.com>
> ---
>  Documentation/virtual/kvm/api.txt |   10 ++++++-
>  arch/arm/include/asm/kvm.h        |    8 ++++++
>  arch/arm/include/asm/kvm_arm.h    |    1 +
>  arch/arm/kvm/arm.c                |   53 ++++++++++++++++++++++++++++++++++++-
>  arch/arm/kvm/trace.h              |   21 +++++++++++++++
>  include/linux/kvm.h               |    1 +
>  6 files changed, 91 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 7945b0b..4abaa67 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -572,7 +572,7 @@ only go to the IOAPIC.  On ia64, a IOSAPIC is created.
>  4.25 KVM_IRQ_LINE
>  
>  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.

You probably wanted to replace the original lines with these two, no?

> +
> +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.
> +
>  struct kvm_irq_level {
>  	union {
>  		__u32 irq;     /* GSI */
> diff --git a/arch/arm/include/asm/kvm.h b/arch/arm/include/asm/kvm.h
> index 87dc33b..8935062 100644
> --- a/arch/arm/include/asm/kvm.h
> +++ b/arch/arm/include/asm/kvm.h
> @@ -20,6 +20,14 @@
>  #include <asm/types.h>
>  
>  /*
> + * KVM_IRQ_LINE macros to set/read IRQ/FIQ for specific VCPU index.
> + */
> +enum KVM_ARM_IRQ_LINE_TYPE {
> +	KVM_ARM_IRQ_LINE = 0,
> +	KVM_ARM_FIQ_LINE = 1,
> +};
> +
> +/*
>   * Modes used for short-hand mode determinition in the world-switch code and
>   * in emulation code.
>   *
> diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h
> index 835abd1..e378a37 100644
> --- a/arch/arm/include/asm/kvm_arm.h
> +++ b/arch/arm/include/asm/kvm_arm.h
> @@ -49,6 +49,7 @@
>  #define HCR_VM		1
>  #define HCR_GUEST_MASK (HCR_TSC | HCR_TWE | HCR_TWI | HCR_VM | HCR_AMO | \
>  			HCR_AMO | HCR_IMO | HCR_FMO | HCR_SWIO)
> +#define HCR_VIRT_EXCP_MASK (HCR_VA | HCR_VI | HCR_VF)
>  
>  /* Hyp System Control Register (HSCTLR) bits */
>  #define HSCTLR_TE	(1 << 30)
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 89ba18d..fc0bd6b 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -299,6 +299,43 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	return -EINVAL;
>  }
>  
> +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;

Due to % 2, default is unreachable. Remove the masking?

> +	}
> +
> +	trace_kvm_irq_line(irq_level->irq % 2, irq_level->level, vcpu_idx);
> +
> +	if (irq_level->level) {
> +		vcpu->arch.virt_irq |= mask;
> +		vcpu->arch.wait_for_interrupts = 0;
> +	} else
> +		vcpu->arch.virt_irq &= ~mask;

No need to protect the bitops on virt_irq? Or what lock does this?

> +
> +	return 0;
> +}
> +
>  long kvm_arch_vcpu_ioctl(struct file *filp,
>  			 unsigned int ioctl, unsigned long arg)
>  {
> @@ -313,8 +350,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)
>  {
> -	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:
> +		return -EINVAL;
> +	}
>  }
>  
>  /**
> diff --git a/arch/arm/kvm/trace.h b/arch/arm/kvm/trace.h
> index f8869c1..ac64e3a 100644
> --- a/arch/arm/kvm/trace.h
> +++ b/arch/arm/kvm/trace.h
> @@ -40,6 +40,27 @@ TRACE_EVENT(kvm_exit,
>  );
>  
>  
> +TRACE_EVENT(kvm_irq_line,
> +	TP_PROTO(unsigned int type, unsigned int level, unsigned int vcpu_idx),
> +	TP_ARGS(type, level, vcpu_idx),
> +
> +	TP_STRUCT__entry(
> +		__field(	unsigned int,	type			)
> +		__field(	unsigned int,	level			)
> +		__field(	unsigned int,	vcpu_idx		)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->type			= type;
> +		__entry->level			= level;
> +		__entry->vcpu_idx		= vcpu_idx;
> +	),
> +
> +	TP_printk("KVM_IRQ_LINE: type: %s, level: %u, vcpu: %u",
> +		(__entry->type == KVM_ARM_IRQ_LINE) ? "IRQ" : "FIQ",
> +		__entry->level, __entry->vcpu_idx)
> +);
> +
>  
>  #endif /* _TRACE_KVM_H */
>  
> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index c3892fc..679abbb 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -111,6 +111,7 @@ struct kvm_irq_level {
>  	 * ACPI gsi notion of irq.
>  	 * For IA-64 (APIC model) IOAPIC0: irq 0-23; IOAPIC1: irq 24-47..
>  	 * For X86 (standard AT mode) PIC0/1: irq 0-15. IOAPIC0: 0-23..
> +	 * For ARM: IRQ: irq = (2*vcpu_index). FIQ: irq = (2*vcpu_indx + 1).
>  	 */
>  	union {
>  		__u32 irq;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]