Re: [PATCH 3/7] hw/acpi/{ich9,piix4}: Resolve redundant io_base address attributes

[Bernhard Beschow]

Am 23. Januar 2023 07:57:08 UTC schrieb "Philippe Mathieu-Daudé" 
<philmd@linaro.org>:
>Hi Bernhard,
>
>On 22/1/23 18:07, Bernhard Beschow wrote:
>> A MemoryRegion has an addr attribute which gets set to the same values
>> as the redundant io_addr attributes.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   include/hw/acpi/ich9.h  |  1 -
>>   include/hw/acpi/piix4.h |  2 --
>>   hw/acpi/ich9.c          | 17 ++++++++---------
>>   hw/acpi/piix4.c         | 11 ++++++-----
>>   4 files changed, 14 insertions(+), 17 deletions(-)
>
>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> index 370b34eacf..2e9bc63fca 100644
>> --- a/hw/acpi/piix4.c
>> +++ b/hw/acpi/piix4.c
>> @@ -91,13 +91,14 @@ static void apm_ctrl_changed(uint32_t val, void *arg)
>>   static void pm_io_space_update(PIIX4PMState *s)
>>   {
>>       PCIDevice *d = PCI_DEVICE(s);
>> +    uint32_t io_base;
>>   -    s->io_base = le32_to_cpu(*(uint32_t *)(d->config + 0x40));
>> -    s->io_base &= 0xffc0;
>> +    io_base = le32_to_cpu(*(uint32_t *)(d->config + 0x40));
>> +    io_base &= 0xffc0;
>>         memory_region_transaction_begin();
>>       memory_region_set_enabled(&s->io, d->config[0x80] & 1);
>> -    memory_region_set_address(&s->io, s->io_base);
>> +    memory_region_set_address(&s->io, io_base);
>
>OK for this part.
>
>>       memory_region_transaction_commit();
>>   }
>>   @@ -433,8 +434,8 @@ static void piix4_pm_add_properties(PIIX4PMState *s)
>>                                     &s->ar.gpe.len, OBJ_PROP_FLAG_READ);
>>       object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
>>                                     &sci_int, OBJ_PROP_FLAG_READ);
>> -    object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
>> -                                  &s->io_base, OBJ_PROP_FLAG_READ);
>> +    object_property_add_uint64_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
>> +                                   &s->io.addr, OBJ_PROP_FLAG_READ);
>
>+Eduardo/Mark
>
>We shouldn't do that IMO, because we access an internal field from
>another QOM object.
>
>We can however alias the MR property:
>
>  object_property_add_alias(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
>                            OBJECT(&s->io), "addr");

Indeed! And the "addr" property is already read-only -- which seems like a good 
fit.

>
>>   }