On Sun, Dec 11, 2011 at 10:18 AM, Jan Kiszka <jan.kiszka at web.de> wrote:
> 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 at 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?
>
ah yes, some stgit re-ordering artifact.
>> +
>> +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?
>
nah, the masking should be there, it's just th default case that should go away.
>> + ? ? }
>> +
>> + ? ? 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?
>
that's fixed in the SMP host patch (but I guess with I/O thread that
doesn't really apply only to SMP, so I could move that logging down
the patch stream. In any case, yes, there should be locking, and there
is).
>> +
>> + ? ? 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;
>>
Thanks,
-Christoffer
