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