* [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 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 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
* [PATCH 0/1] hw: aspeed_gpio: Fix GPIO array indexing @ 2021-09-24 6:19 pdel 2021-09-24 6:19 ` [PATCH 1/1] " pdel 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> Hey everyone, I think there might be a bug aspeed_gpio_update, when it's selecting a GPIO IRQ to update. I was testing booting Facebook's OpenBMC platform "YosemiteV2" (fby2), and I was hitting a segfault in QEMU: 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 The commit message for the fix has a little more info on the bug here, see that for more info. I tested this by verifying that after this diff, I can boot this fby2 platform. I don't see any unit or qtest's for aspeed gpio's, maybe I could add one? I figured that, first, I could just put out an email to let everyone know about it, and get the diff reviewed. 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 | 80 +++++++++++++++-------------------- include/hw/gpio/aspeed_gpio.h | 5 +-- 2 files changed, 35 insertions(+), 50 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-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
* 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
* 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-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
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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).