On 06/05/2014 06:08 PM, Christoffer Dall wrote: > On Thu, Jun 05, 2014 at 05:58:02PM +0200, Eric Auger wrote: >> On 06/05/2014 04:39 PM, Christoffer Dall wrote: >>> On Thu, Jun 05, 2014 at 03:15:15PM +0200, Eric Auger wrote: >>>> On 06/05/2014 12:28 PM, Christoffer Dall wrote: >>>>> On Mon, Jun 02, 2014 at 09:29:56AM +0200, Eric Auger wrote: >>>>>> This patch enables irqfd and irq routing on ARM. >>>>>> >>>>>> It turns on CONFIG_HAVE_KVM_EVENTFD and CONFIG_HAVE_KVM_IRQ_ROUTING >>>>>> >>>>>> irqfd framework enables to assign physical IRQs to guests. >>>>>> >>>>>> 1) user-side uses KVM_IRQFD VM ioctl to pass KVM a kvm_irqfd struct that >>>>>> associates a VM, an eventfd, an IRQ number (aka. the GSI). When an actor >>>>>> signals the eventfd (typically a VFIO platform driver), the irqfd subsystem >>>>>> injects the specified IRQ into the VM (the "GSI" takes the semantic of a >>>>>> virtual IRQ for that guest). >>>>>> >>>>>> 2) the other use case is user-side does 1) and uses KVM_SET_GSI_ROUTING >>>>>> VM ioctl to create an association between a VM, a physical IRQ (aka GSI) and >>>>>> a virtual IRQ (aka irchip.pin). This creates a so-called GSI routing entry. >>>>>> When someone triggers the eventfd, irqfd handles it but uses the specified >>>>>> routing and eventually injects irqchip.pin virtual IRQ into the guest. In that >>>>>> context the GSI takes the semantic of a physical IRQ while the irqchip.pin >>>>>> takes the semantic of a virtual IRQ. >>>>>> >>>>>> in 1) routing is used by irqfd but an identity routing is created by default >>>>>> making the gsi = irqchip.pin. Note on ARM there is a single interrupt >>>>>> controller kind, the GIC. >>>>>> >>>>>> GSI routing mostly is implemented in generic irqchip.c. >>>>>> The tiny ARM specific part is directly implemented in the virtual interrupt >>>>>> controller (vgic.c) as it is done for powerpc for instance. This option was >>>>>> prefered compared to implementing other #ifdef in irq_comm.c (x86 and ia64). >>>>>> Hence irq_comm.c is not used at all. >>>>>> >>>>>> Routing currently is not used for anything else than irqfd IRQ injection. Only >>>>>> SPI can be injected. This means the vgic is not totally hidden behind the >>>>>> irqchip. There are separate discussions on PPI/SGI routing. >>>>> >>>>> What do you mean here? Is there an ongoing discussion on the mailing >>>>> list somewhere? >>>> >>>> Hi Christoffer, >>>> >>>> Thanks for the review. >>>> >>>> I was refering to that thread where it was invoked to put the whole vgic >>>> behind irqchip: >>>> http://www.spinics.net/lists/kvm-arm/msg08413.html >>>>> >>>>>> >>>>>> Only level sensitive IRQs are supported (with a registered resampler). As a >>>>> >>>>> Is it not trivial to add edge-triggered support in the same go? >>>> Yes it shouldn't be a problem. It is more a matter of testing. >>>>> >>>>>> reminder the resampler is a second eventfd called by irqfd framework when the >>>>>> virtual IRQ is completed by the guest. This eventfd is supposed to be handled >>>>>> on user-side >>>>>> >>>>>> MSI routing is not supported yet. >>>>>> >>>>>> This work was tested with Calxeda Midway xgmac main interrupt (with and without >>>>>> explicit user routing) with qemu-system-arm and QEMU VFIO platform device. >>>>>> >>>>>> changes v1 -> v2: >>>>>> 2 fixes: >>>>>> - v1 assumed gsi/irqchip.pin was already incremented by VGIC_NR_PRIVATE_IRQS. >>>>>> This is now vgic_set_assigned_irq that increments it before injection. >>>>>> - v2 now handles the case where a pending assigned irq is cleared through >>>>>> MMIO access. The irq is properly acked allowing the resamplefd handler >>>>>> to possibly unmask the physical IRQ. >>>>>> >>>>>> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx> >>>>>> >>>>>> Conflicts: >>>>>> Documentation/virtual/kvm/api.txt >>>>>> arch/arm/kvm/Kconfig >>>>>> >>>>>> Conflicts: >>>>>> Documentation/virtual/kvm/api.txt >>>>> >>>>> We usually don't include these conflict notices when sending out >>>>> patches. >>>> >>>> OK I will remove them >>>>> >>>>>> --- >>>>>> Documentation/virtual/kvm/api.txt | 4 +- >>>>>> arch/arm/include/uapi/asm/kvm.h | 8 +++ >>>>>> arch/arm/kvm/Kconfig | 2 + >>>>>> arch/arm/kvm/Makefile | 1 + >>>>>> arch/arm/kvm/irq.h | 25 +++++++ >>>>>> virt/kvm/arm/vgic.c | 141 ++++++++++++++++++++++++++++++++++++-- >>>>>> 6 files changed, 174 insertions(+), 7 deletions(-) >>>>>> create mode 100644 arch/arm/kvm/irq.h >>>>>> >>>>>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt >>>>>> index b4f5365..b376334 100644 >>>>>> --- a/Documentation/virtual/kvm/api.txt >>>>>> +++ b/Documentation/virtual/kvm/api.txt >>>>>> @@ -1339,7 +1339,7 @@ KVM_ASSIGN_DEV_IRQ. Partial deassignment of host or guest IRQ is allowed. >>>>>> 4.52 KVM_SET_GSI_ROUTING >>>>>> >>>>>> Capability: KVM_CAP_IRQ_ROUTING >>>>>> -Architectures: x86 ia64 s390 >>>>>> +Architectures: x86 ia64 s390 arm >>>>>> Type: vm ioctl >>>>>> Parameters: struct kvm_irq_routing (in) >>>>>> Returns: 0 on success, -1 on error >>>>>> @@ -2126,7 +2126,7 @@ into the hash PTE second double word). >>>>>> 4.75 KVM_IRQFD >>>>>> >>>>>> Capability: KVM_CAP_IRQFD >>>>>> -Architectures: x86 s390 >>>>>> +Architectures: x86 s390 arm >>>>>> Type: vm ioctl >>>>>> Parameters: struct kvm_irqfd (in) >>>>>> Returns: 0 on success, -1 on error >>>>>> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h >>>>>> index ef0c878..89b864d 100644 >>>>>> --- a/arch/arm/include/uapi/asm/kvm.h >>>>>> +++ b/arch/arm/include/uapi/asm/kvm.h >>>>>> @@ -192,6 +192,14 @@ struct kvm_arch_memory_slot { >>>>>> /* Highest supported SPI, from VGIC_NR_IRQS */ >>>>>> #define KVM_ARM_IRQ_GIC_MAX 127 >>>>>> >>>>>> +/* needed by IRQ routing */ >>>>>> + >>>>>> +/* One single KVM irqchip, ie. the VGIC */ >>>>>> +#define KVM_NR_IRQCHIPS 1 >>>>>> + >>>>>> +/* virtual interrupt controller input pins (max 480 SPI, 32 SGI/PPI) */ >>>>>> +#define KVM_IRQCHIP_NUM_PINS 256 >>>>> >>>>> I don't even see how the comment correlates to the define. Hmmm. But >>>>> since Marc asked you to change this anyhow, maybe this doesn't matter >>>>> now. >>>> >>>> yes you're right. Those were the figures I was able to find in GIC400 >>>> TRM and I was confused by the fact I was not able to find them in the >>>> code so I eventually put the same value as VGIC_NR_IRQS. I started >>>> looking at kvm-arm64/vgic-dyn and found this dist nr_irqs but need more >>>> time to investigate. Nethertheless his KVM_IRQCHIP_NUM_PINS is used in >>>> generic code (irqchip). >>>>> >>>>>> + >>>>>> /* PSCI interface */ >>>>>> #define KVM_PSCI_FN_BASE 0x95c1ba5e >>>>>> #define KVM_PSCI_FN(n) (KVM_PSCI_FN_BASE + (n)) >>>>>> diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig >>>>>> index 4be5bb1..096692c 100644 >>>>>> --- a/arch/arm/kvm/Kconfig >>>>>> +++ b/arch/arm/kvm/Kconfig >>>>>> @@ -24,6 +24,7 @@ config KVM >>>>>> select KVM_MMIO >>>>>> select KVM_ARM_HOST >>>>>> depends on ARM_VIRT_EXT && ARM_LPAE && !CPU_BIG_ENDIAN >>>>>> + select HAVE_KVM_EVENTFD >>>>>> ---help--- >>>>>> Support hosting virtualized guest machines. You will also >>>>>> need to select one or more of the processor modules below. >>>>>> @@ -56,6 +57,7 @@ config KVM_ARM_VGIC >>>>>> bool "KVM support for Virtual GIC" >>>>>> depends on KVM_ARM_HOST && OF >>>>>> select HAVE_KVM_IRQCHIP >>>>>> + select HAVE_KVM_IRQ_ROUTING >>>>>> default y >>>>>> ---help--- >>>>>> Adds support for a hardware assisted, in-kernel GIC emulation. >>>>>> diff --git a/arch/arm/kvm/Makefile b/arch/arm/kvm/Makefile >>>>>> index 789bca9..29de111 100644 >>>>>> --- a/arch/arm/kvm/Makefile >>>>>> +++ b/arch/arm/kvm/Makefile >>>>>> @@ -21,4 +21,5 @@ obj-y += kvm-arm.o init.o interrupts.o >>>>>> obj-y += arm.o handle_exit.o guest.o mmu.o emulate.o reset.o >>>>>> obj-y += coproc.o coproc_a15.o coproc_a7.o mmio.o psci.o perf.o >>>>>> obj-$(CONFIG_KVM_ARM_VGIC) += $(KVM)/arm/vgic.o >>>>>> +obj-$(CONFIG_HAVE_KVM_EVENTFD) += $(KVM)/eventfd.o $(KVM)/irqchip.o >>>>>> obj-$(CONFIG_KVM_ARM_TIMER) += $(KVM)/arm/arch_timer.o >>>>>> diff --git a/arch/arm/kvm/irq.h b/arch/arm/kvm/irq.h >>>>>> new file mode 100644 >>>>>> index 0000000..4d6fcc6 >>>>>> --- /dev/null >>>>>> +++ b/arch/arm/kvm/irq.h >>>>>> @@ -0,0 +1,25 @@ >>>>>> +/* >>>>>> + * Copyright (C) 2014, STMicroelectronics >>>>>> + * Authors: Eric Auger <eric.auger@xxxxxx> >>>>> >>>>> please use the Linaro copyright notice for this. You can add your >>>>> @st.com e-mail in addition to the Linaro one in case you want that for >>>>> long-term support. >>>> >>>> OK I will do >>>>> >>>>>> + * >>>>>> + * This program is free software; you can redistribute it and/or modify >>>>>> + * it under the terms of the GNU General Public License, version 2, as >>>>>> + * published by the Free Software Foundation. >>>>>> + * >>>>>> + * This program is distributed in the hope that it will be useful, >>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>>>> + * GNU General Public License for more details. >>>>>> + * >>>>>> + */ >>>>>> + >>>>>> +#ifndef __IRQ_H >>>>>> +#define __IRQ_H >>>>>> + >>>>>> +#include <linux/kvm_host.h> >>>>>> +/* >>>>>> + * Placeholder for irqchip and irq/msi routing declarations >>>>>> + * included in irqchip.c >>>>>> + */ >>>>> >>>>> But none needed now? >>>> >>>> Yes nothing for the time being. >>>>> >>>>>> + >>>>>> +#endif >>>>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c >>>>>> index 56ff9be..39afa0d 100644 >>>>>> --- a/virt/kvm/arm/vgic.c >>>>>> +++ b/virt/kvm/arm/vgic.c >>>>>> @@ -93,6 +93,9 @@ static struct device_node *vgic_node; >>>>>> #define ACCESS_WRITE_VALUE (3 << 1) >>>>>> #define ACCESS_WRITE_MASK(x) ((x) & (3 << 1)) >>>>>> >>>>>> +static struct kvm_irq_routing_entry identity_table[VGIC_NR_IRQS]; >>>>>> +static int set_default_routing_table(struct kvm *kvm); >>>>>> + >>>>>> static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu); >>>>>> static void vgic_update_state(struct kvm *kvm); >>>>>> static void vgic_kick_vcpus(struct kvm *kvm); >>>>>> @@ -408,11 +411,27 @@ static bool handle_mmio_clear_pending_reg(struct kvm_vcpu *vcpu, >>>>>> struct kvm_exit_mmio *mmio, >>>>>> phys_addr_t offset) >>>>>> { >>>>>> - u32 *reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_state, >>>>>> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic; >>>>>> + unsigned int i; >>>>>> + bool is_assigned_irq; >>>>>> + DECLARE_BITMAP(old, VGIC_NR_SHARED_IRQS); >>>>>> + DECLARE_BITMAP(diff, VGIC_NR_SHARED_IRQS); >>>>>> + unsigned long *pending = >>>>>> + vgic_bitmap_get_shared_map(&dist->irq_state); >>>>>> + u32 *reg; >>>>> >>>>> please add a blank line between your declarations and the actual code. >>>> >>>> OK >>>>> >>>>>> + bitmap_copy(old, pending, VGIC_NR_SHARED_IRQS); >>>>>> + reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_state, >>>>>> vcpu->vcpu_id, offset); >>>>>> vgic_reg_access(mmio, reg, offset, >>>>>> ACCESS_READ_VALUE | ACCESS_WRITE_CLEARBIT); >>>>>> if (mmio->is_write) { >>>>>> + pending = vgic_bitmap_get_shared_map(&dist->irq_state); >>>>>> + bitmap_xor(diff, old, pending, VGIC_NR_SHARED_IRQS); >>>>>> + for_each_set_bit(i, diff, VGIC_NR_SHARED_IRQS) { >>>>>> + is_assigned_irq = kvm_irq_has_notifier(vcpu->kvm, 0, i); >>>>>> + if (is_assigned_irq) >>>>>> + kvm_notify_acked_irq(vcpu->kvm, 0, i); >>>>>> + } >>>>> >>>>> As Mark pointed out, just copy the vgic reg value and do a simple xor on >>>>> the two instead, and factor out the two lines that check for >>>>> is_assigned_irq and calles notify_acked so that you can give a small >>>>> static function a semantically meaningful name and call that from both >>>>> this function and the process_maintenance function. >>>> >>>> Yes I corrected this after looking into more details at the register >>>> semantic. >>>>> >>>>> That being said, this whole thing feels a bit weird, I'll comment on the >>>>> other thread. >>>> OK >>>>> >>>>>> vgic_update_state(vcpu->kvm); >>>>>> return true; >>>>>> } >>>>>> @@ -1172,6 +1191,8 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu) >>>>>> { >>>>>> struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; >>>>>> bool level_pending = false; >>>>>> + struct kvm *kvm; >>>>>> + int is_assigned_irq; >>>>>> >>>>>> kvm_debug("MISR = %08x\n", vgic_cpu->vgic_misr); >>>>>> >>>>>> @@ -1189,12 +1210,23 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu) >>>>>> vgic_irq_clear_active(vcpu, irq); >>>>>> vgic_cpu->vgic_lr[lr] &= ~GICH_LR_EOI; >>>>>> >>>>>> + kvm = vcpu->kvm; >>>>>> + is_assigned_irq = >>>>>> + kvm_irq_has_notifier(kvm, 0, irq-VGIC_NR_PRIVATE_IRQS); >>>>> >>>>> I think the preferred style is to keep the function call name on the >>>>> same line as the assignment, and the do a line break on the parameter >>>>> that doesn't fit in the line width and align that to the opening >>>>> parenthesis on the funciton call. At least I prefer it that way. >>>> >>>> OK I will change this. >>>>> >>>>> also spaces around the '-', please. >>>>> >>>>>> /* Any additional pending interrupt? */ >>>>> >>>>> This comment seems to only aplly for non-assigned IRQs now, right? >>>> >>>> yes it does >>>>> >>> >>> so move the comment in the else-clause, if it's an assigned irq then >>> that's not what you're handling here. >> >> correct >> >> But come to think of it, don't we >>> need to check if the line is still high? >> >> by construction it should not be possible since the physical IRQ is >> masked by the VFIO driver. > > But this mechanism is not necessarily tied to VFIO is it? > Yes that's correct. but it is tied to eventfd mechanism. I need to further check what it induces as constraints. vhost uses it. Would be good to put this in place yo test genericity of next versions. >>> >>>>>> - if (vgic_dist_irq_is_pending(vcpu, irq)) { >>>>>> - vgic_cpu_irq_set(vcpu, irq); >>>>>> - level_pending = true; >>>>>> - } else { >>>>>> + if (is_assigned_irq) { >>>>>> vgic_cpu_irq_clear(vcpu, irq); >>>>>> + kvm_debug("EOI irqchip routed vIRQ %d\n", irq); >>>>>> + kvm_notify_acked_irq(kvm, 0, >>>>>> + irq-VGIC_NR_PRIVATE_IRQS); >>>>> >>>>> spaces around the '-', please. >>>> OK >>>>> >>>>>> + vgic_dist_irq_clear(vcpu, irq); >>>>>> + } else { >>>>>> + if (vgic_dist_irq_is_pending(vcpu, irq)) { >>>>>> + vgic_cpu_irq_set(vcpu, irq); >>>>>> + level_pending = true; >>>>>> + } else { >>>>>> + vgic_cpu_irq_clear(vcpu, irq); >>>>>> + } >>>>>> } >>>>>> >>>>>> /* >>>>>> @@ -1627,6 +1659,8 @@ int kvm_vgic_create(struct kvm *kvm) >>>>>> kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF; >>>>>> kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF; >>>>>> >>>>>> + set_default_routing_table(kvm); >>>>>> + >>>>>> out_unlock: >>>>>> for (; vcpu_lock_idx >= 0; vcpu_lock_idx--) { >>>>>> vcpu = kvm_get_vcpu(kvm, vcpu_lock_idx); >>>>>> @@ -2017,3 +2051,100 @@ struct kvm_device_ops kvm_arm_vgic_v2_ops = { >>>>>> .get_attr = vgic_get_attr, >>>>>> .has_attr = vgic_has_attr, >>>>>> }; >>>>>> + >>>>>> + >>>>>> +/* >>>>>> + * set up a default identity routing table >>>>>> + * The user-side can further change the routing table using >>>>>> + * KVM_SET_GSI_ROUTING VM ioctl >>>>>> + */ >>>>>> + >>>>> >>>>> don't add this whitespace between comments and a function declaration, >>>>> please check the entire patch for this. >>>> OK >>>>> >>>>>> +static int set_default_routing_table(struct kvm *kvm) >>>>>> +{ >>>>>> + struct kvm_irq_routing_entry; >>>>>> + int i; >>>>>> + for (i = 0; i < VGIC_NR_IRQS; i++) { >>>>>> + identity_table[i].gsi = i; >>>>>> + identity_table[i].type = KVM_IRQ_ROUTING_IRQCHIP; >>>>>> + identity_table[i].u.irqchip.irqchip = 0; >>>>>> + identity_table[i].u.irqchip.pin = i; >>>>>> + } >>>>>> + return kvm_set_irq_routing(kvm, identity_table, >>>>>> + ARRAY_SIZE(identity_table), 0); >>>>> >>>>> is identity table used after this stage? If not, could you not >>>>> dynamically allocate it and free it after use so we don't waste this >>>>> staic allocation of memory in the kernel? >>>> >>>> yes this definitively can be optimized. >>>>> >>>>>> +} >>>>>> + >>>>>> + >>>>>> +/* >>>>>> + * Functions needed for GSI routing (used by irqchip.c) >>>>>> + * implemented in irq_comm.c for x86 and ia64 >>>>>> + * in architecture specific files for some other archictures (powerpc) >>>>>> + */ >>>>> >>>>> Hmmm, this comment seems rather pointless in this file. If you want to >>>>> describe what this function does, then just document this functionality >>>>> and the parameters/return value, i.e. >>>>> >>>>> vgic_set_assigned_irq - set state of IRQs driven by irqfd >>>>> >>>>> When an IRQ is raised or lowered.... blah blah blah blah. >>>>> >>>>> @e: the routing entry describing... >>>>> @kvm: the kvm struct >>>>> and so on. >>>> >>>>> >>>>> return 0 on success, -error on errors. >>>>> >>>> >>>> OK >>>>>> + >>>>>> +static int vgic_set_assigned_irq(struct kvm_kernel_irq_routing_entry *e, >>>>>> + struct kvm *kvm, int irq_source_id, int level, >>>>>> + bool line_status) >>>>>> +{ >>>>>> + unsigned int spi = e->irqchip.pin + VGIC_NR_PRIVATE_IRQS; >>>>> >>>>> I noticed this in the changelogs too, where's the rationale/API >>>>> documentaiton for the use of irqchip.pin and its semantics on ARM? >>>>> >>>>> Should we add something in Documentation/virtual/kvm/api.txt ? >>>> >>>> - in 4.24 KVM_CREATE_IRQCHIP I might add that similarly to s390 a dummy >>>> identity table is created. >>>> >>>> - KVM_CAP_IRQFD says "kvm_irqfd.gsi specifies the irqchip pin toggled by >>>> this event. When an event is triggered on the eventfd, an interrupt is >>>> injected into the guest using the specified gsi pin" >>>> >>>> Assuming the standard use case is to use an identity/dummy GSI table the >>>> irqchip.pin still remains the "irqchip pin" toggled on eventfd. >>>> >>>> By the way I might remove the mention to the use case where the gsi != >>>> irqchip.pin. >>>> >>>> Now when reading 4.25 KVM_IRQ_LINE, it is said that it is used to inject >>>> a GSI as well. >>>> >>>> Now on ARM The GSI has the following content. >>>> >>>> bits: | 31 ... 24 | 23 ... 16 | 15 ... 0 | >>>> field: | irq_type | vcpu_index | irq_id | >>>> >>>> As such that's true that currently there is an inconsistency. >>>> >>>> Currently my GSI == spi number whereas the GSI as injected by >>>> KVM_IRQ_LINE has the above format. >>> >>> But the ARM use of KVM_IRQFD is not clearly documented. >>> >>> I think this patch needs to include adding arm to the "Architectures" >>> list in api.txt and clearly document what the semantics of the fields of >>> the struct kvm_irqfd are. >> >> Yes indeed, this deserves such clarification, all the more so there is >> the above inconsistency. We come back to the discussion about relevance >> of routing other things than SPI actually. use case? >>> >>>>> >>>>>> + >>>>>> + if (irq_source_id == KVM_USERSPACE_IRQ_SOURCE_ID) { >>>>>> + /* >>>>>> + * This path is not tested yet, >>>>>> + * only irqchip with resampler was exercised >>>>>> + */ >>>>>> + kvm_vgic_inject_irq(kvm, 0, spi, level); >>>>> >>>>> hmmm, stuff like this definitely makes this patch an RFC. >>>> >>>> Yes I acknowledge I shall step back to RFC. I was a bit eager. >>> >>> The alternative is to return an error and put a big fat comment saying >>> this is NOT IMPLEMENTED yet. >> >> actually this KVM_USERSPACE_IRQ_SOURCE_ID path is entered when injecting >> IRQs without resamplerfd (is edge sensitive ones). As such we can link >> this issue to your above related comment. >> >> Anyway I will move it to RFC since there are quite a lot of open points, >> especially the next one. >>> >>>>> >>>>>> + } else if (irq_source_id == KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID) { >>>>>> + if (level == 1) { >>>>> >>>>> This seems very wrong. What if an external device raises a >>>>> level-triggered IRQ, but then lowers it again, without the guest was >>>>> ever running, now you have to wait until the guest sees the (now >>>>> inactive) interrupt and EOIs it before the interrupt is lowered on the >>>>> vgic? >>>> Hum I am bit confused here. when you enter that code, this means an >>>> irqfd was triggered. This irqfd was registered by some user code that is >>>> supposed to be alive and do the proper action to complete the IRQ. in my >>>> case the xgmac toggles down the IRQ if anyone resets the IRQ status >>>> register? this action is done in the xgmac guest ISR. A device >>>> reset/error? might provoke the IRQ pin toggling done. Do you see any >>>> other events? >>> >>> Sure, ok, forget about the 'guest wasn't running', but if the guest ISR >>> is run with guest interrupts disabled and resets the xgmac device (or >>> imagine some other device that just lowers the level triggered interrupt >>> after some timeout), then how does the current code handle this? >> yes I aknowledge current implementation relies on 2 assumptions: >> - either the EOI happens or >> - the pending IRQ is cleared through ICPENDRn access >> >> and in case of reset typically we are in trouble to clear the distributor. >> >> But I currently do not see any way to detect the device IRQ is toggled >> down without trapping something. > > I guess that's hard without trapping or probing the VFIO driver, but the > current scheme just seems to fragile and seems to be built around a very > specific behavior of your guest and host, and not supporting a generic > solution. > > Either we will have to trap MMIO to know what's going on, or perhaps > probe the VFIO state when the interrupt is enabled on the vgic by the > guest. > agreed. >>> >>>>> >>>>>> + kvm_debug("Inject irqchip routed vIRQ %d\n", >>>>>> + e->irqchip.pin); >>>>>> + kvm_vgic_inject_irq(kvm, 0, spi, level); >>>>>> + /* >>>>>> + * toggling down vIRQ wire is directly handled in >>>>>> + * process_maintenance for this reason: >>>>>> + * irqfd_resampler_ack is called in >>>>>> + * process_maintenance which holds the dist lock. >>>>>> + * irqfd_resampler_ack calls kvm_set_irq >>>>>> + * which ends_up calling kvm_vgic_inject_irq. >>>>>> + * This later attempts to take the lock -> deadlock! >>>>>> + */ >>>>> >>>>> Not sure I understand this comment. What are we trying to achieve, are >>>>> we using some sort of a workaround to avoid a deadlock? >>>> >>>> What I wanted to point out here is I would have prefered to handle both >>>> levels 0 and 1 in a symetrical manner. irqfd_resampler_ack (in eventfd) >>>> is calling kvm_set_irq with level 0. This would be the prefered way to >>>> toggle down the SPI at GIC input instead of doing this in >>>> process_maintenance in a dirty manner. However this does work because >>>> irqfd_resampler_ack is called in process_maintenance (the place where >>>> the EOI is analyzed). process_maintenance holds the dist lock and would >>>> eventually call kvm_vgic_inject_irq which also attempts to take the lock. >>>> >>> >>> I'm afraid that's too much of a hack. There's an external mechanism to >>> set an interrupt line to active (level=1) or inactive (level=0) and we >>> must support both. >> >>> The fact that vgic_process_maintenance() can set the interrupt line to >>> inactive is just something we exploit to properly handle level-triggered >>> interrupts, but the main API to the VGIC must absolutely be supported. >>> >>> Am I completely wrong here? >> >> Well I am not sure what you call here the VGIC API? Is it >> kvm_vgic_inject_irq? I agree with you on the fact It would be cleaner to >> use this later in both edge transitions. > > Yes, the "set" function pointer, which eventually calls > vgic_set_assigned_irq(), is your API. If that API carries semantics > that you can raise AND lower the irq through that API, then we need to > support that I think. agreed too > > Thanks, > -Christoffer > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
