Re: [Qemu-devel] [Android-virt] [PATCH 02/12] arm: make the number of GIC interrupts configurable
On 27 January 2012 00:33, Rusty Russell ru...@rustcorp.com.au wrote:
Peter Maydell wrote:
Anyway, if we would otherwise die horribly later on we should
catch these cases, but it would be good to have at least a comment
saying that these are implementation limitations rather than
architectural ones.
Good point. If we add an supported bit to each irq, we could do weird
things, but presumably -num_irq would still correspond to
ITLinesNumber.
I don't want to put too much of an essay in there. How's this:
/* ITLinesNumber is represented as (N - 32) / 1. See
gic_dist_readb. */
if (s-num_irq 32 || (s-num_irq % 32)) {
hw_error(%u interrupt lines unsupported: not divisible by
32\n,
num_irq);
I think that's a notch too terse for my taste. How about:
/* ITLinesNumber is represented as (N - 32) / 1 (see
* gic_dist_readb) so this is an implementation imposed
* restriction, not an architectural one:
*/
thanks
-- PMM
Re: [Qemu-devel] [Android-virt] [PATCH 02/12] arm: make the number of GIC interrupts configurable
On Wed, 25 Jan 2012 15:09:41 +, Peter Maydell peter.mayd...@linaro.org
wrote:
On 24 January 2012 08:42, Rusty Russell ru...@rustcorp.com.au wrote:
Reading through this, I see a lot of - 32. Trivial patch follows,
which applies to your rebasing branch:
(If you send patches as fresh new emails then they just apply
with git am without needing manual cleanup, appear with sensible
subjects in patchwork/patches.linaro, etc.)
Indeed, but it's so conversaionally gauche :( I thought git am did the
Right Thing, but it doesn't, and --scissors doesn't help either (it gets
the wrong Subject line). Oh well, I'll do it that way in future.
Subject: ARM: clean up GIC constants.
From: Rusty Russell ru...@rustcorp.com.au
Interrupts numbers 0-31 are private to the processor interface, 32-1019 are
general interrups. Add GIC_INTERNAL and substitute everywhere.
Also, add a check that the total number of interrupts is divisible by
32 (required for reporting interupt numbers, see gic_dist_readb(), and
is greater than 32. And remove a single stray tab.
I agree with Avi that the presence of Also in a git
commit message is generally a sign you should have submitted
more than one patch :-)
Indeed, guilty as charged :)
Signed-off-by: Rusty Russell ru...@rustcorp.com.au
---
hw/arm_gic.c | 48 ++--
1 files changed, 26 insertions(+), 22 deletions(-)
diff --git a/hw/arm_gic.c b/hw/arm_gic.c
index cf582a5..a29eacb 100644
--- a/hw/arm_gic.c
+++ b/hw/arm_gic.c
@@ -13,6 +13,8 @@
/* Maximum number of possible interrupts, determined by the GIC
architecture */
#define GIC_MAXIRQ 1020
+/* First 32 are private to each CPU (SGIs and PPIs). */
+#define GIC_INTERNAL 32
//#define DEBUG_GIC
#ifdef DEBUG_GIC
@@ -74,7 +76,7 @@ typedef struct gic_irq_state
#define GIC_CLEAR_TRIGGER(irq) s-irq_state[irq].trigger = 0
#define GIC_TEST_TRIGGER(irq) s-irq_state[irq].trigger
#define GIC_GET_PRIORITY(irq, cpu) \
- (((irq) 32) ? s-priority1[irq][cpu] : s-priority2[(irq) - 32])
+ (((irq) GIC_INTERNAL) ? s-priority1[irq][cpu] : s-priority2[(irq) -
GIC_INTERNAL])
This line is now 80 characters and needs folding.
(scripts/checkpatch.pl will catch this kind of nit.)
Will do.
@@ -812,11 +814,13 @@ static void gic_init(gic_state *s, int num_irq)
s-num_cpu = num_cpu;
#endif
s-num_irq = num_irq + GIC_BASE_IRQ;
- if (s-num_irq GIC_MAXIRQ) {
- hw_error(requested %u interrupt lines exceeds GIC maximum %d\n,
- num_irq, GIC_MAXIRQ);
+ if (s-num_irq GIC_MAXIRQ
+ || s-num_irq GIC_INTERNAL
+ || (s-num_irq % 32) != 0) {
So I guess our implementation isn't likely to work properly for a
non-multiple-of-32 number of IRQs, but this isn't an architectural GIC
restriction. (In fact the GIC architecture spec allows the supported
interrupt IDs to not even be in a contiguous range, which we certainly
don't support...)
Yes, I intuited it from here:
static uint32_t gic_dist_readb(void *opaque, target_phys_addr_t offset)
{
...
if (offset == 4)
return ((s-num_irq / 32) - 1) | ((NUM_CPU(s) - 1) 5);
If want want to support non-32-divisible # irqs, we need at least:
((s-num_irq + 31) / 32 - 1)
Seemed easier to have an initialization-time assert than check
everywhere else for overflows.
I also think it's architecturally permitted that not all the internal
(SPI/PPI) interrupts are implemented, ie that s-num_irq 32 (you
have to read the GIC architecture manually quite closely to deduce
this, though).
This made me read that part of the manual... interesting.
Anyway, if we would otherwise die horribly later on we should
catch these cases, but it would be good to have at least a comment
saying that these are implementation limitations rather than
architectural ones.
Good point. If we add an supported bit to each irq, we could do weird
things, but presumably -num_irq would still correspond to
ITLinesNumber.
I don't want to put too much of an essay in there. How's this:
/* ITLinesNumber is represented as (N - 32) / 1. See
gic_dist_readb. */
if (s-num_irq 32 || (s-num_irq % 32)) {
hw_error(%u interrupt lines unsupported: not divisible by
32\n,
num_irq);
Beefing up the parameter check should be a separate patch.
Thanks, coming soon.
I should be getting access to git.linaro.org RSN, so I can post there
for easier merging.
Thanks,
Rusty.
Re: [Qemu-devel] [Android-virt] [PATCH 02/12] arm: make the number of GIC interrupts configurable
On 24 January 2012 08:42, Rusty Russell ru...@rustcorp.com.au wrote:
On Fri, 13 Jan 2012 20:52:39 +, Peter Maydell peter.mayd...@linaro.org
wrote:
From: Mark Langsdorf mark.langsd...@calxeda.com
Increase the maximum number of GIC interrupts for a9mp and a11mp to 1020,
and create a configurable property for each defaulting to 96 and 64
(respectively) so that device modelers can set the value appropriately
for their SoC. Other ARM processors also set their maximum number of
used IRQs appropriately.
Set the maximum theoretical number of GIC interrupts to 1020 and
update the save/restore code to only use the appropriate number for
each SoC.
Reading through this, I see a lot of - 32. Trivial patch follows,
which applies to your rebasing branch:
(If you send patches as fresh new emails then they just apply
with git am without needing manual cleanup, appear with sensible
subjects in patchwork/patches.linaro, etc.)
Subject: ARM: clean up GIC constants.
From: Rusty Russell ru...@rustcorp.com.au
Interrupts numbers 0-31 are private to the processor interface, 32-1019 are
general interrups. Add GIC_INTERNAL and substitute everywhere.
Also, add a check that the total number of interrupts is divisible by
32 (required for reporting interupt numbers, see gic_dist_readb(), and
is greater than 32. And remove a single stray tab.
I agree with Avi that the presence of Also in a git
commit message is generally a sign you should have submitted
more than one patch :-)
Signed-off-by: Rusty Russell ru...@rustcorp.com.au
---
hw/arm_gic.c | 48 ++--
1 files changed, 26 insertions(+), 22 deletions(-)
diff --git a/hw/arm_gic.c b/hw/arm_gic.c
index cf582a5..a29eacb 100644
--- a/hw/arm_gic.c
+++ b/hw/arm_gic.c
@@ -13,6 +13,8 @@
/* Maximum number of possible interrupts, determined by the GIC architecture
*/
#define GIC_MAXIRQ 1020
+/* First 32 are private to each CPU (SGIs and PPIs). */
+#define GIC_INTERNAL 32
//#define DEBUG_GIC
#ifdef DEBUG_GIC
@@ -74,7 +76,7 @@ typedef struct gic_irq_state
#define GIC_CLEAR_TRIGGER(irq) s-irq_state[irq].trigger = 0
#define GIC_TEST_TRIGGER(irq) s-irq_state[irq].trigger
#define GIC_GET_PRIORITY(irq, cpu) \
- (((irq) 32) ? s-priority1[irq][cpu] : s-priority2[(irq) - 32])
+ (((irq) GIC_INTERNAL) ? s-priority1[irq][cpu] : s-priority2[(irq) -
GIC_INTERNAL])
This line is now 80 characters and needs folding.
(scripts/checkpatch.pl will catch this kind of nit.)
@@ -812,11 +814,13 @@ static void gic_init(gic_state *s, int num_irq)
s-num_cpu = num_cpu;
#endif
s-num_irq = num_irq + GIC_BASE_IRQ;
- if (s-num_irq GIC_MAXIRQ) {
- hw_error(requested %u interrupt lines exceeds GIC maximum %d\n,
- num_irq, GIC_MAXIRQ);
+ if (s-num_irq GIC_MAXIRQ
+ || s-num_irq GIC_INTERNAL
+ || (s-num_irq % 32) != 0) {
So I guess our implementation isn't likely to work properly for a
non-multiple-of-32 number of IRQs, but this isn't an architectural GIC
restriction. (In fact the GIC architecture spec allows the supported
interrupt IDs to not even be in a contiguous range, which we certainly
don't support...)
I also think it's architecturally permitted that not all the internal
(SPI/PPI) interrupts are implemented, ie that s-num_irq 32 (you
have to read the GIC architecture manually quite closely to deduce
this, though).
Anyway, if we would otherwise die horribly later on we should
catch these cases, but it would be good to have at least a comment
saying that these are implementation limitations rather than
architectural ones.
Beefing up the parameter check should be a separate patch.
Otherwise OK.
-- PMM
Re: [Qemu-devel] [Android-virt] [PATCH 02/12] arm: make the number of GIC interrupts configurable
On Fri, 13 Jan 2012 20:52:39 +, Peter Maydell peter.mayd...@linaro.org
wrote:
From: Mark Langsdorf mark.langsd...@calxeda.com
Increase the maximum number of GIC interrupts for a9mp and a11mp to 1020,
and create a configurable property for each defaulting to 96 and 64
(respectively) so that device modelers can set the value appropriately
for their SoC. Other ARM processors also set their maximum number of
used IRQs appropriately.
Set the maximum theoretical number of GIC interrupts to 1020 and
update the save/restore code to only use the appropriate number for
each SoC.
Reading through this, I see a lot of - 32. Trivial patch follows,
which applies to your rebasing branch:
Subject: ARM: clean up GIC constants.
From: Rusty Russell ru...@rustcorp.com.au
Interrupts numbers 0-31 are private to the processor interface, 32-1019 are
general interrups. Add GIC_INTERNAL and substitute everywhere.
Also, add a check that the total number of interrupts is divisible by
32 (required for reporting interupt numbers, see gic_dist_readb(), and
is greater than 32. And remove a single stray tab.
Signed-off-by: Rusty Russell ru...@rustcorp.com.au
---
hw/arm_gic.c | 48 ++--
1 files changed, 26 insertions(+), 22 deletions(-)
diff --git a/hw/arm_gic.c b/hw/arm_gic.c
index cf582a5..a29eacb 100644
--- a/hw/arm_gic.c
+++ b/hw/arm_gic.c
@@ -13,6 +13,8 @@
/* Maximum number of possible interrupts, determined by the GIC architecture */
#define GIC_MAXIRQ 1020
+/* First 32 are private to each CPU (SGIs and PPIs). */
+#define GIC_INTERNAL 32
//#define DEBUG_GIC
#ifdef DEBUG_GIC
@@ -74,7 +76,7 @@ typedef struct gic_irq_state
#define GIC_CLEAR_TRIGGER(irq) s-irq_state[irq].trigger = 0
#define GIC_TEST_TRIGGER(irq) s-irq_state[irq].trigger
#define GIC_GET_PRIORITY(irq, cpu) \
- (((irq) 32) ? s-priority1[irq][cpu] : s-priority2[(irq) - 32])
+ (((irq) GIC_INTERNAL) ? s-priority1[irq][cpu] : s-priority2[(irq) -
GIC_INTERNAL])
#ifdef NVIC
#define GIC_TARGET(irq) 1
#else
@@ -92,8 +94,8 @@ typedef struct gic_state
#ifndef NVIC
int irq_target[GIC_MAXIRQ];
#endif
-int priority1[32][NCPU];
-int priority2[GIC_MAXIRQ - 32];
+int priority1[GIC_INTERNAL][NCPU];
+int priority2[GIC_MAXIRQ - GIC_INTERNAL];
int last_active[GIC_MAXIRQ][NCPU];
int priority_mask[NCPU];
@@ -131,7 +133,7 @@ static void gic_update(gic_state *s)
cm = 1 cpu;
s-current_pending[cpu] = 1023;
if (!s-enabled || !s-cpu_enabled[cpu]) {
- qemu_irq_lower(s-parent_irq[cpu]);
+qemu_irq_lower(s-parent_irq[cpu]);
return;
}
best_prio = 0x100;
@@ -174,7 +176,7 @@ static void gic_set_irq(void *opaque, int irq, int level)
{
gic_state *s = (gic_state *)opaque;
/* The first external input line is internal interrupt 32. */
-irq += 32;
+irq += GIC_INTERNAL;
if (level == GIC_TEST_LEVEL(irq, ALL_CPU_MASK))
return;
@@ -316,7 +318,7 @@ static uint32_t gic_dist_readb(void *opaque,
target_phys_addr_t offset)
if (irq = s-num_irq)
goto bad_reg;
res = 0;
-mask = (irq 32) ? cm : ALL_CPU_MASK;
+mask = (irq GIC_INTERNAL) ? cm : ALL_CPU_MASK;
for (i = 0; i 8; i++) {
if (GIC_TEST_PENDING(irq + i, mask)) {
res |= (1 i);
@@ -328,7 +330,7 @@ static uint32_t gic_dist_readb(void *opaque,
target_phys_addr_t offset)
if (irq = s-num_irq)
goto bad_reg;
res = 0;
-mask = (irq 32) ? cm : ALL_CPU_MASK;
+mask = (irq GIC_INTERNAL) ? cm : ALL_CPU_MASK;
for (i = 0; i 8; i++) {
if (GIC_TEST_ACTIVE(irq + i, mask)) {
res |= (1 i);
@@ -435,8 +437,8 @@ static void gic_dist_writeb(void *opaque,
target_phys_addr_t offset,
value = 0xff;
for (i = 0; i 8; i++) {
if (value (1 i)) {
-int mask = (irq 32) ? (1 cpu) : GIC_TARGET(irq);
-int cm = (irq 32) ? (1 cpu) : ALL_CPU_MASK;
+int mask = (irq GIC_INTERNAL) ? (1 cpu) : GIC_TARGET(irq);
+int cm = (irq GIC_INTERNAL) ? (1 cpu) : ALL_CPU_MASK;
if (!GIC_TEST_ENABLED(irq + i, cm)) {
DPRINTF(Enabled IRQ %d\n, irq + i);
@@ -460,7 +462,7 @@ static void gic_dist_writeb(void *opaque,
target_phys_addr_t offset,
value = 0;
for (i = 0; i 8; i++) {
if (value (1 i)) {
-int cm = (irq 32) ? (1 cpu) : ALL_CPU_MASK;
+int cm = (irq GIC_INTERNAL) ? (1 cpu) : ALL_CPU_MASK;
if (GIC_TEST_ENABLED(irq + i, cm)) {
DPRINTF(Disabled IRQ %d\n, irq + i);
@@ -502,10 +504,10 @@ static void gic_dist_writeb(void *opaque,
target_phys_addr_t offset,
irq = (offset - 0x400) + GIC_BASE_IRQ;
if (irq = s-num_irq)
goto bad_reg;
