Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On 12 December 2011 17:40, Avi Kivity a...@redhat.com 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 jan.kis...@web.de 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 11/12/11 20:07, Christoffer Dall wrote: On Dec 11, 2011, at 2:48 PM, Peter Maydell peter.mayd...@linaro.org wrote: On 11 December 2011 19:30, Christoffer Dall c.d...@virtualopensystems.com wrote: On Sun, Dec 11, 2011 at 11:03 AM, Peter Maydell peter.mayd...@linaro.org 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 Mon, Dec 12, 2011 at 6:06 AM, Marc Zyngier marc.zyng...@arm.com wrote: On 11/12/11 20:07, Christoffer Dall wrote: On Dec 11, 2011, at 2:48 PM, Peter Maydell peter.mayd...@linaro.org wrote: On 11 December 2011 19:30, Christoffer Dall c.d...@virtualopensystems.com wrote: On Sun, Dec 11, 2011 at 11:03 AM, Peter Maydell peter.mayd...@linaro.org 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: [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
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).
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.
+
+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.
+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.
+ 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').
+ } else
+ vcpu-arch.virt_irq = ~mask;
+
+ return 0;
+}
+
--
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 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
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 9:50 AM, Avi Kivity a...@redhat.com 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 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 11 December 2011 23:01, Jan Kiszka jan.kis...@web.de 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 06:31 PM, Peter Maydell wrote: On 11 December 2011 23:01, Jan Kiszka jan.kis...@web.de 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
[PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
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.d...@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.
+
+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;
+ }
+
+ 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 =
Re: [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
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.d...@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?
+
+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;
Re: [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On 11 December 2011 15:18, Jan Kiszka jan.kis...@web.de wrote:
Just found two, maybe three nits while browsing by:
On 2011-12-11 11:24, Christoffer Dall wrote:
+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.
This seems to me a slightly obscure way of defining the two fields
in this word (ie bits [31..1] cpu number, bit [0] irq-vs-fiq).
+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?
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?)
-- 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: [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On Sun, Dec 11, 2011 at 10:18 AM, Jan Kiszka jan.kis...@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.d...@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;
+
Re: [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On Sun, Dec 11, 2011 at 11:03 AM, Peter Maydell
peter.mayd...@linaro.org wrote:
On 11 December 2011 15:18, Jan Kiszka jan.kis...@web.de wrote:
Just found two, maybe three nits while browsing by:
On 2011-12-11 11:24, Christoffer Dall wrote:
+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.
This seems to me a slightly obscure way of defining the two fields
in this word (ie bits [31..1] cpu number, bit [0] irq-vs-fiq).
Isn't that just personal preference? The other scheme was suggested by
Avi, and nobody else complained then, so I'd be inclined to just leave
it as is.
+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?
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.
--
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: [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On 11 December 2011 19:30, Christoffer Dall c.d...@virtualopensystems.com wrote: On Sun, Dec 11, 2011 at 11:03 AM, Peter Maydell peter.mayd...@linaro.org 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 :-) -- 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 peter.mayd...@linaro.org wrote: On 11 December 2011 19:30, Christoffer Dall c.d...@virtualopensystems.com wrote: On Sun, Dec 11, 2011 at 11:03 AM, Peter Maydell peter.mayd...@linaro.org 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
Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace
On 11 December 2011 20:07, Christoffer Dall christofferd...@christofferdall.dk 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 Sun, Dec 11, 2011 at 3:25 PM, Peter Maydell peter.mayd...@linaro.org wrote: On 11 December 2011 20:07, Christoffer Dall christofferd...@christofferdall.dk 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 21:36, Christoffer Dall c.d...@virtualopensystems.com wrote: On Sun, Dec 11, 2011 at 3:25 PM, Peter Maydell peter.mayd...@linaro.org wrote: On 11 December 2011 20:07, Christoffer Dall christofferd...@christofferdall.dk 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 11 December 2011 22:12, Peter Maydell peter.mayd...@linaro.org 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 Sun, Dec 11, 2011 at 5:35 PM, Peter Maydell peter.mayd...@linaro.org wrote: On 11 December 2011 22:12, Peter Maydell peter.mayd...@linaro.org 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 2011-12-11 23:53, Christoffer Dall wrote: On Sun, Dec 11, 2011 at 5:35 PM, Peter Maydell peter.mayd...@linaro.org wrote: On 11 December 2011 22:12, Peter Maydell peter.mayd...@linaro.org 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 11.12.2011, at 20:48, Peter Maydell peter.mayd...@linaro.org wrote: On 11 December 2011 19:30, Christoffer Dall c.d...@virtualopensystems.com wrote: On Sun, Dec 11, 2011 at 11:03 AM, Peter Maydell peter.mayd...@linaro.org 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
