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

[Bernhard Beschow]

Am 24. Januar 2023 16:05:40 UTC schrieb Igor Mammedov <imammedo@redhat.com>:
>s/resolve/remove|drop/
>
>On Mon, 23 Jan 2023 15:49:29 +0000
>Bernhard Beschow <shentey@gmail.com> wrote:
>
>> 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.
>
>
>MemoryRegion::addr is an offset within subregion while fields you
>are removing are absolute values (offset within address space).
>
>Assuming that the former is the same as the later seems wrong
>to me (even if they both match at the moment).
>So I'd drop this patch.

The device models try hard to keep those in sync. I'd rather avoid having two 
sources of truth, so I think there is merit in keeping this patch and split it 
in two.

Note that in addition, the base addresses are also present in PCI config space 
which is yet another source of truth. The config space is preserved during 
migration, and I meanwhile noticed that the addresses are recovered from there. 
This makes the io_addr attributes seem even more redundant.

There is already a memory_region_to_absolute_addr() which would fit well here. 
It would need to be exported, and I wonder if it isn't for a reason? Any 
thoughts?

>
>
>> >> 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");  
>
>also, do not access 'io.addr' directly elsewhere in the patch either.
>
>> 
>> Indeed! And the "addr" property is already read-only -- which seems like a 
>> good fit.
>> 
>> >  
>> >>   }  
>> 
>