[PATCH 1/2] hw/gpio/aspeed: Don't let guests modify input pins

[PATCH 1/2] hw/gpio/aspeed: Don't let guests modify input pins

[Peter Delevoryas]

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).

Signed-off-by: Peter Delevoryas <peter@pjd.dev>
Fixes: 4b7f956862dc ("hw/gpio: Add basic Aspeed GPIO model for AST2400 and AST2500")
---
 hw/gpio/aspeed_gpio.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index a62a673857..2eae427201 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -268,7 +268,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, bool force)
 {
     uint32_t input_mask = regs->input_mask;
     uint32_t direction = regs->direction;
@@ -293,10 +293,12 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
             }
 
             /* ...then update the state. */
-            if (mask & new) {
-                regs->data_value |= mask;
-            } else {
-                regs->data_value &= ~mask;
+            if (direction & mask || force) {
+                if (mask & new) {
+                    regs->data_value |= mask;
+                } else {
+                    regs->data_value &= ~mask;
+                }
             }
 
             /* If the gpio is set to output... */
@@ -339,7 +341,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, true);
 }
 
 /*
@@ -653,7 +655,7 @@ static void aspeed_gpio_write_index_mode(void *opaque, hwaddr offset,
         reg_value = update_value_control_source(set, set->data_value,
                                                 reg_value);
         set->data_read = reg_value;
-        aspeed_gpio_update(s, set, reg_value);
+        aspeed_gpio_update(s, set, reg_value, false);
         return;
     case gpio_reg_idx_direction:
         reg_value = set->direction;
@@ -753,7 +755,7 @@ static void aspeed_gpio_write_index_mode(void *opaque, hwaddr offset,
             __func__, offset, data, reg_idx_type);
         return;
     }
-    aspeed_gpio_update(s, set, set->data_value);
+    aspeed_gpio_update(s, set, set->data_value, false);
     return;
 }
 
@@ -799,7 +801,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, false);
         return;
     case gpio_reg_direction:
         /*
@@ -875,7 +877,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t data,
                       PRIx64"\n", __func__, offset);
         return;
     }
-    aspeed_gpio_update(s, set, set->data_value);
+    aspeed_gpio_update(s, set, set->data_value, false);
     return;
 }
 
-- 
2.36.1

[PATCH 2/2] aspeed: Add fby35-bmc slot GPIO's

[Peter Delevoryas]

Signed-off-by: Peter Delevoryas <peter@pjd.dev>
---
 hw/arm/aspeed.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 6fe9b13548..0ce9a42c2b 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -1343,11 +1343,23 @@ static void fby35_reset(MachineState *state)
 
     qemu_devices_reset();
 
-    /* Board ID */
+    /* Board ID: 7 (Class-1, 4 slots) */
     object_property_set_bool(OBJECT(gpio), "gpioV4", true, &error_fatal);
     object_property_set_bool(OBJECT(gpio), "gpioV5", true, &error_fatal);
     object_property_set_bool(OBJECT(gpio), "gpioV6", true, &error_fatal);
     object_property_set_bool(OBJECT(gpio), "gpioV7", false, &error_fatal);
+
+    /* Slot presence pins, inverse polarity. (False means present) */
+    object_property_set_bool(OBJECT(gpio), "gpioH4", false, &error_fatal);
+    object_property_set_bool(OBJECT(gpio), "gpioH5", true, &error_fatal);
+    object_property_set_bool(OBJECT(gpio), "gpioH6", true, &error_fatal);
+    object_property_set_bool(OBJECT(gpio), "gpioH7", true, &error_fatal);
+
+    /* Slot 12v power pins, normal polarity. (True means powered-on) */
+    object_property_set_bool(OBJECT(gpio), "gpioB2", true, &error_fatal);
+    object_property_set_bool(OBJECT(gpio), "gpioB3", false, &error_fatal);
+    object_property_set_bool(OBJECT(gpio), "gpioB4", false, &error_fatal);
+    object_property_set_bool(OBJECT(gpio), "gpioB5", false, &error_fatal);
 }
 
 static void aspeed_machine_fby35_class_init(ObjectClass *oc, void *data)
-- 
2.36.1

Re: [PATCH 0/2] hw/gpio/aspeed: Don't let guests modify input pins

[Peter Delevoryas]

On Thu, Jul 07, 2022 at 12:17:29AM -0700, Peter Delevoryas wrote:
> The fby35 OpenBMC sysvinit scripts check various GPIO pins at start and
> decide where to start IPMB daemons for each slot in the sled (4 slots max).
> It only starts an IPMB daemon if the slot GPIO pins indicate that it's
> present and powered on.
> 
> I've been simulating some input pins by setting their value in the machine
> reset function. I think a proper solution would be to add input pins to the
> Aspeed GPIO code and create devices that force the pins high or low
> appropriately, but for now setting the QOM property seemed fine.
> 
> But, I noticed that while the values were set initially, something in the
> boot process resets all the values I set to "low". I imagine something in
> userspace or the driver is blanket writing zero to the data registers. I
> think the Aspeed GPIO controller probably shouldn't be changing the value of
> input pins in this case.
> 
> To fix this, we could just make sure that aspeed_gpio_update() never sets
> the value of an input pin. However, that would also prevent my code in
> fby35_reset from initializing the input pins to some special value. So, to
> support the QOM property setup use-case, I added a "force" parameter. Kinda
> hacky, but it was the simplest thing I could think of.
> 
> Thanks,
> Peter

My gitconfig was messed up, I was using the maintainers.pl script in my
send-email.ccCmd, but that doesn't work for the cover letter.  So, it just sent
the cover letter to me. I didn't notice it in test emailing cause I usually just
test sending to myself. Sorry about this. Hopefully I should finally have
my email configuration fixed at this point...I hope.

Peter

> 
> Peter Delevoryas (2):
>   hw/gpio/aspeed: Don't let guests modify input pins
>   aspeed: Add fby35-bmc slot GPIO's
> 
>  hw/arm/aspeed.c       | 14 +++++++++++++-
>  hw/gpio/aspeed_gpio.c | 22 ++++++++++++----------
>  2 files changed, 25 insertions(+), 11 deletions(-)
> 
> -- 
> 2.36.1
> 

Re: [PATCH 1/2] hw/gpio/aspeed: Don't let guests modify input pins

[Joel Stanley]

On Thu, 7 Jul 2022 at 07:17, Peter Delevoryas <peter@pjd.dev> 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).
>
> Signed-off-by: Peter Delevoryas <peter@pjd.dev>
> Fixes: 4b7f956862dc ("hw/gpio: Add basic Aspeed GPIO model for AST2400 and AST2500")
> ---
>  hw/gpio/aspeed_gpio.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> index a62a673857..2eae427201 100644
> --- a/hw/gpio/aspeed_gpio.c
> +++ b/hw/gpio/aspeed_gpio.c
> @@ -268,7 +268,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, bool force)
>  {
>      uint32_t input_mask = regs->input_mask;
>      uint32_t direction = regs->direction;
> @@ -293,10 +293,12 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
>              }
>

Reading this model hurts my head a little. Perhaps we also need to add
a test for this case to make it clear what's going on.

The test above the code you've changed does this:

>            /* ...and we're output or not input-masked... */
>            if (!(direction & mask) && (input_mask & mask)) {
>                continue;
>            }

For clarity, !(direction & mask) means "is input".

The comment confuses me because it says "or", but the code has "and".

input_mask doesn't seem to feature in the Linux driver, so that will
always be zero. The test will be X && 0, which is always 0.

If you changed it to || then we would do the test that the comment
suggests. When the pin is input, we will skip updating the value.

This will solve the bug you had with your input pins being reset. It
won't fix the QOM case, but we could consider handling that separately
without confusing the logic in this function.


>              /* ...then update the state. */
> -            if (mask & new) {
> -                regs->data_value |= mask;
> -            } else {
> -                regs->data_value &= ~mask;
> +            if (direction & mask || force) {
> +                if (mask & new) {
> +                    regs->data_value |= mask;
> +                } else {
> +                    regs->data_value &= ~mask;
> +                }
>              }
>
>              /* If the gpio is set to output... */
> @@ -339,7 +341,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, true);
>  }
>
>  /*
> @@ -653,7 +655,7 @@ static void aspeed_gpio_write_index_mode(void *opaque, hwaddr offset,
>          reg_value = update_value_control_source(set, set->data_value,
>                                                  reg_value);
>          set->data_read = reg_value;
> -        aspeed_gpio_update(s, set, reg_value);
> +        aspeed_gpio_update(s, set, reg_value, false);
>          return;
>      case gpio_reg_idx_direction:
>          reg_value = set->direction;
> @@ -753,7 +755,7 @@ static void aspeed_gpio_write_index_mode(void *opaque, hwaddr offset,
>              __func__, offset, data, reg_idx_type);
>          return;
>      }
> -    aspeed_gpio_update(s, set, set->data_value);
> +    aspeed_gpio_update(s, set, set->data_value, false);
>      return;
>  }
>
> @@ -799,7 +801,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, false);
>          return;
>      case gpio_reg_direction:
>          /*
> @@ -875,7 +877,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t data,
>                        PRIx64"\n", __func__, offset);
>          return;
>      }
> -    aspeed_gpio_update(s, set, set->data_value);
> +    aspeed_gpio_update(s, set, set->data_value, false);
>      return;
>  }
>
> --
> 2.36.1
>

Re: [PATCH 1/2] hw/gpio/aspeed: Don't let guests modify input pins

[Cédric Le Goater]

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.

Thanks,

C.

> 
> Signed-off-by: Peter Delevoryas <peter@pjd.dev>
> Fixes: 4b7f956862dc ("hw/gpio: Add basic Aspeed GPIO model for AST2400 and AST2500")
> ---
>   hw/gpio/aspeed_gpio.c | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> index a62a673857..2eae427201 100644
> --- a/hw/gpio/aspeed_gpio.c
> +++ b/hw/gpio/aspeed_gpio.c
> @@ -268,7 +268,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, bool force)
>   {
>       uint32_t input_mask = regs->input_mask;
>       uint32_t direction = regs->direction;
> @@ -293,10 +293,12 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
>               }
>   
>               /* ...then update the state. */
> -            if (mask & new) {
> -                regs->data_value |= mask;
> -            } else {
> -                regs->data_value &= ~mask;
> +            if (direction & mask || force) {
> +                if (mask & new) {
> +                    regs->data_value |= mask;
> +                } else {
> +                    regs->data_value &= ~mask;
> +                }
>               }
>   
>               /* If the gpio is set to output... */
> @@ -339,7 +341,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, true);
>   }
>   
>   /*
> @@ -653,7 +655,7 @@ static void aspeed_gpio_write_index_mode(void *opaque, hwaddr offset,
>           reg_value = update_value_control_source(set, set->data_value,
>                                                   reg_value);
>           set->data_read = reg_value;
> -        aspeed_gpio_update(s, set, reg_value);
> +        aspeed_gpio_update(s, set, reg_value, false);
>           return;
>       case gpio_reg_idx_direction:
>           reg_value = set->direction;
> @@ -753,7 +755,7 @@ static void aspeed_gpio_write_index_mode(void *opaque, hwaddr offset,
>               __func__, offset, data, reg_idx_type);
>           return;
>       }
> -    aspeed_gpio_update(s, set, set->data_value);
> +    aspeed_gpio_update(s, set, set->data_value, false);
>       return;
>   }
>   
> @@ -799,7 +801,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, false);
>           return;
>       case gpio_reg_direction:
>           /*
> @@ -875,7 +877,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t data,
>                         PRIx64"\n", __func__, offset);
>           return;
>       }
> -    aspeed_gpio_update(s, set, set->data_value);
> +    aspeed_gpio_update(s, set, set->data_value, false);
>       return;
>   }
>   

Re: [PATCH 1/2] hw/gpio/aspeed: Don't let guests modify input pins

[Peter Delevoryas]

On Thu, Jul 07, 2022 at 08:20:01AM +0000, Joel Stanley wrote:
> On Thu, 7 Jul 2022 at 07:17, Peter Delevoryas <peter@pjd.dev> 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).
> >
> > Signed-off-by: Peter Delevoryas <peter@pjd.dev>
> > Fixes: 4b7f956862dc ("hw/gpio: Add basic Aspeed GPIO model for AST2400 and AST2500")
> > ---
> >  hw/gpio/aspeed_gpio.c | 22 ++++++++++++----------
> >  1 file changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> > index a62a673857..2eae427201 100644
> > --- a/hw/gpio/aspeed_gpio.c
> > +++ b/hw/gpio/aspeed_gpio.c
> > @@ -268,7 +268,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, bool force)
> >  {
> >      uint32_t input_mask = regs->input_mask;
> >      uint32_t direction = regs->direction;
> > @@ -293,10 +293,12 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
> >              }
> >
> 
> Reading this model hurts my head a little. Perhaps we also need to add
> a test for this case to make it clear what's going on.

Yeah, I guess the idea was to create one function "aspeed_gpio_update"
to handle register writes, so it takes a 32-bit register value function
parameter.

The problem is that it still iterates over all the GPIO's in the
register and updates them individually anyways. Maybe there's an
opportunity here to move the xor + for-loop to a wrapper.

But yeah, I'll add another commit that includes a test case for this.

> The test above the code you've changed does this:
> 
> >            /* ...and we're output or not input-masked... */
> >            if (!(direction & mask) && (input_mask & mask)) {
> >                continue;
> >            }
> 
> For clarity, !(direction & mask) means "is input".
> 
> The comment confuses me because it says "or", but the code has "and".

Yeah I was looking at this as well and noticed the comment doesn't make
any sense.

> 
> input_mask doesn't seem to feature in the Linux driver, so that will
> always be zero. The test will be X && 0, which is always 0.

Ohhhhhh I didn't notice that though. Good catch!!

> 
> If you changed it to || then we would do the test that the comment
> suggests. When the pin is input, we will skip updating the value.
> 
> This will solve the bug you had with your input pins being reset. It
> won't fix the QOM case, but we could consider handling that separately
> without confusing the logic in this function.

Yes good point: I'll change this to || and figure out a different way to
update the value through the QOM property setter.

> 
> 
> >              /* ...then update the state. */
> > -            if (mask & new) {
> > -                regs->data_value |= mask;
> > -            } else {
> > -                regs->data_value &= ~mask;
> > +            if (direction & mask || force) {
> > +                if (mask & new) {
> > +                    regs->data_value |= mask;
> > +                } else {
> > +                    regs->data_value &= ~mask;
> > +                }
> >              }
> >
> >              /* If the gpio is set to output... */
> > @@ -339,7 +341,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, true);
> >  }
> >
> >  /*
> > @@ -653,7 +655,7 @@ static void aspeed_gpio_write_index_mode(void *opaque, hwaddr offset,
> >          reg_value = update_value_control_source(set, set->data_value,
> >                                                  reg_value);
> >          set->data_read = reg_value;
> > -        aspeed_gpio_update(s, set, reg_value);
> > +        aspeed_gpio_update(s, set, reg_value, false);
> >          return;
> >      case gpio_reg_idx_direction:
> >          reg_value = set->direction;
> > @@ -753,7 +755,7 @@ static void aspeed_gpio_write_index_mode(void *opaque, hwaddr offset,
> >              __func__, offset, data, reg_idx_type);
> >          return;
> >      }
> > -    aspeed_gpio_update(s, set, set->data_value);
> > +    aspeed_gpio_update(s, set, set->data_value, false);
> >      return;
> >  }
> >
> > @@ -799,7 +801,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, false);
> >          return;
> >      case gpio_reg_direction:
> >          /*
> > @@ -875,7 +877,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t data,
> >                        PRIx64"\n", __func__, offset);
> >          return;
> >      }
> > -    aspeed_gpio_update(s, set, set->data_value);
> > +    aspeed_gpio_update(s, set, set->data_value, false);
> >      return;
> >  }
> >
> > --
> > 2.36.1
> >

Re: [PATCH 1/2] hw/gpio/aspeed: Don't let guests modify input pins

[Peter Delevoryas]

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.

Thanks,
Peter

> 
> Thanks,
> 
> C.
> 
> > 
> > Signed-off-by: Peter Delevoryas <peter@pjd.dev>
> > Fixes: 4b7f956862dc ("hw/gpio: Add basic Aspeed GPIO model for AST2400 and AST2500")
> > ---
> >   hw/gpio/aspeed_gpio.c | 22 ++++++++++++----------
> >   1 file changed, 12 insertions(+), 10 deletions(-)
> > 
> > diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> > index a62a673857..2eae427201 100644
> > --- a/hw/gpio/aspeed_gpio.c
> > +++ b/hw/gpio/aspeed_gpio.c
> > @@ -268,7 +268,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, bool force)
> >   {
> >       uint32_t input_mask = regs->input_mask;
> >       uint32_t direction = regs->direction;
> > @@ -293,10 +293,12 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
> >               }
> >               /* ...then update the state. */
> > -            if (mask & new) {
> > -                regs->data_value |= mask;
> > -            } else {
> > -                regs->data_value &= ~mask;
> > +            if (direction & mask || force) {
> > +                if (mask & new) {
> > +                    regs->data_value |= mask;
> > +                } else {
> > +                    regs->data_value &= ~mask;
> > +                }
> >               }
> >               /* If the gpio is set to output... */
> > @@ -339,7 +341,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, true);
> >   }
> >   /*
> > @@ -653,7 +655,7 @@ static void aspeed_gpio_write_index_mode(void *opaque, hwaddr offset,
> >           reg_value = update_value_control_source(set, set->data_value,
> >                                                   reg_value);
> >           set->data_read = reg_value;
> > -        aspeed_gpio_update(s, set, reg_value);
> > +        aspeed_gpio_update(s, set, reg_value, false);
> >           return;
> >       case gpio_reg_idx_direction:
> >           reg_value = set->direction;
> > @@ -753,7 +755,7 @@ static void aspeed_gpio_write_index_mode(void *opaque, hwaddr offset,
> >               __func__, offset, data, reg_idx_type);
> >           return;
> >       }
> > -    aspeed_gpio_update(s, set, set->data_value);
> > +    aspeed_gpio_update(s, set, set->data_value, false);
> >       return;
> >   }
> > @@ -799,7 +801,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, false);
> >           return;
> >       case gpio_reg_direction:
> >           /*
> > @@ -875,7 +877,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t data,
> >                         PRIx64"\n", __func__, offset);
> >           return;
> >       }
> > -    aspeed_gpio_update(s, set, set->data_value);
> > +    aspeed_gpio_update(s, set, set->data_value, false);
> >       return;
> >   }
> 

Re: [PATCH 1/2] hw/gpio/aspeed: Don't let guests modify input pins

[Peter Delevoryas]

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.

> 
> Thanks,
> Peter
> 
> > 
> > Thanks,
> > 
> > C.
> > 
> > > 
> > > Signed-off-by: Peter Delevoryas <peter@pjd.dev>
> > > Fixes: 4b7f956862dc ("hw/gpio: Add basic Aspeed GPIO model for AST2400 and AST2500")
> > > ---
> > >   hw/gpio/aspeed_gpio.c | 22 ++++++++++++----------
> > >   1 file changed, 12 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> > > index a62a673857..2eae427201 100644
> > > --- a/hw/gpio/aspeed_gpio.c
> > > +++ b/hw/gpio/aspeed_gpio.c
> > > @@ -268,7 +268,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, bool force)
> > >   {
> > >       uint32_t input_mask = regs->input_mask;
> > >       uint32_t direction = regs->direction;
> > > @@ -293,10 +293,12 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
> > >               }
> > >               /* ...then update the state. */
> > > -            if (mask & new) {
> > > -                regs->data_value |= mask;
> > > -            } else {
> > > -                regs->data_value &= ~mask;
> > > +            if (direction & mask || force) {
> > > +                if (mask & new) {
> > > +                    regs->data_value |= mask;
> > > +                } else {
> > > +                    regs->data_value &= ~mask;
> > > +                }
> > >               }
> > >               /* If the gpio is set to output... */
> > > @@ -339,7 +341,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, true);
> > >   }
> > >   /*
> > > @@ -653,7 +655,7 @@ static void aspeed_gpio_write_index_mode(void *opaque, hwaddr offset,
> > >           reg_value = update_value_control_source(set, set->data_value,
> > >                                                   reg_value);
> > >           set->data_read = reg_value;
> > > -        aspeed_gpio_update(s, set, reg_value);
> > > +        aspeed_gpio_update(s, set, reg_value, false);
> > >           return;
> > >       case gpio_reg_idx_direction:
> > >           reg_value = set->direction;
> > > @@ -753,7 +755,7 @@ static void aspeed_gpio_write_index_mode(void *opaque, hwaddr offset,
> > >               __func__, offset, data, reg_idx_type);
> > >           return;
> > >       }
> > > -    aspeed_gpio_update(s, set, set->data_value);
> > > +    aspeed_gpio_update(s, set, set->data_value, false);
> > >       return;
> > >   }
> > > @@ -799,7 +801,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, false);
> > >           return;
> > >       case gpio_reg_direction:
> > >           /*
> > > @@ -875,7 +877,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t data,
> > >                         PRIx64"\n", __func__, offset);
> > >           return;
> > >       }
> > > -    aspeed_gpio_update(s, set, set->data_value);
> > > +    aspeed_gpio_update(s, set, set->data_value, false);
> > >       return;
> > >   }
> > 
> 

Re: [PATCH 1/2] hw/gpio/aspeed: Don't let guests modify input pins

[Cédric Le Goater]

On 7/7/22 21:04, 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.

I am not an expert of the GPIO controller. Andrew may be ?

Anyhow, anything that can help tracking invalid software operations
is good to have and it seems that your patch is trying to fix one.
Hence my suggestion to add some logging where it makes sense.

Thanks,

C.

Re: [PATCH 1/2] hw/gpio/aspeed: Don't let guests modify input pins

[Andrew Jeffery]

On Thu, 7 Jul 2022, at 17:50, Joel Stanley wrote:
> On Thu, 7 Jul 2022 at 07:17, Peter Delevoryas <peter@pjd.dev> 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).
>>
>> Signed-off-by: Peter Delevoryas <peter@pjd.dev>
>> Fixes: 4b7f956862dc ("hw/gpio: Add basic Aspeed GPIO model for AST2400 and AST2500")
>> ---
>>  hw/gpio/aspeed_gpio.c | 22 ++++++++++++----------
>>  1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
>> index a62a673857..2eae427201 100644
>> --- a/hw/gpio/aspeed_gpio.c
>> +++ b/hw/gpio/aspeed_gpio.c
>> @@ -268,7 +268,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, bool force)
>>  {
>>      uint32_t input_mask = regs->input_mask;
>>      uint32_t direction = regs->direction;
>> @@ -293,10 +293,12 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
>>              }
>>
>
> Reading this model hurts my head a little. Perhaps we also need to add
> a test for this case to make it clear what's going on.
>
> The test above the code you've changed does this:
>
>>            /* ...and we're output or not input-masked... */
>>            if (!(direction & mask) && (input_mask & mask)) {
>>                continue;
>>            }
>
> For clarity, !(direction & mask) means "is input".
>
> The comment confuses me because it says "or", but the code has "and".

The comment documents what conditions we need before we actually go and set the
output value.

The test is whether we fail to meet those conditions.

If the condition evaluates true we don't want to modify the GPIO value, so we
`continue` to the next loop iteration to test the next GPIO.

>
> input_mask doesn't seem to feature in the Linux driver, so that will
> always be zero. The test will be X && 0, which is always 0.
>
> If you changed it to || then we would do the test that the comment
> suggests. When the pin is input, we will skip updating the value.

The condition shouldn't change, rather if the comment makes things more
confusing rather than less, we should change that instead.

Andrew

Re: [PATCH 1/2] hw/gpio/aspeed: Don't let guests modify input pins

[Andrew Jeffery]

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);
     return;
 }
 

Re: [PATCH 1/2] hw/gpio/aspeed: Don't let guests modify input pins

[Peter Delevoryas]

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;
>  }
>  
> 

Re: [PATCH 1/2] hw/gpio/aspeed: Don't let guests modify input pins

[Andrew Jeffery]

I think we've sorted this out, but replying to finalise the conversation

On Tue, 12 Jul 2022, at 11:27, Peter Delevoryas wrote:
> On Mon, Jul 11, 2022 at 10:56:08PM +0930, Andrew Jeffery wrote:
>>  
>>  /*
>> @@ -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?

The set->direction case is handled in the top hunk which handles the 
data register write. Note that it performs an early return.

The bottom hunk deals with making the value register consistent when 
we've updated any register that isn't the data register.

Andrew