* [PATCH 0/1] hw: aspeed_gpio: Fix GPIO array indexing
@ 2021-09-28 3:43 pdel
2021-09-28 3:43 ` [PATCH 1/1] " pdel
2021-09-30 0:46 ` [PATCH 0/1] " Peter Delevoryas
0 siblings, 2 replies; 10+ messages in thread
From: pdel @ 2021-09-28 3:43 UTC (permalink / raw)
Cc: clg, joel, rashmica.g, patrick, qemu-devel, f4bug, Peter Delevoryas
From: Peter Delevoryas <pdel@fb.com>
Hey everyone,
I think there might be a bug in aspeed_gpio_update, where it's selecting
a GPIO IRQ to update. The indexing that maps from GPIO pin to IRQ leads
to an out-of-bounds array access and a segfault after that.
tl;dr
There's 8 rows of 32 pins (8 * 32 == 256 total) on the AST2500, but some
of the pins are not actually active: there's only 228 pins actually
active in the AST2500.
The GPIO IRQ array has length 228, but we index it using a matrix
indexing scheme like [row][column], and end up out-of-bounds for
high-numbered pins.
I fixed this by converting the IRQ array to a matrix, where some
of the entries are uninitialized (zero). This retains the matrix
indexing scheme, which I think is easy to understand.
Notes on reproducing:
I was testing booting Facebook's OpenBMC platform "YosemiteV2" (fby2)
and hit a segfault:
qemu-system-arm -machine ast2500-evb \
-drive file=fby2.mtd,format=raw,if=mtd \
-serial stdio -display none
...
Setup Caching for Bridge IC info..done.
Setup Front Panel Daemon..done.
Setup fan speed...
FAN CONFIG : Single Rotor FAN
Unexpected 4 Servers config! Run FSC 4 TLs Config as default config
Setting Zone 0 speed to 70%
Setting Zone 1 speed to 70%
ok: run: fscd: (pid 1726) 0s
done.
Powering fru 1 to ON state...
Segmentation fault (core dumped)
In gdb:
Thread 3 "qemu-system-arm" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff20ee700 (LWP 1840353)]
qemu_set_irq (irq=0xffffffff00000000, level=1) at ../hw/core/irq.c:45
45 irq->handler(irq->opaque, irq->n, level);
(gdb) p irq
$1 = (qemu_irq) 0xffffffff00000000
(gdb) up
#1 0x00005555558e36f5 in aspeed_gpio_update (s=0x7ffff7ecffb0, regs=0x7ffff7ed0c94, value=128) at ../hw/gpio/aspeed_gpio.c:287
287 qemu_set_irq(s->gpios[offset], !!(new & mask));
(gdb) p s->gpios
$2 = {0x0 <repeats 228 times>}
(gdb) p offset
$3 = 231
(gdb) p set
$5 = 7
(gdb) p gpio
$4 = 7
With my fix, I can boot the fby2 platform. The image I was using is here:
https://github.com/peterdelevoryas/openbmc/releases/tag/fby2.debug.mtd
Peter Delevoryas (1):
hw: aspeed_gpio: Fix GPIO array indexing
hw/gpio/aspeed_gpio.c | 72 ++++++++++++++---------------------
include/hw/gpio/aspeed_gpio.h | 5 +--
2 files changed, 31 insertions(+), 46 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] hw: aspeed_gpio: Fix GPIO array indexing
2021-09-28 3:43 [PATCH 0/1] hw: aspeed_gpio: Fix GPIO array indexing pdel
@ 2021-09-28 3:43 ` pdel
2021-10-04 9:07 ` Cédric Le Goater
2021-09-30 0:46 ` [PATCH 0/1] " Peter Delevoryas
1 sibling, 1 reply; 10+ messages in thread
From: pdel @ 2021-09-28 3:43 UTC (permalink / raw)
Cc: clg, joel, rashmica.g, patrick, qemu-devel, f4bug, Peter Delevoryas
From: Peter Delevoryas <pdel@fb.com>
The gpio array is declared as a dense array:
qemu_irq gpios[ASPEED_GPIO_NR_PINS];
(AST2500 has 228, AST2400 has 216, AST2600 has 208)
However, this array is used like a matrix of GPIO sets
(e.g. gpio[NR_SETS][NR_PINS_PER_SET] = gpio[8][32])
size_t offset = set * GPIOS_PER_SET + gpio;
qemu_set_irq(s->gpios[offset], !!(new & mask));
This can result in an out-of-bounds access to "s->gpios" because the
gpio sets do _not_ have the same length. Some of the groups (e.g.
GPIOAB) only have 4 pins. 228 != 8 * 32 == 256.
To fix this, I converted the gpio array from dense to sparse, to match
both the hardware layout and this existing indexing code.
Fixes: 4b7f956862dc2db4c5c ("hw/gpio: Add basic Aspeed GPIO model for AST2400 and AST2500")
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
hw/gpio/aspeed_gpio.c | 72 ++++++++++++++---------------------
include/hw/gpio/aspeed_gpio.h | 5 +--
2 files changed, 31 insertions(+), 46 deletions(-)
diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index dfa6d6cb40..f04d4a454c 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -16,11 +16,7 @@
#include "hw/irq.h"
#include "migration/vmstate.h"
-#define GPIOS_PER_REG 32
-#define GPIOS_PER_SET GPIOS_PER_REG
-#define GPIO_PIN_GAP_SIZE 4
#define GPIOS_PER_GROUP 8
-#define GPIO_GROUP_SHIFT 3
/* GPIO Source Types */
#define ASPEED_CMD_SRC_MASK 0x01010101
@@ -259,7 +255,7 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
diff = old ^ new;
if (diff) {
- for (gpio = 0; gpio < GPIOS_PER_REG; gpio++) {
+ for (gpio = 0; gpio < ASPEED_GPIOS_PER_SET; gpio++) {
uint32_t mask = 1 << gpio;
/* If the gpio needs to be updated... */
@@ -283,8 +279,7 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
if (direction & mask) {
/* ...trigger the line-state IRQ */
ptrdiff_t set = aspeed_gpio_set_idx(s, regs);
- size_t offset = set * GPIOS_PER_SET + gpio;
- qemu_set_irq(s->gpios[offset], !!(new & mask));
+ qemu_set_irq(s->gpios[set][gpio], !!(new & mask));
} else {
/* ...otherwise if we meet the line's current IRQ policy... */
if (aspeed_evaluate_irq(regs, old & mask, gpio)) {
@@ -297,21 +292,6 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
qemu_set_irq(s->irq, !!(s->pending));
}
-static uint32_t aspeed_adjust_pin(AspeedGPIOState *s, uint32_t pin)
-{
- AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
- /*
- * The 2500 has a 4 pin gap in group AB and the 2400 has a 4 pin
- * gap in group Y (and only four pins in AB but this is the last group so
- * it doesn't matter).
- */
- if (agc->gap && pin >= agc->gap) {
- pin += GPIO_PIN_GAP_SIZE;
- }
-
- return pin;
-}
-
static bool aspeed_gpio_get_pin_level(AspeedGPIOState *s, uint32_t set_idx,
uint32_t pin)
{
@@ -367,7 +347,7 @@ static uint32_t update_value_control_source(GPIOSets *regs, uint32_t old_value,
uint32_t new_value = 0;
/* for each group in set */
- for (i = 0; i < GPIOS_PER_REG; i += GPIOS_PER_GROUP) {
+ for (i = 0; i < ASPEED_GPIOS_PER_SET; i += GPIOS_PER_GROUP) {
cmd_source = extract32(regs->cmd_source_0, i, 1)
| (extract32(regs->cmd_source_1, i, 1) << 1);
@@ -637,7 +617,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t data,
* bidirectional | 1 | 1 | data
* input only | 1 | 0 | 0
* output only | 0 | 1 | 1
- * no pin / gap | 0 | 0 | 0
+ * no pin | 0 | 0 | 0
*
* which is captured by:
* data = ( data | ~input) & output;
@@ -836,14 +816,20 @@ static void aspeed_gpio_realize(DeviceState *dev, Error **errp)
AspeedGPIOState *s = ASPEED_GPIO(dev);
SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
- int pin;
/* Interrupt parent line */
sysbus_init_irq(sbd, &s->irq);
/* Individual GPIOs */
- for (pin = 0; pin < agc->nr_gpio_pins; pin++) {
- sysbus_init_irq(sbd, &s->gpios[pin]);
+ for (int i = 0; i < ASPEED_GPIO_MAX_NR_SETS; i++) {
+ const GPIOSetProperties *props = &agc->props[i];
+ uint32_t skip = ~(props->input | props->output);
+ for (int j = 0; j < ASPEED_GPIOS_PER_SET; j++) {
+ if (skip >> j & 1) {
+ continue;
+ }
+ sysbus_init_irq(sbd, &s->gpios[i][j]);
+ }
}
memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_gpio_ops, s,
@@ -856,20 +842,22 @@ static void aspeed_gpio_init(Object *obj)
{
AspeedGPIOState *s = ASPEED_GPIO(obj);
AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
- int pin;
-
- for (pin = 0; pin < agc->nr_gpio_pins; pin++) {
- char *name;
- int set_idx = pin / GPIOS_PER_SET;
- int pin_idx = aspeed_adjust_pin(s, pin) - (set_idx * GPIOS_PER_SET);
- int group_idx = pin_idx >> GPIO_GROUP_SHIFT;
- const GPIOSetProperties *props = &agc->props[set_idx];
-
- name = g_strdup_printf("gpio%s%d", props->group_label[group_idx],
- pin_idx % GPIOS_PER_GROUP);
- object_property_add(obj, name, "bool", aspeed_gpio_get_pin,
- aspeed_gpio_set_pin, NULL, NULL);
- g_free(name);
+
+ for (int i = 0; i < ASPEED_GPIO_MAX_NR_SETS; i++) {
+ const GPIOSetProperties *props = &agc->props[i];
+ uint32_t skip = ~(props->input | props->output);
+ for (int j = 0; j < ASPEED_GPIOS_PER_SET; j++) {
+ if (skip >> j & 1) {
+ continue;
+ }
+ int group_idx = j / GPIOS_PER_GROUP;
+ int pin_idx = j % GPIOS_PER_GROUP;
+ const char *group = &props->group_label[group_idx][0];
+ char *name = g_strdup_printf("gpio%s%d", group, pin_idx);
+ object_property_add(obj, name, "bool", aspeed_gpio_get_pin,
+ aspeed_gpio_set_pin, NULL, NULL);
+ g_free(name);
+ }
}
}
@@ -926,7 +914,6 @@ static void aspeed_gpio_ast2400_class_init(ObjectClass *klass, void *data)
agc->props = ast2400_set_props;
agc->nr_gpio_pins = 216;
agc->nr_gpio_sets = 7;
- agc->gap = 196;
agc->reg_table = aspeed_3_3v_gpios;
}
@@ -937,7 +924,6 @@ static void aspeed_gpio_2500_class_init(ObjectClass *klass, void *data)
agc->props = ast2500_set_props;
agc->nr_gpio_pins = 228;
agc->nr_gpio_sets = 8;
- agc->gap = 220;
agc->reg_table = aspeed_3_3v_gpios;
}
diff --git a/include/hw/gpio/aspeed_gpio.h b/include/hw/gpio/aspeed_gpio.h
index e1636ce7fe..801846befb 100644
--- a/include/hw/gpio/aspeed_gpio.h
+++ b/include/hw/gpio/aspeed_gpio.h
@@ -17,9 +17,9 @@
OBJECT_DECLARE_TYPE(AspeedGPIOState, AspeedGPIOClass, ASPEED_GPIO)
#define ASPEED_GPIO_MAX_NR_SETS 8
+#define ASPEED_GPIOS_PER_SET 32
#define ASPEED_REGS_PER_BANK 14
#define ASPEED_GPIO_MAX_NR_REGS (ASPEED_REGS_PER_BANK * ASPEED_GPIO_MAX_NR_SETS)
-#define ASPEED_GPIO_NR_PINS 228
#define ASPEED_GROUPS_PER_SET 4
#define ASPEED_GPIO_NR_DEBOUNCE_REGS 3
#define ASPEED_CHARS_PER_GROUP_LABEL 4
@@ -60,7 +60,6 @@ struct AspeedGPIOClass {
const GPIOSetProperties *props;
uint32_t nr_gpio_pins;
uint32_t nr_gpio_sets;
- uint32_t gap;
const AspeedGPIOReg *reg_table;
};
@@ -72,7 +71,7 @@ struct AspeedGPIOState {
MemoryRegion iomem;
int pending;
qemu_irq irq;
- qemu_irq gpios[ASPEED_GPIO_NR_PINS];
+ qemu_irq gpios[ASPEED_GPIO_MAX_NR_SETS][ASPEED_GPIOS_PER_SET];
/* Parallel GPIO Registers */
uint32_t debounce_regs[ASPEED_GPIO_NR_DEBOUNCE_REGS];
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/1] hw: aspeed_gpio: Fix GPIO array indexing
2021-09-28 3:43 [PATCH 0/1] hw: aspeed_gpio: Fix GPIO array indexing pdel
2021-09-28 3:43 ` [PATCH 1/1] " pdel
@ 2021-09-30 0:46 ` Peter Delevoryas
1 sibling, 0 replies; 10+ messages in thread
From: Peter Delevoryas @ 2021-09-30 0:46 UTC (permalink / raw)
Cc: Cédric Le Goater, Joel Stanley, rashmica.g, patrick,
Cameron Esfahani via, Philippe Mathieu-Daudé,
Dan Zhang
> On Sep 27, 2021, at 8:43 PM, pdel@fb.com wrote:
>
> From: Peter Delevoryas <pdel@fb.com>
>
> Hey everyone,
>
> I think there might be a bug in aspeed_gpio_update, where it's selecting
> a GPIO IRQ to update. The indexing that maps from GPIO pin to IRQ leads
> to an out-of-bounds array access and a segfault after that.
>
> tl;dr
>
> There's 8 rows of 32 pins (8 * 32 == 256 total) on the AST2500, but some
> of the pins are not actually active: there's only 228 pins actually
> active in the AST2500.
>
> The GPIO IRQ array has length 228, but we index it using a matrix
> indexing scheme like [row][column], and end up out-of-bounds for
> high-numbered pins.
>
> I fixed this by converting the IRQ array to a matrix, where some
> of the entries are uninitialized (zero). This retains the matrix
> indexing scheme, which I think is easy to understand.
>
> Notes on reproducing:
>
> I was testing booting Facebook's OpenBMC platform "YosemiteV2" (fby2)
> and hit a segfault:
>
> qemu-system-arm -machine ast2500-evb \
> -drive file=fby2.mtd,format=raw,if=mtd \
> -serial stdio -display none
> ...
> Setup Caching for Bridge IC info..done.
> Setup Front Panel Daemon..done.
> Setup fan speed...
> FAN CONFIG : Single Rotor FAN
> Unexpected 4 Servers config! Run FSC 4 TLs Config as default config
> Setting Zone 0 speed to 70%
> Setting Zone 1 speed to 70%
> ok: run: fscd: (pid 1726) 0s
> done.
> Powering fru 1 to ON state...
> Segmentation fault (core dumped)
>
> In gdb:
>
> Thread 3 "qemu-system-arm" received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7ffff20ee700 (LWP 1840353)]
> qemu_set_irq (irq=0xffffffff00000000, level=1) at ../hw/core/irq.c:45
> 45 irq->handler(irq->opaque, irq->n, level);
> (gdb) p irq
> $1 = (qemu_irq) 0xffffffff00000000
> (gdb) up
> #1 0x00005555558e36f5 in aspeed_gpio_update (s=0x7ffff7ecffb0, regs=0x7ffff7ed0c94, value=128) at ../hw/gpio/aspeed_gpio.c:287
> 287 qemu_set_irq(s->gpios[offset], !!(new & mask));
> (gdb) p s->gpios
> $2 = {0x0 <repeats 228 times>}
> (gdb) p offset
> $3 = 231
> (gdb) p set
> $5 = 7
> (gdb) p gpio
> $4 = 7
>
> With my fix, I can boot the fby2 platform. The image I was using is here:
>
> https://github.com/peterdelevoryas/openbmc/releases/tag/fby2.debug.mtd
>
> Peter Delevoryas (1):
> hw: aspeed_gpio: Fix GPIO array indexing
>
> hw/gpio/aspeed_gpio.c | 72 ++++++++++++++---------------------
> include/hw/gpio/aspeed_gpio.h | 5 +--
> 2 files changed, 31 insertions(+), 46 deletions(-)
>
> --
> 2.30.2
>
cc’ing Dan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] hw: aspeed_gpio: Fix GPIO array indexing
2021-09-28 3:43 ` [PATCH 1/1] " pdel
@ 2021-10-04 9:07 ` Cédric Le Goater
2021-10-04 11:43 ` Cédric Le Goater
0 siblings, 1 reply; 10+ messages in thread
From: Cédric Le Goater @ 2021-10-04 9:07 UTC (permalink / raw)
To: pdel; +Cc: patrick, rashmica.g, f4bug, joel, qemu-devel
On 9/28/21 05:43, pdel@fb.com wrote:
> From: Peter Delevoryas <pdel@fb.com>
>
> The gpio array is declared as a dense array:
>
> qemu_irq gpios[ASPEED_GPIO_NR_PINS];
>
> (AST2500 has 228, AST2400 has 216, AST2600 has 208)
>
> However, this array is used like a matrix of GPIO sets
> (e.g. gpio[NR_SETS][NR_PINS_PER_SET] = gpio[8][32])
>
> size_t offset = set * GPIOS_PER_SET + gpio;
> qemu_set_irq(s->gpios[offset], !!(new & mask));
>
> This can result in an out-of-bounds access to "s->gpios" because the
> gpio sets do _not_ have the same length. Some of the groups (e.g.
> GPIOAB) only have 4 pins. 228 != 8 * 32 == 256.
>
> To fix this, I converted the gpio array from dense to sparse, to match
> both the hardware layout and this existing indexing code.
>
> Fixes: 4b7f956862dc2db4c5c ("hw/gpio: Add basic Aspeed GPIO model for AST2400 and AST2500")
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
This patch is raising an error in qtest-arm/qom-test when compiled
with clang :
Running test qtest-arm/qom-test
Unexpected error in object_property_try_add() at ../qom/object.c:1224:
qemu-system-arm: attempt to add duplicate property 'gpio0' to object (type 'aspeed.gpio-ast2600-1_8v')
Broken pipe
ERROR qtest-arm/qom-test - too few tests run (expected 78, got 0)
make: *** [Makefile.mtest:24: run-test-1] Error 1
Thanks,
C.
> ---
> hw/gpio/aspeed_gpio.c | 72 ++++++++++++++---------------------
> include/hw/gpio/aspeed_gpio.h | 5 +--
> 2 files changed, 31 insertions(+), 46 deletions(-)
>
> diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> index dfa6d6cb40..f04d4a454c 100644
> --- a/hw/gpio/aspeed_gpio.c
> +++ b/hw/gpio/aspeed_gpio.c
> @@ -16,11 +16,7 @@
> #include "hw/irq.h"
> #include "migration/vmstate.h"
>
> -#define GPIOS_PER_REG 32
> -#define GPIOS_PER_SET GPIOS_PER_REG
> -#define GPIO_PIN_GAP_SIZE 4
> #define GPIOS_PER_GROUP 8
> -#define GPIO_GROUP_SHIFT 3
>
> /* GPIO Source Types */
> #define ASPEED_CMD_SRC_MASK 0x01010101
> @@ -259,7 +255,7 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
>
> diff = old ^ new;
> if (diff) {
> - for (gpio = 0; gpio < GPIOS_PER_REG; gpio++) {
> + for (gpio = 0; gpio < ASPEED_GPIOS_PER_SET; gpio++) {
> uint32_t mask = 1 << gpio;
>
> /* If the gpio needs to be updated... */
> @@ -283,8 +279,7 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
> if (direction & mask) {
> /* ...trigger the line-state IRQ */
> ptrdiff_t set = aspeed_gpio_set_idx(s, regs);
> - size_t offset = set * GPIOS_PER_SET + gpio;
> - qemu_set_irq(s->gpios[offset], !!(new & mask));
> + qemu_set_irq(s->gpios[set][gpio], !!(new & mask));
> } else {
> /* ...otherwise if we meet the line's current IRQ policy... */
> if (aspeed_evaluate_irq(regs, old & mask, gpio)) {
> @@ -297,21 +292,6 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
> qemu_set_irq(s->irq, !!(s->pending));
> }
>
> -static uint32_t aspeed_adjust_pin(AspeedGPIOState *s, uint32_t pin)
> -{
> - AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
> - /*
> - * The 2500 has a 4 pin gap in group AB and the 2400 has a 4 pin
> - * gap in group Y (and only four pins in AB but this is the last group so
> - * it doesn't matter).
> - */
> - if (agc->gap && pin >= agc->gap) {
> - pin += GPIO_PIN_GAP_SIZE;
> - }
> -
> - return pin;
> -}
> -
> static bool aspeed_gpio_get_pin_level(AspeedGPIOState *s, uint32_t set_idx,
> uint32_t pin)
> {
> @@ -367,7 +347,7 @@ static uint32_t update_value_control_source(GPIOSets *regs, uint32_t old_value,
> uint32_t new_value = 0;
>
> /* for each group in set */
> - for (i = 0; i < GPIOS_PER_REG; i += GPIOS_PER_GROUP) {
> + for (i = 0; i < ASPEED_GPIOS_PER_SET; i += GPIOS_PER_GROUP) {
> cmd_source = extract32(regs->cmd_source_0, i, 1)
> | (extract32(regs->cmd_source_1, i, 1) << 1);
>
> @@ -637,7 +617,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t data,
> * bidirectional | 1 | 1 | data
> * input only | 1 | 0 | 0
> * output only | 0 | 1 | 1
> - * no pin / gap | 0 | 0 | 0
> + * no pin | 0 | 0 | 0
> *
> * which is captured by:
> * data = ( data | ~input) & output;
> @@ -836,14 +816,20 @@ static void aspeed_gpio_realize(DeviceState *dev, Error **errp)
> AspeedGPIOState *s = ASPEED_GPIO(dev);
> SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
> - int pin;
>
> /* Interrupt parent line */
> sysbus_init_irq(sbd, &s->irq);
>
> /* Individual GPIOs */
> - for (pin = 0; pin < agc->nr_gpio_pins; pin++) {
> - sysbus_init_irq(sbd, &s->gpios[pin]);
> + for (int i = 0; i < ASPEED_GPIO_MAX_NR_SETS; i++) {
> + const GPIOSetProperties *props = &agc->props[i];
> + uint32_t skip = ~(props->input | props->output);
> + for (int j = 0; j < ASPEED_GPIOS_PER_SET; j++) {
> + if (skip >> j & 1) {
> + continue;
> + }
> + sysbus_init_irq(sbd, &s->gpios[i][j]);
> + }
> }
>
> memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_gpio_ops, s,
> @@ -856,20 +842,22 @@ static void aspeed_gpio_init(Object *obj)
> {
> AspeedGPIOState *s = ASPEED_GPIO(obj);
> AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
> - int pin;
> -
> - for (pin = 0; pin < agc->nr_gpio_pins; pin++) {
> - char *name;
> - int set_idx = pin / GPIOS_PER_SET;
> - int pin_idx = aspeed_adjust_pin(s, pin) - (set_idx * GPIOS_PER_SET);
> - int group_idx = pin_idx >> GPIO_GROUP_SHIFT;
> - const GPIOSetProperties *props = &agc->props[set_idx];
> -
> - name = g_strdup_printf("gpio%s%d", props->group_label[group_idx],
> - pin_idx % GPIOS_PER_GROUP);
> - object_property_add(obj, name, "bool", aspeed_gpio_get_pin,
> - aspeed_gpio_set_pin, NULL, NULL);
> - g_free(name);
> +
> + for (int i = 0; i < ASPEED_GPIO_MAX_NR_SETS; i++) {
> + const GPIOSetProperties *props = &agc->props[i];
> + uint32_t skip = ~(props->input | props->output);
> + for (int j = 0; j < ASPEED_GPIOS_PER_SET; j++) {
> + if (skip >> j & 1) {
> + continue;
> + }
> + int group_idx = j / GPIOS_PER_GROUP;
> + int pin_idx = j % GPIOS_PER_GROUP;
> + const char *group = &props->group_label[group_idx][0];
> + char *name = g_strdup_printf("gpio%s%d", group, pin_idx);
> + object_property_add(obj, name, "bool", aspeed_gpio_get_pin,
> + aspeed_gpio_set_pin, NULL, NULL);
> + g_free(name);
> + }
> }
> }
>
> @@ -926,7 +914,6 @@ static void aspeed_gpio_ast2400_class_init(ObjectClass *klass, void *data)
> agc->props = ast2400_set_props;
> agc->nr_gpio_pins = 216;
> agc->nr_gpio_sets = 7;
> - agc->gap = 196;
> agc->reg_table = aspeed_3_3v_gpios;
> }
>
> @@ -937,7 +924,6 @@ static void aspeed_gpio_2500_class_init(ObjectClass *klass, void *data)
> agc->props = ast2500_set_props;
> agc->nr_gpio_pins = 228;
> agc->nr_gpio_sets = 8;
> - agc->gap = 220;
> agc->reg_table = aspeed_3_3v_gpios;
> }
>
> diff --git a/include/hw/gpio/aspeed_gpio.h b/include/hw/gpio/aspeed_gpio.h
> index e1636ce7fe..801846befb 100644
> --- a/include/hw/gpio/aspeed_gpio.h
> +++ b/include/hw/gpio/aspeed_gpio.h
> @@ -17,9 +17,9 @@
> OBJECT_DECLARE_TYPE(AspeedGPIOState, AspeedGPIOClass, ASPEED_GPIO)
>
> #define ASPEED_GPIO_MAX_NR_SETS 8
> +#define ASPEED_GPIOS_PER_SET 32
> #define ASPEED_REGS_PER_BANK 14
> #define ASPEED_GPIO_MAX_NR_REGS (ASPEED_REGS_PER_BANK * ASPEED_GPIO_MAX_NR_SETS)
> -#define ASPEED_GPIO_NR_PINS 228
> #define ASPEED_GROUPS_PER_SET 4
> #define ASPEED_GPIO_NR_DEBOUNCE_REGS 3
> #define ASPEED_CHARS_PER_GROUP_LABEL 4
> @@ -60,7 +60,6 @@ struct AspeedGPIOClass {
> const GPIOSetProperties *props;
> uint32_t nr_gpio_pins;
> uint32_t nr_gpio_sets;
> - uint32_t gap;
> const AspeedGPIOReg *reg_table;
> };
>
> @@ -72,7 +71,7 @@ struct AspeedGPIOState {
> MemoryRegion iomem;
> int pending;
> qemu_irq irq;
> - qemu_irq gpios[ASPEED_GPIO_NR_PINS];
> + qemu_irq gpios[ASPEED_GPIO_MAX_NR_SETS][ASPEED_GPIOS_PER_SET];
>
> /* Parallel GPIO Registers */
> uint32_t debounce_regs[ASPEED_GPIO_NR_DEBOUNCE_REGS];
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] hw: aspeed_gpio: Fix GPIO array indexing
2021-10-04 9:07 ` Cédric Le Goater
@ 2021-10-04 11:43 ` Cédric Le Goater
2021-10-08 3:19 ` Peter Delevoryas
0 siblings, 1 reply; 10+ messages in thread
From: Cédric Le Goater @ 2021-10-04 11:43 UTC (permalink / raw)
To: pdel; +Cc: patrick, rashmica.g, f4bug, joel, qemu-devel
On 10/4/21 11:07, Cédric Le Goater wrote:
> On 9/28/21 05:43, pdel@fb.com wrote:
>> From: Peter Delevoryas <pdel@fb.com>
>>
>> The gpio array is declared as a dense array:
>>
>> qemu_irq gpios[ASPEED_GPIO_NR_PINS];
>>
>> (AST2500 has 228, AST2400 has 216, AST2600 has 208)
>>
>> However, this array is used like a matrix of GPIO sets
>> (e.g. gpio[NR_SETS][NR_PINS_PER_SET] = gpio[8][32])
>>
>> size_t offset = set * GPIOS_PER_SET + gpio;
>> qemu_set_irq(s->gpios[offset], !!(new & mask));
>>
>> This can result in an out-of-bounds access to "s->gpios" because the
>> gpio sets do _not_ have the same length. Some of the groups (e.g.
>> GPIOAB) only have 4 pins. 228 != 8 * 32 == 256.
>>
>> To fix this, I converted the gpio array from dense to sparse, to match
>> both the hardware layout and this existing indexing code.
>>
>> Fixes: 4b7f956862dc2db4c5c ("hw/gpio: Add basic Aspeed GPIO model for AST2400 and AST2500")
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>
>
> This patch is raising an error in qtest-arm/qom-test when compiled
> with clang :
>
> Running test qtest-arm/qom-test
> Unexpected error in object_property_try_add() at ../qom/object.c:1224:
> qemu-system-arm: attempt to add duplicate property 'gpio0' to object (type 'aspeed.gpio-ast2600-1_8v')
> Broken pipe
> ERROR qtest-arm/qom-test - too few tests run (expected 78, got 0)
> make: *** [Makefile.mtest:24: run-test-1] Error 1
The GPIOSetProperties arrary is smaller for the ast2600_1_8v model :
static GPIOSetProperties ast2600_1_8v_set_props[] = {
[0] = {0xffffffff, 0xffffffff, {"18A", "18B", "18C", "18D"} },
[1] = {0x0000000f, 0x0000000f, {"18E"} },
};
and in aspeed_gpio_init() :
for (int i = 0; i < ASPEED_GPIO_MAX_NR_SETS; i++) {
we loop beyond.
To configure compilation with clang, use the configure option :
--cc=clang
and simply run 'make check-qtest-arm'
Thanks
C.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] hw: aspeed_gpio: Fix GPIO array indexing
2021-10-04 11:43 ` Cédric Le Goater
@ 2021-10-08 3:19 ` Peter Delevoryas
0 siblings, 0 replies; 10+ messages in thread
From: Peter Delevoryas @ 2021-10-08 3:19 UTC (permalink / raw)
Cc: Joel Stanley, rashmica.g, patrick, Cameron Esfahani via,
Philippe Mathieu-Daudé,
Cédric Le Goater
> On Oct 4, 2021, at 4:43 AM, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 10/4/21 11:07, Cédric Le Goater wrote:
>> On 9/28/21 05:43, pdel@fb.com wrote:
>>> From: Peter Delevoryas <pdel@fb.com>
>>>
>>> The gpio array is declared as a dense array:
>>>
>>> qemu_irq gpios[ASPEED_GPIO_NR_PINS];
>>>
>>> (AST2500 has 228, AST2400 has 216, AST2600 has 208)
>>>
>>> However, this array is used like a matrix of GPIO sets
>>> (e.g. gpio[NR_SETS][NR_PINS_PER_SET] = gpio[8][32])
>>>
>>> size_t offset = set * GPIOS_PER_SET + gpio;
>>> qemu_set_irq(s->gpios[offset], !!(new & mask));
>>>
>>> This can result in an out-of-bounds access to "s->gpios" because the
>>> gpio sets do _not_ have the same length. Some of the groups (e.g.
>>> GPIOAB) only have 4 pins. 228 != 8 * 32 == 256.
>>>
>>> To fix this, I converted the gpio array from dense to sparse, to match
>>> both the hardware layout and this existing indexing code.
>>>
>>> Fixes: 4b7f956862dc2db4c5c ("hw/gpio: Add basic Aspeed GPIO model for AST2400 and AST2500")
>>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>> This patch is raising an error in qtest-arm/qom-test when compiled
>> with clang :
>> Running test qtest-arm/qom-test
>> Unexpected error in object_property_try_add() at ../qom/object.c:1224:
>> qemu-system-arm: attempt to add duplicate property 'gpio0' to object (type 'aspeed.gpio-ast2600-1_8v')
>> Broken pipe
>> ERROR qtest-arm/qom-test - too few tests run (expected 78, got 0)
>> make: *** [Makefile.mtest:24: run-test-1] Error 1
>
> The GPIOSetProperties arrary is smaller for the ast2600_1_8v model :
>
> static GPIOSetProperties ast2600_1_8v_set_props[] = {
> [0] = {0xffffffff, 0xffffffff, {"18A", "18B", "18C", "18D"} },
> [1] = {0x0000000f, 0x0000000f, {"18E"} },
> };
>
> and in aspeed_gpio_init() :
>
> for (int i = 0; i < ASPEED_GPIO_MAX_NR_SETS; i++) {
>
> we loop beyond.
>
> To configure compilation with clang, use the configure option :
>
> --cc=clang
>
> and simply run 'make check-qtest-arm'
Thanks for pointing this out! To fix it, I think the simplest thing I could do
is just make sure all of the GPIOSetProperties arrays have the same length,
ASPEED_GPIO_MAX_NR_SETS. There is already a filtering mechanism in place in
the code that iterates over the properties to skip zeroed entries. Alternatively,
we could keep some variable length nr_sets value with each class, but I figured
that is more complicated and error-prone. I’m submitting the v2 version
with this fix.
Thanks,
Peter
diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index 911d21c8cf..78b0f64b03 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -759,7 +759,7 @@ static void aspeed_gpio_set_pin(Object *obj, Visitor *v, const char *name,
}
/****************** Setup functions ******************/
-static const GPIOSetProperties ast2400_set_props[ASPEED_GPIO_MAX_NR_SETS] = {
+static const GPIOSetProperties ast2400_set_props[] = {
[0] = {0xffffffff, 0xffffffff, {"A", "B", "C", "D"} },
[1] = {0xffffffff, 0xffffffff, {"E", "F", "G", "H"} },
[2] = {0xffffffff, 0xffffffff, {"I", "J", "K", "L"} },
@@ -769,7 +769,7 @@ static const GPIOSetProperties ast2400_set_props[ASPEED_GPIO_MAX_NR_SETS] = {
[6] = {0x0000000f, 0x0fffff0f, {"Y", "Z", "AA", "AB"} },
};
-static const GPIOSetProperties ast2500_set_props[ASPEED_GPIO_MAX_NR_SETS] = {
+static const GPIOSetProperties ast2500_set_props[] = {
[0] = {0xffffffff, 0xffffffff, {"A", "B", "C", "D"} },
[1] = {0xffffffff, 0xffffffff, {"E", "F", "G", "H"} },
[2] = {0xffffffff, 0xffffffff, {"I", "J", "K", "L"} },
@@ -780,7 +780,7 @@ static const GPIOSetProperties ast2500_set_props[ASPEED_GPIO_MAX_NR_SETS] = {
[7] = {0x000000ff, 0x000000ff, {"AC"} },
};
-static GPIOSetProperties ast2600_3_3v_set_props[ASPEED_GPIO_MAX_NR_SETS] = {
+static GPIOSetProperties ast2600_3_3v_set_props[] = {
[0] = {0xffffffff, 0xffffffff, {"A", "B", "C", "D"} },
[1] = {0xffffffff, 0xffffffff, {"E", "F", "G", "H"} },
[2] = {0xffffffff, 0xffffffff, {"I", "J", "K", "L"} },
@@ -790,7 +790,7 @@ static GPIOSetProperties ast2600_3_3v_set_props[ASPEED_GPIO_MAX_NR_SETS] = {
[6] = {0x0000ffff, 0x0000ffff, {"Y", "Z"} },
};
-static GPIOSetProperties ast2600_1_8v_set_props[ASPEED_GPIO_MAX_NR_SETS] = {
+static GPIOSetProperties ast2600_1_8v_set_props[] = {
[0] = {0xffffffff, 0xffffffff, {"18A", "18B", "18C", "18D"} },
[1] = {0x0000000f, 0x0000000f, {"18E"} },
};
>
> Thanks
>
> C.
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] hw: aspeed_gpio: Fix GPIO array indexing
2021-09-25 14:28 ` Peter Delevoryas
@ 2021-09-27 10:05 ` Cédric Le Goater
0 siblings, 0 replies; 10+ messages in thread
From: Cédric Le Goater @ 2021-09-27 10:05 UTC (permalink / raw)
To: Peter Delevoryas, Philippe Mathieu-Daudé
Cc: patrick, rashmica.g, joel, qemu-devel
On 9/25/21 16:28, Peter Delevoryas wrote:
>
>> On Sep 25, 2021, at 4:03 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Hi Peter,
>>
>>> On 9/24/21 08:19, pdel@fb.com wrote:
>>> From: Peter Delevoryas <pdel@fb.com>
>>> The gpio array is declared as a dense array:
>>> ...
>>> qemu_irq gpios[ASPEED_GPIO_NR_PINS];
>>> (AST2500 has 228, AST2400 has 216, AST2600 has 208)
>>> However, this array is used like a matrix of GPIO sets
>>> (e.g. gpio[NR_SETS][NR_PINS_PER_SET] = gpio[8][32])
>>> size_t offset = set * GPIOS_PER_SET + gpio;
>>> qemu_set_irq(s->gpios[offset], !!(new & mask));
>>> This can result in an out-of-bounds access to "s->gpios" because the
>>> gpio sets do _not_ have the same length. Some of the groups (e.g.
>>> GPIOAB) only have 4 pins. 228 != 8 * 32 == 256.
>>> To fix this, I converted the gpio array from dense to sparse, to that
>>> match both the hardware layout and this existing indexing code.
>>
>> This is one logical change: 1 patch
>>
>>> Also, I noticed that some of the property specifications looked wrong:
>>> the lower 8 bits in the input and output u32's correspond to the first
>>> group label, and the upper 8 bits correspond to the last group label.
>>
>> Another logical change: another patch :)
>>
>> So please split this patch in 2. Maybe easier to fix GPIOSetProperties
>> first, then convert from dense to sparse array.
>>
>
> Good point, I’ll split it up then!
Yes. We can surely merge the GPIOSetProperties patch quickly.
I hope Rashmica has some time to check the second part.
Thanks,
C.
>
>> Regards,
>>
>> Phil.
>>
>>> I looked at the datasheet and several of these declarations seemed
>>> incorrect to me (I was basing it off of the I/O column). If somebody
>>> can double-check this, I'd really appreciate it!
>>> Some were definitely wrong though, like "Y" and "Z" in the 2600.
>>> @@ -796,7 +776,7 @@ static const GPIOSetProperties ast2500_set_props[] = {
>>> [3] = {0xffffffff, 0xffffffff, {"M", "N", "O", "P"} },
>>> [4] = {0xffffffff, 0xffffffff, {"Q", "R", "S", "T"} },
>>> [5] = {0xffffffff, 0x0000ffff, {"U", "V", "W", "X"} },
>>> - [6] = {0xffffff0f, 0x0fffff0f, {"Y", "Z", "AA", "AB"} },
>>> + [6] = {0x0fffffff, 0x0fffffff, {"Y", "Z", "AA", "AB"} },
>>> [7] = {0x000000ff, 0x000000ff, {"AC"} },
>>> };
>>> @@ -805,9 +785,9 @@ static GPIOSetProperties ast2600_3_3v_set_props[] = {
>>> [1] = {0xffffffff, 0xffffffff, {"E", "F", "G", "H"} },
>>> [2] = {0xffffffff, 0xffffffff, {"I", "J", "K", "L"} },
>>> [3] = {0xffffffff, 0xffffffff, {"M", "N", "O", "P"} },
>>> - [4] = {0xffffffff, 0xffffffff, {"Q", "R", "S", "T"} },
>>> - [5] = {0xffffffff, 0x0000ffff, {"U", "V", "W", "X"} },
>>> - [6] = {0xffff0000, 0x0fff0000, {"Y", "Z", "", ""} },
>>> + [4] = {0xffffffff, 0x00ffffff, {"Q", "R", "S", "T"} },
>>> + [5] = {0xffffffff, 0xffffff00, {"U", "V", "W", "X"} },
>>> + [6] = {0x0000ffff, 0x0000ffff, {"Y", "Z", "", ""} },
>>> };
>>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>>> ---
>>> hw/gpio/aspeed_gpio.c | 80 +++++++++++++++--------------------
>>> include/hw/gpio/aspeed_gpio.h | 5 +--
>>> 2 files changed, 35 insertions(+), 50 deletions(-)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] hw: aspeed_gpio: Fix GPIO array indexing
2021-09-25 11:02 ` Philippe Mathieu-Daudé
@ 2021-09-25 14:28 ` Peter Delevoryas
2021-09-27 10:05 ` Cédric Le Goater
0 siblings, 1 reply; 10+ messages in thread
From: Peter Delevoryas @ 2021-09-25 14:28 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: clg, joel, rashmica.g, patrick, qemu-devel
> On Sep 25, 2021, at 4:03 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Peter,
>
>> On 9/24/21 08:19, pdel@fb.com wrote:
>> From: Peter Delevoryas <pdel@fb.com>
>> The gpio array is declared as a dense array:
>> ...
>> qemu_irq gpios[ASPEED_GPIO_NR_PINS];
>> (AST2500 has 228, AST2400 has 216, AST2600 has 208)
>> However, this array is used like a matrix of GPIO sets
>> (e.g. gpio[NR_SETS][NR_PINS_PER_SET] = gpio[8][32])
>> size_t offset = set * GPIOS_PER_SET + gpio;
>> qemu_set_irq(s->gpios[offset], !!(new & mask));
>> This can result in an out-of-bounds access to "s->gpios" because the
>> gpio sets do _not_ have the same length. Some of the groups (e.g.
>> GPIOAB) only have 4 pins. 228 != 8 * 32 == 256.
>> To fix this, I converted the gpio array from dense to sparse, to that
>> match both the hardware layout and this existing indexing code.
>
> This is one logical change: 1 patch
>
>> Also, I noticed that some of the property specifications looked wrong:
>> the lower 8 bits in the input and output u32's correspond to the first
>> group label, and the upper 8 bits correspond to the last group label.
>
> Another logical change: another patch :)
>
> So please split this patch in 2. Maybe easier to fix GPIOSetProperties
> first, then convert from dense to sparse array.
>
Good point, I’ll split it up then!
> Regards,
>
> Phil.
>
>> I looked at the datasheet and several of these declarations seemed
>> incorrect to me (I was basing it off of the I/O column). If somebody
>> can double-check this, I'd really appreciate it!
>> Some were definitely wrong though, like "Y" and "Z" in the 2600.
>> @@ -796,7 +776,7 @@ static const GPIOSetProperties ast2500_set_props[] = {
>> [3] = {0xffffffff, 0xffffffff, {"M", "N", "O", "P"} },
>> [4] = {0xffffffff, 0xffffffff, {"Q", "R", "S", "T"} },
>> [5] = {0xffffffff, 0x0000ffff, {"U", "V", "W", "X"} },
>> - [6] = {0xffffff0f, 0x0fffff0f, {"Y", "Z", "AA", "AB"} },
>> + [6] = {0x0fffffff, 0x0fffffff, {"Y", "Z", "AA", "AB"} },
>> [7] = {0x000000ff, 0x000000ff, {"AC"} },
>> };
>> @@ -805,9 +785,9 @@ static GPIOSetProperties ast2600_3_3v_set_props[] = {
>> [1] = {0xffffffff, 0xffffffff, {"E", "F", "G", "H"} },
>> [2] = {0xffffffff, 0xffffffff, {"I", "J", "K", "L"} },
>> [3] = {0xffffffff, 0xffffffff, {"M", "N", "O", "P"} },
>> - [4] = {0xffffffff, 0xffffffff, {"Q", "R", "S", "T"} },
>> - [5] = {0xffffffff, 0x0000ffff, {"U", "V", "W", "X"} },
>> - [6] = {0xffff0000, 0x0fff0000, {"Y", "Z", "", ""} },
>> + [4] = {0xffffffff, 0x00ffffff, {"Q", "R", "S", "T"} },
>> + [5] = {0xffffffff, 0xffffff00, {"U", "V", "W", "X"} },
>> + [6] = {0x0000ffff, 0x0000ffff, {"Y", "Z", "", ""} },
>> };
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>> ---
>> hw/gpio/aspeed_gpio.c | 80 +++++++++++++++--------------------
>> include/hw/gpio/aspeed_gpio.h | 5 +--
>> 2 files changed, 35 insertions(+), 50 deletions(-)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] hw: aspeed_gpio: Fix GPIO array indexing
2021-09-24 6:19 ` [PATCH 1/1] " pdel
@ 2021-09-25 11:02 ` Philippe Mathieu-Daudé
2021-09-25 14:28 ` Peter Delevoryas
0 siblings, 1 reply; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-25 11:02 UTC (permalink / raw)
To: pdel; +Cc: qemu-devel, patrick, rashmica.g, clg, joel
Hi Peter,
On 9/24/21 08:19, pdel@fb.com wrote:
> From: Peter Delevoryas <pdel@fb.com>
>
> The gpio array is declared as a dense array:
>
> ...
> qemu_irq gpios[ASPEED_GPIO_NR_PINS];
>
> (AST2500 has 228, AST2400 has 216, AST2600 has 208)
>
> However, this array is used like a matrix of GPIO sets
> (e.g. gpio[NR_SETS][NR_PINS_PER_SET] = gpio[8][32])
>
> size_t offset = set * GPIOS_PER_SET + gpio;
> qemu_set_irq(s->gpios[offset], !!(new & mask));
>
> This can result in an out-of-bounds access to "s->gpios" because the
> gpio sets do _not_ have the same length. Some of the groups (e.g.
> GPIOAB) only have 4 pins. 228 != 8 * 32 == 256.
>
> To fix this, I converted the gpio array from dense to sparse, to that
> match both the hardware layout and this existing indexing code.
This is one logical change: 1 patch
> Also, I noticed that some of the property specifications looked wrong:
> the lower 8 bits in the input and output u32's correspond to the first
> group label, and the upper 8 bits correspond to the last group label.
Another logical change: another patch :)
So please split this patch in 2. Maybe easier to fix GPIOSetProperties
first, then convert from dense to sparse array.
Regards,
Phil.
> I looked at the datasheet and several of these declarations seemed
> incorrect to me (I was basing it off of the I/O column). If somebody
> can double-check this, I'd really appreciate it!
>
> Some were definitely wrong though, like "Y" and "Z" in the 2600.
>
> @@ -796,7 +776,7 @@ static const GPIOSetProperties ast2500_set_props[] = {
> [3] = {0xffffffff, 0xffffffff, {"M", "N", "O", "P"} },
> [4] = {0xffffffff, 0xffffffff, {"Q", "R", "S", "T"} },
> [5] = {0xffffffff, 0x0000ffff, {"U", "V", "W", "X"} },
> - [6] = {0xffffff0f, 0x0fffff0f, {"Y", "Z", "AA", "AB"} },
> + [6] = {0x0fffffff, 0x0fffffff, {"Y", "Z", "AA", "AB"} },
> [7] = {0x000000ff, 0x000000ff, {"AC"} },
> };
>
> @@ -805,9 +785,9 @@ static GPIOSetProperties ast2600_3_3v_set_props[] = {
> [1] = {0xffffffff, 0xffffffff, {"E", "F", "G", "H"} },
> [2] = {0xffffffff, 0xffffffff, {"I", "J", "K", "L"} },
> [3] = {0xffffffff, 0xffffffff, {"M", "N", "O", "P"} },
> - [4] = {0xffffffff, 0xffffffff, {"Q", "R", "S", "T"} },
> - [5] = {0xffffffff, 0x0000ffff, {"U", "V", "W", "X"} },
> - [6] = {0xffff0000, 0x0fff0000, {"Y", "Z", "", ""} },
> + [4] = {0xffffffff, 0x00ffffff, {"Q", "R", "S", "T"} },
> + [5] = {0xffffffff, 0xffffff00, {"U", "V", "W", "X"} },
> + [6] = {0x0000ffff, 0x0000ffff, {"Y", "Z", "", ""} },
> };
>
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---
> hw/gpio/aspeed_gpio.c | 80 +++++++++++++++--------------------
> include/hw/gpio/aspeed_gpio.h | 5 +--
> 2 files changed, 35 insertions(+), 50 deletions(-)
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] hw: aspeed_gpio: Fix GPIO array indexing
2021-09-24 6:19 pdel
@ 2021-09-24 6:19 ` pdel
2021-09-25 11:02 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 10+ messages in thread
From: pdel @ 2021-09-24 6:19 UTC (permalink / raw)
Cc: clg, joel, rashmica.g, patrick, qemu-devel, Peter Delevoryas
From: Peter Delevoryas <pdel@fb.com>
The gpio array is declared as a dense array:
...
qemu_irq gpios[ASPEED_GPIO_NR_PINS];
(AST2500 has 228, AST2400 has 216, AST2600 has 208)
However, this array is used like a matrix of GPIO sets
(e.g. gpio[NR_SETS][NR_PINS_PER_SET] = gpio[8][32])
size_t offset = set * GPIOS_PER_SET + gpio;
qemu_set_irq(s->gpios[offset], !!(new & mask));
This can result in an out-of-bounds access to "s->gpios" because the
gpio sets do _not_ have the same length. Some of the groups (e.g.
GPIOAB) only have 4 pins. 228 != 8 * 32 == 256.
To fix this, I converted the gpio array from dense to sparse, to that
match both the hardware layout and this existing indexing code.
Also, I noticed that some of the property specifications looked wrong:
the lower 8 bits in the input and output u32's correspond to the first
group label, and the upper 8 bits correspond to the last group label.
I looked at the datasheet and several of these declarations seemed
incorrect to me (I was basing it off of the I/O column). If somebody
can double-check this, I'd really appreciate it!
Some were definitely wrong though, like "Y" and "Z" in the 2600.
@@ -796,7 +776,7 @@ static const GPIOSetProperties ast2500_set_props[] = {
[3] = {0xffffffff, 0xffffffff, {"M", "N", "O", "P"} },
[4] = {0xffffffff, 0xffffffff, {"Q", "R", "S", "T"} },
[5] = {0xffffffff, 0x0000ffff, {"U", "V", "W", "X"} },
- [6] = {0xffffff0f, 0x0fffff0f, {"Y", "Z", "AA", "AB"} },
+ [6] = {0x0fffffff, 0x0fffffff, {"Y", "Z", "AA", "AB"} },
[7] = {0x000000ff, 0x000000ff, {"AC"} },
};
@@ -805,9 +785,9 @@ static GPIOSetProperties ast2600_3_3v_set_props[] = {
[1] = {0xffffffff, 0xffffffff, {"E", "F", "G", "H"} },
[2] = {0xffffffff, 0xffffffff, {"I", "J", "K", "L"} },
[3] = {0xffffffff, 0xffffffff, {"M", "N", "O", "P"} },
- [4] = {0xffffffff, 0xffffffff, {"Q", "R", "S", "T"} },
- [5] = {0xffffffff, 0x0000ffff, {"U", "V", "W", "X"} },
- [6] = {0xffff0000, 0x0fff0000, {"Y", "Z", "", ""} },
+ [4] = {0xffffffff, 0x00ffffff, {"Q", "R", "S", "T"} },
+ [5] = {0xffffffff, 0xffffff00, {"U", "V", "W", "X"} },
+ [6] = {0x0000ffff, 0x0000ffff, {"Y", "Z", "", ""} },
};
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
hw/gpio/aspeed_gpio.c | 80 +++++++++++++++--------------------
include/hw/gpio/aspeed_gpio.h | 5 +--
2 files changed, 35 insertions(+), 50 deletions(-)
diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index dfa6d6cb40..8e3f7e4398 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -16,11 +16,7 @@
#include "hw/irq.h"
#include "migration/vmstate.h"
-#define GPIOS_PER_REG 32
-#define GPIOS_PER_SET GPIOS_PER_REG
-#define GPIO_PIN_GAP_SIZE 4
#define GPIOS_PER_GROUP 8
-#define GPIO_GROUP_SHIFT 3
/* GPIO Source Types */
#define ASPEED_CMD_SRC_MASK 0x01010101
@@ -259,7 +255,7 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
diff = old ^ new;
if (diff) {
- for (gpio = 0; gpio < GPIOS_PER_REG; gpio++) {
+ for (gpio = 0; gpio < ASPEED_GPIOS_PER_SET; gpio++) {
uint32_t mask = 1 << gpio;
/* If the gpio needs to be updated... */
@@ -283,8 +279,7 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
if (direction & mask) {
/* ...trigger the line-state IRQ */
ptrdiff_t set = aspeed_gpio_set_idx(s, regs);
- size_t offset = set * GPIOS_PER_SET + gpio;
- qemu_set_irq(s->gpios[offset], !!(new & mask));
+ qemu_set_irq(s->gpios[set][gpio], !!(new & mask));
} else {
/* ...otherwise if we meet the line's current IRQ policy... */
if (aspeed_evaluate_irq(regs, old & mask, gpio)) {
@@ -297,21 +292,6 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
qemu_set_irq(s->irq, !!(s->pending));
}
-static uint32_t aspeed_adjust_pin(AspeedGPIOState *s, uint32_t pin)
-{
- AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
- /*
- * The 2500 has a 4 pin gap in group AB and the 2400 has a 4 pin
- * gap in group Y (and only four pins in AB but this is the last group so
- * it doesn't matter).
- */
- if (agc->gap && pin >= agc->gap) {
- pin += GPIO_PIN_GAP_SIZE;
- }
-
- return pin;
-}
-
static bool aspeed_gpio_get_pin_level(AspeedGPIOState *s, uint32_t set_idx,
uint32_t pin)
{
@@ -367,7 +347,7 @@ static uint32_t update_value_control_source(GPIOSets *regs, uint32_t old_value,
uint32_t new_value = 0;
/* for each group in set */
- for (i = 0; i < GPIOS_PER_REG; i += GPIOS_PER_GROUP) {
+ for (i = 0; i < ASPEED_GPIOS_PER_SET; i += GPIOS_PER_GROUP) {
cmd_source = extract32(regs->cmd_source_0, i, 1)
| (extract32(regs->cmd_source_1, i, 1) << 1);
@@ -637,7 +617,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t data,
* bidirectional | 1 | 1 | data
* input only | 1 | 0 | 0
* output only | 0 | 1 | 1
- * no pin / gap | 0 | 0 | 0
+ * no pin | 0 | 0 | 0
*
* which is captured by:
* data = ( data | ~input) & output;
@@ -796,7 +776,7 @@ static const GPIOSetProperties ast2500_set_props[] = {
[3] = {0xffffffff, 0xffffffff, {"M", "N", "O", "P"} },
[4] = {0xffffffff, 0xffffffff, {"Q", "R", "S", "T"} },
[5] = {0xffffffff, 0x0000ffff, {"U", "V", "W", "X"} },
- [6] = {0xffffff0f, 0x0fffff0f, {"Y", "Z", "AA", "AB"} },
+ [6] = {0x0fffffff, 0x0fffffff, {"Y", "Z", "AA", "AB"} },
[7] = {0x000000ff, 0x000000ff, {"AC"} },
};
@@ -805,9 +785,9 @@ static GPIOSetProperties ast2600_3_3v_set_props[] = {
[1] = {0xffffffff, 0xffffffff, {"E", "F", "G", "H"} },
[2] = {0xffffffff, 0xffffffff, {"I", "J", "K", "L"} },
[3] = {0xffffffff, 0xffffffff, {"M", "N", "O", "P"} },
- [4] = {0xffffffff, 0xffffffff, {"Q", "R", "S", "T"} },
- [5] = {0xffffffff, 0x0000ffff, {"U", "V", "W", "X"} },
- [6] = {0xffff0000, 0x0fff0000, {"Y", "Z", "", ""} },
+ [4] = {0xffffffff, 0x00ffffff, {"Q", "R", "S", "T"} },
+ [5] = {0xffffffff, 0xffffff00, {"U", "V", "W", "X"} },
+ [6] = {0x0000ffff, 0x0000ffff, {"Y", "Z", "", ""} },
};
static GPIOSetProperties ast2600_1_8v_set_props[] = {
@@ -836,14 +816,20 @@ static void aspeed_gpio_realize(DeviceState *dev, Error **errp)
AspeedGPIOState *s = ASPEED_GPIO(dev);
SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
- int pin;
/* Interrupt parent line */
sysbus_init_irq(sbd, &s->irq);
/* Individual GPIOs */
- for (pin = 0; pin < agc->nr_gpio_pins; pin++) {
- sysbus_init_irq(sbd, &s->gpios[pin]);
+ for (int i = 0; i < ASPEED_GPIO_MAX_NR_SETS; i++) {
+ const GPIOSetProperties *props = &agc->props[i];
+ uint32_t skip = ~(props->input | props->output);
+ for (int j = 0; j < ASPEED_GPIOS_PER_SET; j++) {
+ if (skip >> j & 1) {
+ continue;
+ }
+ sysbus_init_irq(sbd, &s->gpios[i][j]);
+ }
}
memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_gpio_ops, s,
@@ -856,20 +842,22 @@ static void aspeed_gpio_init(Object *obj)
{
AspeedGPIOState *s = ASPEED_GPIO(obj);
AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
- int pin;
-
- for (pin = 0; pin < agc->nr_gpio_pins; pin++) {
- char *name;
- int set_idx = pin / GPIOS_PER_SET;
- int pin_idx = aspeed_adjust_pin(s, pin) - (set_idx * GPIOS_PER_SET);
- int group_idx = pin_idx >> GPIO_GROUP_SHIFT;
- const GPIOSetProperties *props = &agc->props[set_idx];
-
- name = g_strdup_printf("gpio%s%d", props->group_label[group_idx],
- pin_idx % GPIOS_PER_GROUP);
- object_property_add(obj, name, "bool", aspeed_gpio_get_pin,
- aspeed_gpio_set_pin, NULL, NULL);
- g_free(name);
+
+ for (int i = 0; i < ASPEED_GPIO_MAX_NR_SETS; i++) {
+ const GPIOSetProperties *props = &agc->props[i];
+ uint32_t skip = ~(props->input | props->output);
+ for (int j = 0; j < ASPEED_GPIOS_PER_SET; j++) {
+ if (skip >> j & 1) {
+ continue;
+ }
+ int group_idx = j / GPIOS_PER_GROUP;
+ int pin_idx = j % GPIOS_PER_GROUP;
+ const char *group = &props->group_label[group_idx][0];
+ char *name = g_strdup_printf("gpio%s%d", group, pin_idx);
+ object_property_add(obj, name, "bool", aspeed_gpio_get_pin,
+ aspeed_gpio_set_pin, NULL, NULL);
+ g_free(name);
+ }
}
}
@@ -926,7 +914,6 @@ static void aspeed_gpio_ast2400_class_init(ObjectClass *klass, void *data)
agc->props = ast2400_set_props;
agc->nr_gpio_pins = 216;
agc->nr_gpio_sets = 7;
- agc->gap = 196;
agc->reg_table = aspeed_3_3v_gpios;
}
@@ -937,7 +924,6 @@ static void aspeed_gpio_2500_class_init(ObjectClass *klass, void *data)
agc->props = ast2500_set_props;
agc->nr_gpio_pins = 228;
agc->nr_gpio_sets = 8;
- agc->gap = 220;
agc->reg_table = aspeed_3_3v_gpios;
}
diff --git a/include/hw/gpio/aspeed_gpio.h b/include/hw/gpio/aspeed_gpio.h
index e1636ce7fe..801846befb 100644
--- a/include/hw/gpio/aspeed_gpio.h
+++ b/include/hw/gpio/aspeed_gpio.h
@@ -17,9 +17,9 @@
OBJECT_DECLARE_TYPE(AspeedGPIOState, AspeedGPIOClass, ASPEED_GPIO)
#define ASPEED_GPIO_MAX_NR_SETS 8
+#define ASPEED_GPIOS_PER_SET 32
#define ASPEED_REGS_PER_BANK 14
#define ASPEED_GPIO_MAX_NR_REGS (ASPEED_REGS_PER_BANK * ASPEED_GPIO_MAX_NR_SETS)
-#define ASPEED_GPIO_NR_PINS 228
#define ASPEED_GROUPS_PER_SET 4
#define ASPEED_GPIO_NR_DEBOUNCE_REGS 3
#define ASPEED_CHARS_PER_GROUP_LABEL 4
@@ -60,7 +60,6 @@ struct AspeedGPIOClass {
const GPIOSetProperties *props;
uint32_t nr_gpio_pins;
uint32_t nr_gpio_sets;
- uint32_t gap;
const AspeedGPIOReg *reg_table;
};
@@ -72,7 +71,7 @@ struct AspeedGPIOState {
MemoryRegion iomem;
int pending;
qemu_irq irq;
- qemu_irq gpios[ASPEED_GPIO_NR_PINS];
+ qemu_irq gpios[ASPEED_GPIO_MAX_NR_SETS][ASPEED_GPIOS_PER_SET];
/* Parallel GPIO Registers */
uint32_t debounce_regs[ASPEED_GPIO_NR_DEBOUNCE_REGS];
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-10-08 3:22 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-28 3:43 [PATCH 0/1] hw: aspeed_gpio: Fix GPIO array indexing pdel
2021-09-28 3:43 ` [PATCH 1/1] " pdel
2021-10-04 9:07 ` Cédric Le Goater
2021-10-04 11:43 ` Cédric Le Goater
2021-10-08 3:19 ` Peter Delevoryas
2021-09-30 0:46 ` [PATCH 0/1] " Peter Delevoryas
-- strict thread matches above, loose matches on Subject: below --
2021-09-24 6:19 pdel
2021-09-24 6:19 ` [PATCH 1/1] " pdel
2021-09-25 11:02 ` Philippe Mathieu-Daudé
2021-09-25 14:28 ` Peter Delevoryas
2021-09-27 10:05 ` Cédric Le Goater
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.