On Mon, Dec 12, 2011 at 8:28 AM, Avi Kivity <avi at redhat.com> wrote: > 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). > because there is no notion of gsi and irq_source_id on ARM. What's the harm of this additional tracepoint? If I should re-use the existing one, should I simply move it outside of __KVM_HAVE_IOAPIC? >> >> ?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. > right, has been noted and will be fixed. It's a merge error. >> + >> +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. > I simply dropped this idea and changed the explanation to (vcpu_index << 1) and ((vcpu_index << 1) | 1). >> >> +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. > this is taken care of in SMP host patch, but will be moved down the patches for next revision. >> + ? ? ? ? ? ? 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'). > the wakeup - same as above. Good point that we need to signal the other CPU. Will come, maybe not next revision, but the one after that. >> + ? ? } else >> + ? ? ? ? ? ? vcpu->arch.virt_irq &= ~mask; >> + >> + ? ? return 0; >> +} >> + >> Thanks, Christoffer