On Mon, Jul 11, 2022 at 10:56:08PM +0930, Andrew Jeffery wrote:
>
>
> On Fri, 8 Jul 2022, at 04:34, Peter Delevoryas wrote:
> > On Thu, Jul 07, 2022 at 10:53:57AM -0700, Peter Delevoryas wrote:
> >> On Thu, Jul 07, 2022 at 10:56:02AM +0200, Cédric Le Goater wrote:
> >> > On 7/7/22 09:17, Peter Delevoryas wrote:
> >> > > It seems that aspeed_gpio_update is allowing the value for input pins
> >> > > to be
> >> > > modified through register writes and QOM property modification.
> >> > >
> >> > > The QOM property modification is fine, but modifying the value through
> >> > > register writes from the guest OS seems wrong if the pin's direction
> >> > > is set
> >> > > to input.
> >> > >
> >> > > The datasheet specifies that "0" bits in the direction register select
> >> > > input
> >> > > mode, and "1" selects output mode.
> >> > >
> >> > > OpenBMC userspace code is accidentally writing 0's to the GPIO data
> >> > > registers somewhere (or perhaps the driver is doing it through a reset
> >> > > or
> >> > > something), and this is overwriting GPIO FRU information (board ID,
> >> > > slot
> >> > > presence pins) that is initialized in Aspeed machine reset code (see
> >> > > fby35_reset() in hw/arm/aspeed.c).
> >> >
> >> > It might be good to log a GUEST_ERROR in that case, when writing to an
> >> > input GPIO and when reading from an output GPIO.
> >>
> >> Good idea, I'll include a GUEST_ERROR for writing to an input GPIO.
> >>
> >> I'm actually not totally certain about emitting an error when reading from
> >> an
> >> output GPIO, because the driver can only do 8-bit reads at the finest
> >> granularity, and if 1 of the 8 pins' direction is output, then it will be
> >> reading the value of an output pin. But, that's not really bad, because
> >> presumably the value will be ignored. Maybe I can go test this out on
> >> hardware and figure out what happens though.
> >
> > Did a small experiment, I was looking at some of the most significant
> > bits:
> >
> > root@dhcp-100-96-192-133:~# devmem 0x1e780000
> > 0x3CFF303E
> > root@dhcp-100-96-192-133:~# devmem 0x1e780004
> > 0x2800000C
> > root@dhcp-100-96-192-133:~# devmem 0x1e780000 32 0xffffffff
> > root@dhcp-100-96-192-133:~# devmem 0x1e780004
> > 0x2800000C
> > root@dhcp-100-96-192-133:~# devmem 0x1e780000
> > 0x3CFF303E
> > root@dhcp-100-96-192-133:~# devmem 0x1e780000
> > 0x3CFF303E
> > root@dhcp-100-96-192-133:~# devmem 0x1e780000 32 0
> > root@dhcp-100-96-192-133:~# devmem 0x1e780000
> > 0x14FF303A
> >
> > Seems like the output pin 0x20000000 was initially high, and the input
> > pin right next to it 0x10000000 was also high. After writing 0 to the
> > data register, the value in the data register changed for the output
> > pin, but not the input pin. Which matches what we're planning on doing
> > in the controller.
> >
> > So yeah, I'll add GUEST_ERROR for writes to input pins but not output
> > pins. The driver should probably be doing a read-modify-update.
> > Although...if it's not, that technically wouldn't matter, behavior
> > wise...maybe GUEST_ERROR isn't appropriate for writes to input pins
> > either, for the same reason as I mentioned with reads of output pins.
> > I'll let you guys comment on what you think we should do.
> >
>
> With the quick hack below I think I got sensible behaviour?
>
> ```
> # devmem 0x1e780000
> 0x00000000
> # devmem 0x1e780004
> 0x00000000
> # devmem 0x1e780004 32 1
> # devmem 0x1e780000 32 1
> # devmem 0x1e780000
> 0x00000001
> # devmem 0x1e780000 32 3
> # devmem 0x1e780000
> 0x00000001
> # QEMU 7.0.0 monitor - type 'help' for more information
> (qemu) qom-set gpio gpioA1 on
> (qemu)
>
> # devmem 0x1e780000
> 0x00000003
> # (qemu) qom-set gpio gpioA1 off
> (qemu)
>
> # devmem 0x1e780000
> 0x00000001
> # (qemu) qom-set gpio gpioA0 off
> (qemu)
> # devmem 0x1e780000
> 0x00000001
> #
> ```
>
> That was with the patch below. However, I think there's a deeper issue
> with the input masking that needs to be addressed. Essentially we lack
> modelling for the actual line state, we were proxying that with
> register state. As it stands if we input-mask a line and use qom-set to
> change its state the state update will go missing. However, as Joel
> notes, I don't think we have anything configuring input masking.
>
> diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> index c63634d3d3e2..a1aa6504a8d8 100644
> --- a/hw/gpio/aspeed_gpio.c
> +++ b/hw/gpio/aspeed_gpio.c
> @@ -244,7 +244,7 @@ static ptrdiff_t aspeed_gpio_set_idx(AspeedGPIOState *s,
> GPIOSets *regs)
> }
>
> static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
> - uint32_t value)
> + uint32_t value, uint32_t mode_mask)
> {
> uint32_t input_mask = regs->input_mask;
> uint32_t direction = regs->direction;
> @@ -253,7 +253,8 @@ static void aspeed_gpio_update(AspeedGPIOState *s,
> GPIOSets *regs,
> uint32_t diff;
> int gpio;
>
> - diff = old ^ new;
> + diff = (old ^ new);
> + diff &= mode_mask;
> if (diff) {
> for (gpio = 0; gpio < ASPEED_GPIOS_PER_SET; gpio++) {
> uint32_t mask = 1 << gpio;
> @@ -315,7 +316,7 @@ static void aspeed_gpio_set_pin_level(AspeedGPIOState *s,
> uint32_t set_idx,
> value &= !pin_mask;
> }
>
> - aspeed_gpio_update(s, &s->sets[set_idx], value);
> + aspeed_gpio_update(s, &s->sets[set_idx], value,
> ~s->sets[set_idx].direction);
> }
>
> /*
> @@ -607,7 +608,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr
> offset, uint64_t data,
> data &= props->output;
> data = update_value_control_source(set, set->data_value, data);
> set->data_read = data;
> - aspeed_gpio_update(s, set, data);
> + aspeed_gpio_update(s, set, data, set->direction);
> return;
> case gpio_reg_direction:
> /*
> @@ -683,7 +684,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr
> offset, uint64_t data,
> HWADDR_PRIx"\n", __func__, offset);
> return;
> }
> - aspeed_gpio_update(s, set, set->data_value);
> + aspeed_gpio_update(s, set, set->data_value, UINT32_MAX);
Looks great overall, but why is the mode_mask UINT32_MAX here? Shouldn't it be
set->direction? We only want to let the guest OS write to output pins, right?
Or is that not how the register works?
> return;
> }
>
>
