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

[Cédric Le Goater]

On 7/12/22 04:32, Peter Delevoryas wrote:
> Up until now, guests could modify input pins by overwriting the data
> value register. The guest OS should only be allowed to modify output pin
> values, and the QOM property setter should only be permitted to modify
> input pins.
>
> This change also updates the gpio input pin test to match this
> expectation.
>
> Andrew suggested this particularly refactoring here:
>
>      
> https://lore.kernel.org/qemu-devel/23523aa1-ba81-412b-92cc-8174faba3612@www.fastmail.com/
>
> Suggested-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Peter Delevoryas <peter@pjd.dev>
> Fixes: 4b7f956862dc ("hw/gpio: Add basic Aspeed GPIO model for AST2400 and 
> AST2500")
> ---


Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


>   hw/gpio/aspeed_gpio.c          | 15 ++++++++-------
>   tests/qtest/aspeed_gpio-test.c |  2 +-
>   2 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> index a62a673857..1e267dd482 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, uint32_t mode_mask)
>   {
>       uint32_t input_mask = regs->input_mask;
>       uint32_t direction = regs->direction;
> @@ -277,7 +277,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;
> @@ -339,7 +340,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);
>   }
>   
>   /*
> @@ -653,7 +654,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, set->direction);
>           return;
>       case gpio_reg_idx_direction:
>           reg_value = set->direction;
> @@ -753,7 +754,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, UINT32_MAX);
>       return;
>   }
>   
> @@ -799,7 +800,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:
>           /*
> @@ -875,7 +876,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, UINT32_MAX);
>       return;
>   }
>   
> diff --git a/tests/qtest/aspeed_gpio-test.c b/tests/qtest/aspeed_gpio-test.c
> index 8f52454099..d38f51d719 100644
> --- a/tests/qtest/aspeed_gpio-test.c
> +++ b/tests/qtest/aspeed_gpio-test.c
> @@ -69,7 +69,7 @@ static void test_set_input_pins(const void *data)
>   
>       qtest_writel(s, AST2600_GPIO_BASE + GPIO_ABCD_DATA_VALUE, 0x00000000);
>       value = qtest_readl(s, AST2600_GPIO_BASE + GPIO_ABCD_DATA_VALUE);
> -    g_assert_cmphex(value, ==, 0x00000000);
> +    g_assert_cmphex(value, ==, 0xffffffff);
>   }
>   
>   int main(int argc, char **argv)