Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On 12 December 2011 17:40, Avi Kivity wrote: > On 12/12/2011 06:31 PM, Peter Maydell wrote: >> I think with an in-kernel GIC model you'd only need to be able to set >> one of the (256 including internal-to-the-CPU inputs) GIC input lines; >> the GIC itself then connects directly to the vcpu IRQ and FIQ. >> >> So we could just have different semantics for the ioctl in the 'kernel >> GIC model enabled' config, as you suggest. > > btw, since we use the KVM_IRQ_LINE ioctl, it may make sense to require > KVM_CREATE_IRQCHIP. To create a kernel GIC model, just call > KVM_CREATE_IRQCHIP with a different parameter. This removes an "except > for ARM" from the documentation. Just to yank this thread back from the dead, Christoffer pointed out that at the moment QEMU only calls KVM_CREATE_IRQCHIP if the user asked for one on the command line. This makes sense to me, since KVM_CREATE_IRQCHIP is saying "create me an in-kernel interrupt controller", and so for ARM it ought to mean "create an in-kernel GIC model"... So I think it would be better just to fix the documentation to note that on some architectures it doesn't make sense to call KVM_IRQ_LINE unless you've previously asked for an in kernel irq chip via KVM_CREATE_IRQCHIP. -- PMM -- 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
Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On Dec 12, 2011, at 12:40 PM, Avi Kivity wrote: > On 12/12/2011 06:31 PM, Peter Maydell wrote: >> On 11 December 2011 23:01, Jan Kiszka wrote: >>> Enabling in-kernel irqchips usually means "switching worlds". So the >>> semantics of these particular IRQ inject interface details may change >>> without breaking anything. >>> >>> However, things might look different if there will be a need to inject >>> also the CPU IRQs directly, not only the irqchip inputs. In that case, >>> it may make some sense to reserve more space for interrupt types than >>> just one bit and use a common encoding scheme. >> >> I think with an in-kernel GIC model you'd only need to be able to set >> one of the (256 including internal-to-the-CPU inputs) GIC input lines; >> the GIC itself then connects directly to the vcpu IRQ and FIQ. >> >> So we could just have different semantics for the ioctl in the 'kernel >> GIC model enabled' config, as you suggest. > > btw, since we use the KVM_IRQ_LINE ioctl, it may make sense to require > KVM_CREATE_IRQCHIP. To create a kernel GIC model, just call > KVM_CREATE_IRQCHIP with a different parameter. This removes an "except > for ARM" from the documentation. I added this, but it feels a bit contrived. Please take a look when I post the next patch series if this is what you have in mind. Thanks.-- 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
Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On 12/12/2011 06:31 PM, Peter Maydell wrote: > On 11 December 2011 23:01, Jan Kiszka wrote: > > Enabling in-kernel irqchips usually means "switching worlds". So the > > semantics of these particular IRQ inject interface details may change > > without breaking anything. > > > > However, things might look different if there will be a need to inject > > also the CPU IRQs directly, not only the irqchip inputs. In that case, > > it may make some sense to reserve more space for interrupt types than > > just one bit and use a common encoding scheme. > > I think with an in-kernel GIC model you'd only need to be able to set > one of the (256 including internal-to-the-CPU inputs) GIC input lines; > the GIC itself then connects directly to the vcpu IRQ and FIQ. > > So we could just have different semantics for the ioctl in the 'kernel > GIC model enabled' config, as you suggest. btw, since we use the KVM_IRQ_LINE ioctl, it may make sense to require KVM_CREATE_IRQCHIP. To create a kernel GIC model, just call KVM_CREATE_IRQCHIP with a different parameter. This removes an "except for ARM" from the documentation. -- error compiling committee.c: too many arguments to function -- 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
Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On 11 December 2011 23:01, Jan Kiszka wrote: > Enabling in-kernel irqchips usually means "switching worlds". So the > semantics of these particular IRQ inject interface details may change > without breaking anything. > > However, things might look different if there will be a need to inject > also the CPU IRQs directly, not only the irqchip inputs. In that case, > it may make some sense to reserve more space for interrupt types than > just one bit and use a common encoding scheme. I think with an in-kernel GIC model you'd only need to be able to set one of the (256 including internal-to-the-CPU inputs) GIC input lines; the GIC itself then connects directly to the vcpu IRQ and FIQ. So we could just have different semantics for the ioctl in the 'kernel GIC model enabled' config, as you suggest. -- PMM -- 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
Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On 12/12/2011 05:11 PM, Christoffer Dall wrote: > >> If I should re-use the existing one, should I simply move it outside > >> of __KVM_HAVE_IOAPIC? > > > > Protect it with __KVM_HAVE_IRQ_LINE so we don't leak unused tracepoints > > for other archs. > > > > ok. I used to be scared of touching your arch independent stuff, but > maybe I should ease up there... Yup. We should also make more x86 code arch indepedent where possible. -- error compiling committee.c: too many arguments to function -- 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
Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On Mon, Dec 12, 2011 at 9:50 AM, Avi Kivity wrote: > On 12/12/2011 04:38 PM, Christoffer Dall wrote: >> > >> > 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. > > > gsi = number of irq line, just a bad name, but you do have it on ARM. > > irq_source_id really shouldn't have been in kvm_set_irq(), it's an > implementation detail rather than an architectural feature; just ignore it. > >> What's the >> harm of this additional tracepoint? > > If we get tools that use them, they have an additional difference to > consider. It's a weak argument but it's there. > ok, I am all for re-using as much as possible. >> If I should re-use the existing one, should I simply move it outside >> of __KVM_HAVE_IOAPIC? > > Protect it with __KVM_HAVE_IRQ_LINE so we don't leak unused tracepoints > for other archs. > ok. I used to be scared of touching your arch independent stuff, but maybe I should ease up there... >> >> + 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. > > Yes please. It's hard to review this way. Fold all the smp stuff into > the patches which introduce the functionality. > sorry about that, I see this pretty clearly after the fact. >> >> >> + 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. > > Ok. I think you can reuse x86 concepts and even code. I'll accept > patches to make code arch independent ahead of the merge if it helps. > ok, I'll look into it. -- 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
Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On 12/12/2011 04:38 PM, Christoffer Dall wrote: > > > > 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. gsi = number of irq line, just a bad name, but you do have it on ARM. irq_source_id really shouldn't have been in kvm_set_irq(), it's an implementation detail rather than an architectural feature; just ignore it. > What's the > harm of this additional tracepoint? If we get tools that use them, they have an additional difference to consider. It's a weak argument but it's there. > If I should re-use the existing one, should I simply move it outside > of __KVM_HAVE_IOAPIC? Protect it with __KVM_HAVE_IRQ_LINE so we don't leak unused tracepoints for other archs. > >> + 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. Yes please. It's hard to review this way. Fold all the smp stuff into the patches which introduce the functionality. > > >> + 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. Ok. I think you can reuse x86 concepts and even code. I'll accept patches to make code arch independent ahead of the merge if it helps. -- error compiling committee.c: too many arguments to function -- 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
Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On Mon, Dec 12, 2011 at 8:28 AM, Avi Kivity 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 ; 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
Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On Mon, Dec 12, 2011 at 6:06 AM, Marc Zyngier wrote: > On 11/12/11 20:07, Christoffer Dall wrote: >> On Dec 11, 2011, at 2:48 PM, Peter Maydell wrote: >> >>> On 11 December 2011 19:30, Christoffer Dall >>> wrote: On Sun, Dec 11, 2011 at 11:03 AM, Peter Maydell wrote: > Removing the mask would be wrong since the irq field here > is encoding both cpu number and irq-vs-fiq. The default is > just an unreachable condition. (Why are we using % here > rather than the obvious bit operation, incidentally?) > right, I will remove the default case. I highly doubt that the difference in using a bitop will be measurably more efficient, but if you feel strongly about it, I can change it to a shift and bitwise and, which I assume is what you mean by the obvious bit operation? I think my CS background speaks for using %, but whatever. >>> >>> Certainly the compiler ought to be able to figure out the >>> two are the same thing; I just think "irq & 1" is more readable >>> than "irq % 2" (because it's being clear that it's treating the >>> variable as a pile of bits rather than an integer). This is >>> bikeshedding rather, though, and style issues in kernel code >>> are a matter for the kernel folk. So you can ignore me :-) >>> >> Well, if it was just "irq & 1", then I hear you, but it would be "(irq cpu_idx) & 1" which I don't think is more clear. >> >> But yes let's see what the kernel folks say. > > The general consensus is to use bit operations rather than arithmetic. > The compiler will usually convert the "% 2" pattern into a shift, but I > tend to agree with Peter on the readability of the thing. When encoding > multiple information in a word, bit operations should be used, as they > make it obvious which part of the word contains the bit you're > interested in. > > But I've probably been corrupted by working with HW guys for a bit too > long... ;-) > > ok, ok, I'll change it to a bit op. Can't wait for the dazzling performance improvement ;) -- 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
Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On 11/12/11 20:07, Christoffer Dall wrote: > On Dec 11, 2011, at 2:48 PM, Peter Maydell wrote: > >> On 11 December 2011 19:30, Christoffer Dall >> wrote: >>> On Sun, Dec 11, 2011 at 11:03 AM, Peter Maydell >>> wrote: Removing the mask would be wrong since the irq field here is encoding both cpu number and irq-vs-fiq. The default is just an unreachable condition. (Why are we using % here rather than the obvious bit operation, incidentally?) >>> right, I will remove the default case. >>> >>> I highly doubt that the difference in using a bitop will be measurably >>> more efficient, but if you feel strongly about it, I can change it to >>> a shift and bitwise and, which I assume is what you mean by the >>> obvious bit operation? I think my CS background speaks for using %, >>> but whatever. >> >> Certainly the compiler ought to be able to figure out the >> two are the same thing; I just think "irq & 1" is more readable >> than "irq % 2" (because it's being clear that it's treating the >> variable as a pile of bits rather than an integer). This is >> bikeshedding rather, though, and style issues in kernel code >> are a matter for the kernel folk. So you can ignore me :-) >> > Well, if it was just "irq & 1", then I hear you, but it would be "(irq >>> cpu_idx) & 1" which I don't think is more clear. > > But yes let's see what the kernel folks say. The general consensus is to use bit operations rather than arithmetic. The compiler will usually convert the "% 2" pattern into a shift, but I tend to agree with Peter on the readability of the thing. When encoding multiple information in a word, bit operations should be used, as they make it obvious which part of the word contains the bit you're interested in. But I've probably been corrupted by working with HW guys for a bit too long... ;-) M. -- Jazz is not dead. It just smells funny... -- 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
Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On 11.12.2011, at 20:48, Peter Maydell wrote: > On 11 December 2011 19:30, Christoffer Dall > wrote: >> On Sun, Dec 11, 2011 at 11:03 AM, Peter Maydell >> wrote: >>> Removing the mask would be wrong since the irq field here >>> is encoding both cpu number and irq-vs-fiq. The default is >>> just an unreachable condition. (Why are we using % here >>> rather than the obvious bit operation, incidentally?) >>> >> right, I will remove the default case. >> >> I highly doubt that the difference in using a bitop will be measurably >> more efficient, but if you feel strongly about it, I can change it to >> a shift and bitwise and, which I assume is what you mean by the >> obvious bit operation? I think my CS background speaks for using %, >> but whatever. > > Certainly the compiler ought to be able to figure out the > two are the same thing; I just think "irq & 1" is more readable > than "irq % 2" (because it's being clear that it's treating the > variable as a pile of bits rather than an integer). This is > bikeshedding rather, though, and style issues in kernel code > are a matter for the kernel folk. So you can ignore me :-) Yes, the general rule of thumb is to use bit operations where you can. And in this case it certainly makes sense :). Plus, bit operations are an order of magnitude faster than div/mod usually. Alex -- 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
Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On 2011-12-11 23:53, Christoffer Dall wrote: > On Sun, Dec 11, 2011 at 5:35 PM, Peter Maydell > wrote: >> On 11 December 2011 22:12, Peter Maydell wrote: >>> (Actually what would be clearest would be if the ioctl took the >>> (interrupt-target, interrupt-line-for-that-target, value-of-line) >>> tuple as three separate values rather than encoding two of them into >>> a single integer, but I assume there's a reason we can't have that.) >> >> Have you thought about how this encoding scheme would be extended >> when we move to using the VGIC and an in-kernel interrupt controller >> implementation, incidentally? I haven't really looked into that at >> all, but I assume that then QEMU is going to start having to tell >> the kernel it wants to deliver interrupt 35 to the GIC, and so on... >> >> > no, I haven't looked into that at all. My plan was to decipher the > common irq, ioapic stuff for x86 and see how much we can re-use and if > there will be some nice way to either use what's there or change some > bits to accomodate both existing archs and ARM. But the short answer > is, no not really, I was focusing so far on getting a stable > implementation upstream. > > yes, we are going to have to have some interface with QEMU for this > and if we need new features from what's already there that should > probably be discussed in the same round as the mechanism for handing > of CP15 stuff to QEMU that we touched upon earlier. Enabling in-kernel irqchips usually means "switching worlds". So the semantics of these particular IRQ inject interface details may change without breaking anything. However, things might look different if there will be a need to inject also the CPU IRQs directly, not only the irqchip inputs. In that case, it may make some sense to reserve more space for interrupt types than just one bit and use a common encoding scheme. Jan signature.asc Description: OpenPGP digital signature
Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On Sun, Dec 11, 2011 at 5:35 PM, Peter Maydell wrote: > On 11 December 2011 22:12, Peter Maydell wrote: >> (Actually what would be clearest would be if the ioctl took the >> (interrupt-target, interrupt-line-for-that-target, value-of-line) >> tuple as three separate values rather than encoding two of them into >> a single integer, but I assume there's a reason we can't have that.) > > Have you thought about how this encoding scheme would be extended > when we move to using the VGIC and an in-kernel interrupt controller > implementation, incidentally? I haven't really looked into that at > all, but I assume that then QEMU is going to start having to tell > the kernel it wants to deliver interrupt 35 to the GIC, and so on... > > no, I haven't looked into that at all. My plan was to decipher the common irq, ioapic stuff for x86 and see how much we can re-use and if there will be some nice way to either use what's there or change some bits to accomodate both existing archs and ARM. But the short answer is, no not really, I was focusing so far on getting a stable implementation upstream. yes, we are going to have to have some interface with QEMU for this and if we need new features from what's already there that should probably be discussed in the same round as the mechanism for handing of CP15 stuff to QEMU that we touched upon earlier. -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
Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On 11 December 2011 22:12, Peter Maydell wrote: > (Actually what would be clearest would be if the ioctl took the > (interrupt-target, interrupt-line-for-that-target, value-of-line) > tuple as three separate values rather than encoding two of them into > a single integer, but I assume there's a reason we can't have that.) Have you thought about how this encoding scheme would be extended when we move to using the VGIC and an in-kernel interrupt controller implementation, incidentally? I haven't really looked into that at all, but I assume that then QEMU is going to start having to tell the kernel it wants to deliver interrupt 35 to the GIC, and so on... -- PMM -- 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
Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On 11 December 2011 21:36, Christoffer Dall wrote: > On Sun, Dec 11, 2011 at 3:25 PM, Peter Maydell > wrote: >> On 11 December 2011 20:07, Christoffer Dall >> wrote: >>> Well, if it was just "irq & 1", then I hear you, but it would be "(irq >>> >> cpu_idx) & 1" which I don't think is more clear. >> >> Er, what? The fields are [31..1] CPU index and [0] irqtype, >> right? So what you have now is: >> vcpu_idx = irq_level->irq / 2; >> irqtype = irq_level->irq % 2; >> and the bitshifting equivalent is: >> vcpu_idx = irq_level->irq >> 1; >> irqtype = irq_level->irq & 1; >> surely? >> >> Shifting by the cpuindex is definitely wrong. > > actually, that's not how the irq_level field is defined. It's not clear to me which part of my comment this is aimed at. Shifting by the cpuindex doesn't give the right answer whether you define irq_level by bitfields or with the current phrasing you quote below. > If you look > in Documentation/virtual/kvm/api.txt: > > "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." That's exactly the same thing, though, right? It's just a matter of how you choose to phrase it (in either text or in code; the values come out identical). When I was sorting out the QEMU side, I started out by looking at the kernel source code, deduced that we were encoding CPU number and irq-vs-fiq as described above (and documenting it in a slightly confusing way as a multiplication) and then wrote the qemu code in what seemed to me the clearest way. (Actually what would be clearest would be if the ioctl took the (interrupt-target, interrupt-line-for-that-target, value-of-line) tuple as three separate values rather than encoding two of them into a single integer, but I assume there's a reason we can't have that.) > we should definitely fix either side, and the only sane argument is > that this is an irq_line field, so an index resembling an actual line > seems more semantically in line with the field purpose rather than a > bit encoding, but I am open to arguments and not married to the > current implementation. To be clear, I'm not attempting to suggest a change in the semantics of this field. (The qemu patch fixes the qemu side to adhere to what the kernel requires.) -- PMM -- 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
Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On Sun, Dec 11, 2011 at 3:25 PM, Peter Maydell wrote: > On 11 December 2011 20:07, Christoffer Dall > wrote: >> Well, if it was just "irq & 1", then I hear you, but it would be "(irq >> >> cpu_idx) & 1" which I don't think is more clear. > > Er, what? The fields are [31..1] CPU index and [0] irqtype, > right? So what you have now is: > vcpu_idx = irq_level->irq / 2; > irqtype = irq_level->irq % 2; > and the bitshifting equivalent is: > vcpu_idx = irq_level->irq >> 1; > irqtype = irq_level->irq & 1; > surely? > > Shifting by the cpuindex is definitely wrong. actually, that's not how the irq_level field is defined. If you look in Documentation/virtual/kvm/api.txt: "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." also, in the kernel code the cpu_index is achieved by a simple integer division by 2. as I said, this was the proposal from the last round of reviews after a lengthy discussion, so I sticked with that. we should definitely fix either side, and the only sane argument is that this is an irq_line field, so an index resembling an actual line seems more semantically in line with the field purpose rather than a bit encoding, but I am open to arguments and not married to the current implementation. > (Incidentally I fixed a bug in your QEMU-side code which wasn't > feeding this field to the kernel in the way it expects: > > http://git.linaro.org/gitweb?p=qemu/qemu-linaro.git;a=commitdiff;h=2502ba067e795e48d346f9816fad45177ca64bca > > Sorry, I should have posted that to the list. I'll do that now.) -- 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
Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On 11 December 2011 20:07, Christoffer Dall wrote: > Well, if it was just "irq & 1", then I hear you, but it would be "(irq > >> cpu_idx) & 1" which I don't think is more clear. Er, what? The fields are [31..1] CPU index and [0] irqtype, right? So what you have now is: vcpu_idx = irq_level->irq / 2; irqtype = irq_level->irq % 2; and the bitshifting equivalent is: vcpu_idx = irq_level->irq >> 1; irqtype = irq_level->irq & 1; surely? Shifting by the cpuindex is definitely wrong. (Incidentally I fixed a bug in your QEMU-side code which wasn't feeding this field to the kernel in the way it expects: http://git.linaro.org/gitweb?p=qemu/qemu-linaro.git;a=commitdiff;h=2502ba067e795e48d346f9816fad45177ca64bca Sorry, I should have posted that to the list. I'll do that now.) -- PMM -- 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
Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On Dec 11, 2011, at 2:48 PM, Peter Maydell wrote: > On 11 December 2011 19:30, Christoffer Dall > wrote: >> On Sun, Dec 11, 2011 at 11:03 AM, Peter Maydell >> wrote: >>> Removing the mask would be wrong since the irq field here >>> is encoding both cpu number and irq-vs-fiq. The default is >>> just an unreachable condition. (Why are we using % here >>> rather than the obvious bit operation, incidentally?) >>> >> right, I will remove the default case. >> >> I highly doubt that the difference in using a bitop will be measurably >> more efficient, but if you feel strongly about it, I can change it to >> a shift and bitwise and, which I assume is what you mean by the >> obvious bit operation? I think my CS background speaks for using %, >> but whatever. > > Certainly the compiler ought to be able to figure out the > two are the same thing; I just think "irq & 1" is more readable > than "irq % 2" (because it's being clear that it's treating the > variable as a pile of bits rather than an integer). This is > bikeshedding rather, though, and style issues in kernel code > are a matter for the kernel folk. So you can ignore me :-) > Well, if it was just "irq & 1", then I hear you, but it would be "(irq >> cpu_idx) & 1" which I don't think is more clear. But yes let's see what the kernel folks say. -- 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