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

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


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