On Mon, Dec 12, 2011 at 8:28 AM, Avi Kivity <a...@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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
