| Message ID | 20230122170724.21868-4-shentey@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI controller cleanup | expand |
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"); > }
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. > >> }
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. > > > > >> } >
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. >> >> > >> >> } >> >
diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h index d41866a229..22471a1b9d 100644 --- a/include/hw/acpi/ich9.h +++ b/include/hw/acpi/ich9.h @@ -49,7 +49,6 @@ typedef struct ICH9LPCPMRegs { qemu_irq irq; /* SCI */ - uint32_t pm_io_base; Notifier powerdown_notifier; bool cpu_hotplug_legacy; diff --git a/include/hw/acpi/piix4.h b/include/hw/acpi/piix4.h index be1f8ea80e..62e1925a1f 100644 --- a/include/hw/acpi/piix4.h +++ b/include/hw/acpi/piix4.h @@ -39,8 +39,6 @@ struct PIIX4PMState { /*< public >*/ MemoryRegion io; - uint32_t io_base; - MemoryRegion io_gpe; ACPIREGS ar; diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c index 0313e71e74..f8af238974 100644 --- a/hw/acpi/ich9.c +++ b/hw/acpi/ich9.c @@ -126,17 +126,16 @@ void ich9_pm_iospace_update(ICH9LPCPMRegs *pm, uint32_t pm_io_base) assert((pm_io_base & ICH9_PMIO_MASK) == 0); - pm->pm_io_base = pm_io_base; memory_region_transaction_begin(); - memory_region_set_enabled(&pm->io, pm->pm_io_base != 0); - memory_region_set_address(&pm->io, pm->pm_io_base); + memory_region_set_enabled(&pm->io, pm_io_base != 0); + memory_region_set_address(&pm->io, pm_io_base); memory_region_transaction_commit(); } static int ich9_pm_post_load(void *opaque, int version_id) { ICH9LPCPMRegs *pm = opaque; - ich9_pm_iospace_update(pm, pm->pm_io_base); + ich9_pm_iospace_update(pm, pm->io.addr); return 0; } @@ -349,9 +348,9 @@ static void ich9_pm_get_gpe0_blk(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { ICH9LPCPMRegs *pm = opaque; - uint32_t value = pm->pm_io_base + ICH9_PMIO_GPE0_STS; + uint64_t value = pm->io.addr + ICH9_PMIO_GPE0_STS; - visit_type_uint32(v, name, &value, errp); + visit_type_uint64(v, name, &value, errp); } static bool ich9_pm_get_memory_hotplug_support(Object *obj, Error **errp) @@ -440,9 +439,9 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm) pm->keep_pci_slot_hpc = true; pm->enable_tco = true; - object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE, - &pm->pm_io_base, OBJ_PROP_FLAG_READ); - object_property_add(obj, ACPI_PM_PROP_GPE0_BLK, "uint32", + object_property_add_uint64_ptr(obj, ACPI_PM_PROP_PM_IO_BASE, + &pm->io.addr, OBJ_PROP_FLAG_READ); + object_property_add(obj, ACPI_PM_PROP_GPE0_BLK, "uint64", ich9_pm_get_gpe0_blk, NULL, NULL, pm); object_property_add_uint8_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN, 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); 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); } static void piix4_pm_realize(PCIDevice *dev, Error **errp)
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(-)