On Tue, Jun 19, 2012 at 4:49 AM, Avi Kivity <avi at redhat.com> wrote:
> On 06/18/2012 11:56 PM, Christoffer Dall wrote:
>> On Mon, Jun 18, 2012 at 9:32 AM, Avi Kivity <avi at redhat.com> wrote:
>>> On 06/15/2012 10:08 PM, Christoffer Dall wrote:
>>>> From: Christoffer Dall <cdall at 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()
>>>
>>> ?
>>>
>>
>> we need to atomically know if we changed either the FIQ/IRQ fields and
>> atomically update without locking.
>
> So use test_and_set_bit()/test_and_clear_bit().
>
>> (I think you suggested this
>> approach, in fact).
>
> I think so too, but it only makes sense if you need to consider both
> fields simultaneously (which you don't here). ?For example, if IRQ was
> asserted while FIQ was already raised, you don't need to kick. ?But
> that's not the case according to the below.
>
ok, fair enough. I simplified it a 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.
>>>
>>
>> we need to trigger a world switch to update the virtualized register
>> from the actual running physical CPU if we changed any of the IRQ/FIQ
>> fields. We return in the loop above if we didn't change anything.
>
> Okay.
>
>
>>
>>>> +
>>>> + ? ? 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.
>>>
>>>
>>
>> you mean like this: ?
>>
>>
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index a9f209b..5bf2193 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -535,8 +535,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu,
>> struct kvm_run *run)
>> ? ? ? return ret;
>> ?}
>>
>> -static int kvm_arch_vm_ioctl_irq_line(struct kvm *kvm,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct kvm_irq_level *irq_level)
>> +int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_level)
>> ?{
>> ? ? ? int mask;
>> ? ? ? unsigned int vcpu_idx;
>> @@ -596,20 +595,7 @@ 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)
>> ?{
>> - ? ? 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;
>> - ? ? }
>> + ? ? return -EINVAL;
>> ?}
>>
>> ?static void cpu_set_vector(void *vector)
>> diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
>> index bd77cb5..122a4b2 100644
>> --- a/arch/ia64/kvm/kvm-ia64.c
>> +++ b/arch/ia64/kvm/kvm-ia64.c
>> @@ -924,6 +924,16 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu
>> *vcpu, struct kvm_regs *regs)
>> ? ? ? return 0;
>> ?}
>>
>> +int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_event)
>> +{
>> + ? ? if (!irqchip_in_kernel(kvm))
>> + ? ? ? ? ? ? return -ENXIO;
>> +
>> + ? ? irq_event->statusstatus = kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? irq_event->irq, irq_event->level);
>> + ? ? return 0;
>> +}
>> +
>> ?long kvm_arch_vm_ioctl(struct file *filp,
>> ? ? ? ? ? ? ? unsigned int ioctl, unsigned long arg)
>> ?{
>> @@ -963,29 +973,6 @@ long kvm_arch_vm_ioctl(struct file *filp,
>> ? ? ? ? ? ? ? ? ? ? ? goto out;
>> ? ? ? ? ? ? ? }
>> ? ? ? ? ? ? ? break;
>> - ? ? case KVM_IRQ_LINE_STATUS:
>> - ? ? case KVM_IRQ_LINE: {
>> - ? ? ? ? ? ? struct kvm_irq_level irq_event;
>> -
>> - ? ? ? ? ? ? r = -EFAULT;
>> - ? ? ? ? ? ? if (copy_from_user(&irq_event, argp, sizeof irq_event))
>> - ? ? ? ? ? ? ? ? ? ? goto out;
>> - ? ? ? ? ? ? r = -ENXIO;
>> - ? ? ? ? ? ? if (irqchip_in_kernel(kvm)) {
>> - ? ? ? ? ? ? ? ? ? ? __s32 status;
>> - ? ? ? ? ? ? ? ? ? ? status = kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? irq_event.irq, irq_event.level);
>> - ? ? ? ? ? ? ? ? ? ? if (ioctl == KVM_IRQ_LINE_STATUS) {
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? r = -EFAULT;
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? irq_event.status = status;
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (copy_to_user(argp, &irq_event,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? sizeof irq_event))
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? goto out;
>> - ? ? ? ? ? ? ? ? ? ? }
>> - ? ? ? ? ? ? ? ? ? ? r = 0;
>> - ? ? ? ? ? ? }
>> - ? ? ? ? ? ? break;
>> - ? ? ? ? ? ? }
>> ? ? ? case KVM_GET_IRQCHIP: {
>> ? ? ? ? ? ? ? /* 0: PIC master, 1: PIC slave, 2: IOAPIC */
>> ? ? ? ? ? ? ? struct kvm_irqchip chip;
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index a01a424..c9c4186 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -3144,6 +3144,16 @@ out:
>> ? ? ? return r;
>> ?}
>>
>> +int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_event)
>> +{
>> + ? ? if (!irqchip_in_kernel(kvm))
>> + ? ? ? ? ? ? return -ENXIO;
>> +
>> + ? ? irq_event->status = kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? irq_event->irq, irq_event->level);
>> + ? ? return 0;
>> +}
>> +
>> ?long kvm_arch_vm_ioctl(struct file *filp,
>> ? ? ? ? ? ? ? ? ? ? ?unsigned int ioctl, unsigned long arg)
>> ?{
>> @@ -3250,29 +3260,6 @@ long kvm_arch_vm_ioctl(struct file *filp,
>> ? ? ? create_pit_unlock:
>> ? ? ? ? ? ? ? mutex_unlock(&kvm->slots_lock);
>> ? ? ? ? ? ? ? break;
>> - ? ? case KVM_IRQ_LINE_STATUS:
>> - ? ? case KVM_IRQ_LINE: {
>> - ? ? ? ? ? ? struct kvm_irq_level irq_event;
>> -
>> - ? ? ? ? ? ? r = -EFAULT;
>> - ? ? ? ? ? ? if (copy_from_user(&irq_event, argp, sizeof irq_event))
>> - ? ? ? ? ? ? ? ? ? ? goto out;
>> - ? ? ? ? ? ? r = -ENXIO;
>> - ? ? ? ? ? ? if (irqchip_in_kernel(kvm)) {
>> - ? ? ? ? ? ? ? ? ? ? __s32 status;
>> - ? ? ? ? ? ? ? ? ? ? status = kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? irq_event.irq, irq_event.level);
>> - ? ? ? ? ? ? ? ? ? ? if (ioctl == KVM_IRQ_LINE_STATUS) {
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? r = -EFAULT;
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? irq_event.status = status;
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (copy_to_user(argp, &irq_event,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? sizeof irq_event))
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? goto out;
>> - ? ? ? ? ? ? ? ? ? ? }
>> - ? ? ? ? ? ? ? ? ? ? r = 0;
>> - ? ? ? ? ? ? }
>> - ? ? ? ? ? ? break;
>> - ? ? }
>> ? ? ? case KVM_GET_IRQCHIP: {
>> ? ? ? ? ? ? ? /* 0: PIC master, 1: PIC slave, 2: IOAPIC */
>> ? ? ? ? ? ? ? struct kvm_irqchip *chip;
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index e3c86f8..96aa7fb 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -494,6 +494,7 @@ int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?kvm_userspace_memory_region *mem,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?int user_alloc);
>> +int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_level);
>> ?long kvm_arch_vm_ioctl(struct file *filp,
>> ? ? ? ? ? ? ? ? ? ? ?unsigned int ioctl, unsigned long arg);
>>
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 636bd08..1d33877 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -2093,6 +2093,25 @@ static long kvm_vm_ioctl(struct file *filp,
>> ? ? ? ? ? ? ? break;
>> ? ? ? }
>> ?#endif
>> +#ifdef __KVM_HAVE_IRQ_LINE
>> + ? ? case KVM_IRQ_LINE_STATUS:
>> + ? ? case KVM_IRQ_LINE: {
>> + ? ? ? ? ? ? struct kvm_irq_level irq_event;
>> +
>> + ? ? ? ? ? ? r = -EFAULT;
>> + ? ? ? ? ? ? if (copy_from_user(&irq_event, argp, sizeof irq_event))
>> + ? ? ? ? ? ? ? ? ? ? goto out;
>> +
>> + ? ? ? ? ? ? r = kvm_vm_ioctl_irq_line(kvm, &irq_event);
>> +
>> + ? ? ? ? ? ? if (ioctl == KVM_IRQ_LINE_STATUS) {
>> + ? ? ? ? ? ? ? ? ? ? if (copy_to_user(argp, &irq_event, sizeof irq_event))
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? r = -EFAULT;
>> + ? ? ? ? ? ? }
>> +
>> + ? ? ? ? ? ? break;
>> + ? ? }
>> +#endif
>> ? ? ? default:
>> ? ? ? ? ? ? ? r = kvm_arch_vm_ioctl(filp, ioctl, arg);
>> ? ? ? ? ? ? ? if (r == -ENOTTY)
>
> Yup. ?Note it brings in KVM_IRQ_LINE_STATUS for ARM. ?I think it makes
> sense if you have guests that do timekeeping by counting interrupts (as
> opposed to reading a wall clock register).
>
> --
> error compiling committee.c: too many arguments to function
>
>
