[PATCH 0/7] ACPI controller cleanup

[PATCH 0/7] ACPI controller cleanup

[Bernhard Beschow]

This series brings the PIIX4 PM device closer to reality and resolves some
redundant code along the way.

Testing done:
- `make check`
- Starting a live CD under pc and q35 machines and check that the GPE accesses
  are traced

Bernhard Beschow (7):
  hw/acpi/{ich9,piix4}: Reuse existing attributes for QOM properties
  hw/acpi/ich9: Remove unneeded assignments
  hw/acpi/{ich9,piix4}: Resolve redundant io_base address attributes
  hw/acpi/ich9: Use ICH9_PMIO_GPE0_STS just once
  hw/acpi/piix4: Fix offset of GPE0 registers
  hw/acpi: Trace GPE access in all device models, not just PIIX4
  hw/acpi/core: Trace enable and status registers of GPE separately

 include/hw/acpi/ich9.h  |  1 -
 include/hw/acpi/piix4.h |  3 +--
 hw/acpi/core.c          |  9 +++++++++
 hw/acpi/ich9.c          | 26 ++++++++++++--------------
 hw/acpi/piix4.c         | 31 ++++++++++++++++---------------
 hw/acpi/trace-events    | 10 ++++++----
 6 files changed, 44 insertions(+), 36 deletions(-)

-- 
2.39.1

[PATCH 1/7] hw/acpi/{ich9, piix4}: Reuse existing attributes for QOM properties

[Bernhard Beschow]

The QOM properties are accessed after the device models have been
realized. This means that the constants are redundant. Remove them.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/acpi/ich9.c  |  5 ++---
 hw/acpi/piix4.c | 10 ++++------
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index a93c470e9d..2050af67b9 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -433,7 +433,6 @@ static void ich9_pm_set_keep_pci_slot_hpc(Object *obj, bool value, Error **errp)
 
 void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
 {
-    static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
     pm->acpi_memory_hotplug.is_enabled = true;
     pm->cpu_hotplug_legacy = true;
     pm->disable_s3 = 0;
@@ -448,8 +447,8 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
     object_property_add(obj, ACPI_PM_PROP_GPE0_BLK, "uint32",
                         ich9_pm_get_gpe0_blk,
                         NULL, NULL, pm);
-    object_property_add_uint32_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
-                                   &gpe0_len, OBJ_PROP_FLAG_READ);
+    object_property_add_uint8_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
+                                  &pm->acpi_regs.gpe.len, OBJ_PROP_FLAG_READ);
     object_property_add_bool(obj, "memory-hotplug-support",
                              ich9_pm_get_memory_hotplug_support,
                              ich9_pm_set_memory_hotplug_support);
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 0a81f1ad93..370b34eacf 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -421,18 +421,16 @@ static void piix4_pm_add_properties(PIIX4PMState *s)
 {
     static const uint8_t acpi_enable_cmd = ACPI_ENABLE;
     static const uint8_t acpi_disable_cmd = ACPI_DISABLE;
-    static const uint32_t gpe0_blk = GPE_BASE;
-    static const uint32_t gpe0_blk_len = GPE_LEN;
     static const uint16_t sci_int = 9;
 
     object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_ENABLE_CMD,
                                   &acpi_enable_cmd, OBJ_PROP_FLAG_READ);
     object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
                                   &acpi_disable_cmd, OBJ_PROP_FLAG_READ);
-    object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
-                                  &gpe0_blk, OBJ_PROP_FLAG_READ);
-    object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
-                                  &gpe0_blk_len, OBJ_PROP_FLAG_READ);
+    object_property_add_uint64_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
+                                   &s->io_gpe.addr, OBJ_PROP_FLAG_READ);
+    object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
+                                  &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,
-- 
2.39.1

Re: [PATCH 1/7] hw/acpi/{ich9,piix4}: Reuse existing attributes for QOM properties

[Igor Mammedov]

On Sun, 22 Jan 2023 18:07:18 +0100
Bernhard Beschow <shentey@gmail.com> wrote:

> The QOM properties are accessed after the device models have been
> realized. This means that the constants are redundant. Remove them.
not sure it above means.

Perhaps:

subj: use existing fields type::foo instead of static memory.

all object_property_add_*_ptr() needs is a pointer to memory
storing so RO defaults where provided as pointers to static
constants. Instead of keeping static constants around, drop
them and initialize use existing type::foo field, which were
set set later on to this constant later at ...

(also see below: maybe squash ICH9_PMIO_GPE0_LEN cleanup here as well)

or something along this lines. 

> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>  hw/acpi/ich9.c  |  5 ++---
>  hw/acpi/piix4.c | 10 ++++------
>  2 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index a93c470e9d..2050af67b9 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -433,7 +433,6 @@ static void ich9_pm_set_keep_pci_slot_hpc(Object *obj, bool value, Error **errp)
>  
>  void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
>  {
> -    static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;

you are loosing default value here, (true it's set somewhere later)
suggest:
      pm->acpi_regs.gpe.len = ICH9_PMIO_GPE0_LEN;

and in patch on top maybe cleanup other places that use
ICH9_PMIO_GPE0_LEN with pm->acpi_regs.gpe.len

>      pm->acpi_memory_hotplug.is_enabled = true;
>      pm->cpu_hotplug_legacy = true;
>      pm->disable_s3 = 0;
> @@ -448,8 +447,8 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
>      object_property_add(obj, ACPI_PM_PROP_GPE0_BLK, "uint32",
>                          ich9_pm_get_gpe0_blk,
>                          NULL, NULL, pm);
> -    object_property_add_uint32_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
> -                                   &gpe0_len, OBJ_PROP_FLAG_READ);
> +    object_property_add_uint8_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
> +                                  &pm->acpi_regs.gpe.len, OBJ_PROP_FLAG_READ);
>      object_property_add_bool(obj, "memory-hotplug-support",
>                               ich9_pm_get_memory_hotplug_support,
>                               ich9_pm_set_memory_hotplug_support);
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 0a81f1ad93..370b34eacf 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -421,18 +421,16 @@ static void piix4_pm_add_properties(PIIX4PMState *s)
>  {
>      static const uint8_t acpi_enable_cmd = ACPI_ENABLE;
>      static const uint8_t acpi_disable_cmd = ACPI_DISABLE;
> -    static const uint32_t gpe0_blk = GPE_BASE;
> -    static const uint32_t gpe0_blk_len = GPE_LEN;

ditto

also maybe split on 2 patches 1 for ich9 another for piix4

>      static const uint16_t sci_int = 9;
>  
>      object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_ENABLE_CMD,
>                                    &acpi_enable_cmd, OBJ_PROP_FLAG_READ);
>      object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
>                                    &acpi_disable_cmd, OBJ_PROP_FLAG_READ);
> -    object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
> -                                  &gpe0_blk, OBJ_PROP_FLAG_READ);
> -    object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
> -                                  &gpe0_blk_len, OBJ_PROP_FLAG_READ);
> +    object_property_add_uint64_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
> +                                   &s->io_gpe.addr, OBJ_PROP_FLAG_READ);
> +    object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
> +                                  &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,

[PATCH 2/7] hw/acpi/ich9: Remove unneeded assignments

[Bernhard Beschow]

The first thing ich9_pm_iospace_update() does is to set pm->pm_io_base to
the pm_io_base parameter. The pm_io_base parameter's value is the old
one of pm->pm_io_base.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/acpi/ich9.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 2050af67b9..0313e71e74 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -136,9 +136,7 @@ void ich9_pm_iospace_update(ICH9LPCPMRegs *pm, uint32_t pm_io_base)
 static int ich9_pm_post_load(void *opaque, int version_id)
 {
     ICH9LPCPMRegs *pm = opaque;
-    uint32_t pm_io_base = pm->pm_io_base;
-    pm->pm_io_base = 0;
-    ich9_pm_iospace_update(pm, pm_io_base);
+    ich9_pm_iospace_update(pm, pm->pm_io_base);
     return 0;
 }
 
-- 
2.39.1

Re: [PATCH 2/7] hw/acpi/ich9: Remove unneeded assignments

[Igor Mammedov]

On Sun, 22 Jan 2023 18:07:19 +0100
Bernhard Beschow <shentey@gmail.com> wrote:

> The first thing ich9_pm_iospace_update() does is to set pm->pm_io_base to
> the pm_io_base parameter.
try to explain why 'pm->pm_io_base = 0' was there , what's changed 
and then why it's no longer needed.

> The pm_io_base parameter's value is the old
> one of pm->pm_io_base.
I can't parse this sentence.


fixes: cacaab8bdd7460

> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>  hw/acpi/ich9.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 2050af67b9..0313e71e74 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -136,9 +136,7 @@ void ich9_pm_iospace_update(ICH9LPCPMRegs *pm, uint32_t pm_io_base)
>  static int ich9_pm_post_load(void *opaque, int version_id)
>  {
>      ICH9LPCPMRegs *pm = opaque;
> -    uint32_t pm_io_base = pm->pm_io_base;
> -    pm->pm_io_base = 0;
> -    ich9_pm_iospace_update(pm, pm_io_base);
> +    ich9_pm_iospace_update(pm, pm->pm_io_base);
>      return 0;
>  }
>  

Re: [PATCH 2/7] hw/acpi/ich9: Remove unneeded assignments

[Bernhard Beschow]

Am 24. Januar 2023 16:55:37 UTC schrieb Igor Mammedov <imammedo@redhat.com>:
>On Sun, 22 Jan 2023 18:07:19 +0100
>Bernhard Beschow <shentey@gmail.com> wrote:
>
>> The first thing ich9_pm_iospace_update() does is to set pm->pm_io_base to
>> the pm_io_base parameter.
>try to explain why 'pm->pm_io_base = 0' was there , what's changed 
>and then why it's no longer needed.

Meanwhile I think the whole function is a no-op and can be removed. Some archeology will explain how it came to be.

>
>> The pm_io_base parameter's value is the old
>> one of pm->pm_io_base.
>I can't parse this sentence.
>
>
>fixes: cacaab8bdd7460
>
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>  hw/acpi/ich9.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>> 
>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>> index 2050af67b9..0313e71e74 100644
>> --- a/hw/acpi/ich9.c
>> +++ b/hw/acpi/ich9.c
>> @@ -136,9 +136,7 @@ void ich9_pm_iospace_update(ICH9LPCPMRegs *pm, uint32_t pm_io_base)
>>  static int ich9_pm_post_load(void *opaque, int version_id)
>>  {
>>      ICH9LPCPMRegs *pm = opaque;
>> -    uint32_t pm_io_base = pm->pm_io_base;
>> -    pm->pm_io_base = 0;
>> -    ich9_pm_iospace_update(pm, pm_io_base);
>> +    ich9_pm_iospace_update(pm, pm->pm_io_base);
>>      return 0;
>>  }
>>  
>

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

[Bernhard Beschow]

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/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)
-- 
2.39.1

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

[Philippe Mathieu-Daudé]

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");

>   }

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.

>
>>   }

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

[Igor Mammedov]

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

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

[PATCH 4/7] hw/acpi/ich9: Use ICH9_PMIO_GPE0_STS just once

[Bernhard Beschow]

Cosmetic change emphasizing that always the actual address of the gpe0
block is returned.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/acpi/ich9.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index f8af238974..40a20e01ea 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -348,7 +348,9 @@ static void ich9_pm_get_gpe0_blk(Object *obj, Visitor *v, const char *name,
                                  void *opaque, Error **errp)
 {
     ICH9LPCPMRegs *pm = opaque;
-    uint64_t value = pm->io.addr + ICH9_PMIO_GPE0_STS;
+    uint64_t value = pm->io.addr + pm->io_gpe.addr;
+
+    assert(&pm->io == pm->io_gpe.container);
 
     visit_type_uint64(v, name, &value, errp);
 }
-- 
2.39.1

Re: [PATCH 4/7] hw/acpi/ich9: Use ICH9_PMIO_GPE0_STS just once

[Igor Mammedov]

On Sun, 22 Jan 2023 18:07:21 +0100
Bernhard Beschow <shentey@gmail.com> wrote:

> Cosmetic change emphasizing 

... how 3/7 makes it more confusing
  
> that always the actual address of the gpe0
> block is returned.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>  hw/acpi/ich9.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index f8af238974..40a20e01ea 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -348,7 +348,9 @@ static void ich9_pm_get_gpe0_blk(Object *obj, Visitor *v, const char *name,
>                                   void *opaque, Error **errp)
>  {
>      ICH9LPCPMRegs *pm = opaque;
> -    uint64_t value = pm->io.addr + ICH9_PMIO_GPE0_STS;
> +    uint64_t value = pm->io.addr + pm->io_gpe.addr;

do not poke inside of MemoryRegion, right API to use here
if you really want to get it from memory region would be
  
memory_region_find && MemoryRegionSection::offset_within_address_space
magic

I'd just drop 3/7 and this patch

> +
> +    assert(&pm->io == pm->io_gpe.container);
>  
>      visit_type_uint64(v, name, &value, errp);
>  }

[PATCH 5/7] hw/acpi/piix4: Fix offset of GPE0 registers

[Bernhard Beschow]

The PIIX4 datasheet defines the GPSTS register to be at offset 0x0c of the
power management I/O register block. This register block is represented
in the device model by the io attribute. So make io_gpe a child memory
region of io at offset 0x0c.

Note that SeaBIOS sets the base address of the register block to 0x600,
resulting in the io_gpe block to start at 0x60c. GPE_BASE is defined as
0xafe0 which is 0xa9d4 bytes off. In order to preserve compatibilty,
create an io_gpe_qemu memory region alias at GPE_BASE.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/acpi/piix4.h | 1 +
 hw/acpi/piix4.c         | 9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/hw/acpi/piix4.h b/include/hw/acpi/piix4.h
index 62e1925a1f..4e6cad9e8c 100644
--- a/include/hw/acpi/piix4.h
+++ b/include/hw/acpi/piix4.h
@@ -40,6 +40,7 @@ struct PIIX4PMState {
 
     MemoryRegion io;
     MemoryRegion io_gpe;
+    MemoryRegion io_gpe_qemu;
     ACPIREGS ar;
 
     APMState apm;
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 2e9bc63fca..836f9026b1 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -49,6 +49,7 @@
 #include "qom/object.h"
 
 #define GPE_BASE 0xafe0
+#define GPE_OFS 0xc
 #define GPE_LEN 4
 
 #define ACPI_PCIHP_ADDR_PIIX4 0xae00
@@ -429,7 +430,7 @@ static void piix4_pm_add_properties(PIIX4PMState *s)
     object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
                                   &acpi_disable_cmd, OBJ_PROP_FLAG_READ);
     object_property_add_uint64_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
-                                   &s->io_gpe.addr, OBJ_PROP_FLAG_READ);
+                                   &s->io_gpe_qemu.addr, OBJ_PROP_FLAG_READ);
     object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
                                   &s->ar.gpe.len, OBJ_PROP_FLAG_READ);
     object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
@@ -558,7 +559,11 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
 {
     memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s,
                           "acpi-gpe0", GPE_LEN);
-    memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
+    memory_region_add_subregion(&s->io, GPE_OFS, &s->io_gpe);
+
+    memory_region_init_alias(&s->io_gpe_qemu, OBJECT(s), "acpi-gpe0-qemu",
+                             &s->io_gpe, 0, memory_region_size(&s->io_gpe));
+    memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe_qemu);
 
     if (s->use_acpi_hotplug_bridge || s->use_acpi_root_pci_hotplug) {
         acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
-- 
2.39.1

Re: [PATCH 5/7] hw/acpi/piix4: Fix offset of GPE0 registers

[Igor Mammedov]

On Sun, 22 Jan 2023 18:07:22 +0100
Bernhard Beschow <shentey@gmail.com> wrote:

> The PIIX4 datasheet defines the GPSTS register to be at offset 0x0c of the
> power management I/O register block. This register block is represented
> in the device model by the io attribute. So make io_gpe a child memory
> region of io at offset 0x0c.

to what end?

> Note that SeaBIOS sets the base address of the register block to 0x600,
> resulting in the io_gpe block to start at 0x60c. GPE_BASE is defined as
> 0xafe0 which is 0xa9d4 bytes off. In order to preserve compatibilty,
> create an io_gpe_qemu memory region alias at GPE_BASE.

qemu's io_gpe != piix4(GPSTS)
QEMU simply doesn't implement piix4(GPSTS), instead it has implemented
custom GPE registers block at 0xafe0 for its hotplug purposes.
Bits in both GPE blocks have different meaning,
so moving io_gpe to PMBASE+0x0c, would be a bug.

Interesting question is what guest gets now when it reads
PMBASE+0x0c ?

If reads return -1 and guest uses these
registers it might get confused since all STS/EN bits
are set and writes are ignored. We likely get away
with it since these registers aren't used by non ACPI guests
(non x86 ones) and x86 ones fetch GPE block from FADT
table => not using piix4(GPSTS) at all.
So It's a bug to fix (at least make it read as 0s)


> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>  include/hw/acpi/piix4.h | 1 +
>  hw/acpi/piix4.c         | 9 +++++++--
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/acpi/piix4.h b/include/hw/acpi/piix4.h
> index 62e1925a1f..4e6cad9e8c 100644
> --- a/include/hw/acpi/piix4.h
> +++ b/include/hw/acpi/piix4.h
> @@ -40,6 +40,7 @@ struct PIIX4PMState {
>  
>      MemoryRegion io;
>      MemoryRegion io_gpe;
> +    MemoryRegion io_gpe_qemu;
>      ACPIREGS ar;
>  
>      APMState apm;
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 2e9bc63fca..836f9026b1 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -49,6 +49,7 @@
>  #include "qom/object.h"
>  
>  #define GPE_BASE 0xafe0
> +#define GPE_OFS 0xc
>  #define GPE_LEN 4
>  
>  #define ACPI_PCIHP_ADDR_PIIX4 0xae00
> @@ -429,7 +430,7 @@ static void piix4_pm_add_properties(PIIX4PMState *s)
>      object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
>                                    &acpi_disable_cmd, OBJ_PROP_FLAG_READ);
>      object_property_add_uint64_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
> -                                   &s->io_gpe.addr, OBJ_PROP_FLAG_READ);
> +                                   &s->io_gpe_qemu.addr, OBJ_PROP_FLAG_READ);
>      object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
>                                    &s->ar.gpe.len, OBJ_PROP_FLAG_READ);
>      object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
> @@ -558,7 +559,11 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
>  {
>      memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s,
>                            "acpi-gpe0", GPE_LEN);
> -    memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
> +    memory_region_add_subregion(&s->io, GPE_OFS, &s->io_gpe);
> +
> +    memory_region_init_alias(&s->io_gpe_qemu, OBJECT(s), "acpi-gpe0-qemu",
> +                             &s->io_gpe, 0, memory_region_size(&s->io_gpe));
> +    memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe_qemu);
>  
>      if (s->use_acpi_hotplug_bridge || s->use_acpi_root_pci_hotplug) {
>          acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,

Re: [PATCH 5/7] hw/acpi/piix4: Fix offset of GPE0 registers

[Bernhard Beschow]

Am 25. Januar 2023 15:55:01 UTC schrieb Igor Mammedov <imammedo@redhat.com>:
>On Sun, 22 Jan 2023 18:07:22 +0100
>Bernhard Beschow <shentey@gmail.com> wrote:
>
>> The PIIX4 datasheet defines the GPSTS register to be at offset 0x0c of the
>> power management I/O register block. This register block is represented
>> in the device model by the io attribute. So make io_gpe a child memory
>> region of io at offset 0x0c.
>
>to what end?
>
>> Note that SeaBIOS sets the base address of the register block to 0x600,
>> resulting in the io_gpe block to start at 0x60c. GPE_BASE is defined as
>> 0xafe0 which is 0xa9d4 bytes off. In order to preserve compatibilty,
>> create an io_gpe_qemu memory region alias at GPE_BASE.
>
>qemu's io_gpe != piix4(GPSTS)
>QEMU simply doesn't implement piix4(GPSTS), instead it has implemented
>custom GPE registers block at 0xafe0 for its hotplug purposes.
>Bits in both GPE blocks have different meaning,
>so moving io_gpe to PMBASE+0x0c, would be a bug.
>
>Interesting question is what guest gets now when it reads
>PMBASE+0x0c ?
>
>If reads return -1 and guest uses these
>registers it might get confused since all STS/EN bits
>are set and writes are ignored. We likely get away
>with it since these registers aren't used by non ACPI guests
>(non x86 ones) and x86 ones fetch GPE block from FADT
>table => not using piix4(GPSTS) at all.
>So It's a bug to fix (at least make it read as 0s)

I see. This wasn't obvious to me and I'll drop this patch.

How about renaming io_gpe to something communicating that this is purely a "Frankenstein" functionality, e.g. to io_gpe_qemu or io_gpe_hotplug? Any preferences?

>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>  include/hw/acpi/piix4.h | 1 +
>>  hw/acpi/piix4.c         | 9 +++++++--
>>  2 files changed, 8 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/hw/acpi/piix4.h b/include/hw/acpi/piix4.h
>> index 62e1925a1f..4e6cad9e8c 100644
>> --- a/include/hw/acpi/piix4.h
>> +++ b/include/hw/acpi/piix4.h
>> @@ -40,6 +40,7 @@ struct PIIX4PMState {
>>  
>>      MemoryRegion io;
>>      MemoryRegion io_gpe;
>> +    MemoryRegion io_gpe_qemu;
>>      ACPIREGS ar;
>>  
>>      APMState apm;
>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> index 2e9bc63fca..836f9026b1 100644
>> --- a/hw/acpi/piix4.c
>> +++ b/hw/acpi/piix4.c
>> @@ -49,6 +49,7 @@
>>  #include "qom/object.h"
>>  
>>  #define GPE_BASE 0xafe0
>> +#define GPE_OFS 0xc
>>  #define GPE_LEN 4
>>  
>>  #define ACPI_PCIHP_ADDR_PIIX4 0xae00
>> @@ -429,7 +430,7 @@ static void piix4_pm_add_properties(PIIX4PMState *s)
>>      object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
>>                                    &acpi_disable_cmd, OBJ_PROP_FLAG_READ);
>>      object_property_add_uint64_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
>> -                                   &s->io_gpe.addr, OBJ_PROP_FLAG_READ);
>> +                                   &s->io_gpe_qemu.addr, OBJ_PROP_FLAG_READ);
>>      object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
>>                                    &s->ar.gpe.len, OBJ_PROP_FLAG_READ);
>>      object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
>> @@ -558,7 +559,11 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
>>  {
>>      memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s,
>>                            "acpi-gpe0", GPE_LEN);
>> -    memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
>> +    memory_region_add_subregion(&s->io, GPE_OFS, &s->io_gpe);
>> +
>> +    memory_region_init_alias(&s->io_gpe_qemu, OBJECT(s), "acpi-gpe0-qemu",
>> +                             &s->io_gpe, 0, memory_region_size(&s->io_gpe));
>> +    memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe_qemu);
>>  
>>      if (s->use_acpi_hotplug_bridge || s->use_acpi_root_pci_hotplug) {
>>          acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
>

Re: [PATCH 5/7] hw/acpi/piix4: Fix offset of GPE0 registers

[Igor Mammedov]

On Sun, 29 Jan 2023 14:55:06 +0000
Bernhard Beschow <shentey@gmail.com> wrote:

> Am 25. Januar 2023 15:55:01 UTC schrieb Igor Mammedov <imammedo@redhat.com>:
> >On Sun, 22 Jan 2023 18:07:22 +0100
> >Bernhard Beschow <shentey@gmail.com> wrote:
> >  
> >> The PIIX4 datasheet defines the GPSTS register to be at offset 0x0c of the
> >> power management I/O register block. This register block is represented
> >> in the device model by the io attribute. So make io_gpe a child memory
> >> region of io at offset 0x0c.  
> >
> >to what end?
> >  
> >> Note that SeaBIOS sets the base address of the register block to 0x600,
> >> resulting in the io_gpe block to start at 0x60c. GPE_BASE is defined as
> >> 0xafe0 which is 0xa9d4 bytes off. In order to preserve compatibilty,
> >> create an io_gpe_qemu memory region alias at GPE_BASE.  
> >
> >qemu's io_gpe != piix4(GPSTS)
> >QEMU simply doesn't implement piix4(GPSTS), instead it has implemented
> >custom GPE registers block at 0xafe0 for its hotplug purposes.
> >Bits in both GPE blocks have different meaning,
> >so moving io_gpe to PMBASE+0x0c, would be a bug.
> >
> >Interesting question is what guest gets now when it reads
> >PMBASE+0x0c ?
> >
> >If reads return -1 and guest uses these
> >registers it might get confused since all STS/EN bits
> >are set and writes are ignored. We likely get away
> >with it since these registers aren't used by non ACPI guests
> >(non x86 ones) and x86 ones fetch GPE block from FADT
> >table => not using piix4(GPSTS) at all.
> >So It's a bug to fix (at least make it read as 0s)  
> 
> I see. This wasn't obvious to me and I'll drop this patch.
> 
> How about renaming io_gpe to something communicating that this is purely a "Frankenstein" functionality, e.g. to io_gpe_qemu or io_gpe_hotplug? Any preferences?

I don't think it's worth of effort, io_gpe is not worse that others.

> 
> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> >> ---
> >>  include/hw/acpi/piix4.h | 1 +
> >>  hw/acpi/piix4.c         | 9 +++++++--
> >>  2 files changed, 8 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/include/hw/acpi/piix4.h b/include/hw/acpi/piix4.h
> >> index 62e1925a1f..4e6cad9e8c 100644
> >> --- a/include/hw/acpi/piix4.h
> >> +++ b/include/hw/acpi/piix4.h
> >> @@ -40,6 +40,7 @@ struct PIIX4PMState {
> >>  
> >>      MemoryRegion io;
> >>      MemoryRegion io_gpe;
> >> +    MemoryRegion io_gpe_qemu;
> >>      ACPIREGS ar;
> >>  
> >>      APMState apm;
> >> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> >> index 2e9bc63fca..836f9026b1 100644
> >> --- a/hw/acpi/piix4.c
> >> +++ b/hw/acpi/piix4.c
> >> @@ -49,6 +49,7 @@
> >>  #include "qom/object.h"
> >>  
> >>  #define GPE_BASE 0xafe0
> >> +#define GPE_OFS 0xc
> >>  #define GPE_LEN 4
> >>  
> >>  #define ACPI_PCIHP_ADDR_PIIX4 0xae00
> >> @@ -429,7 +430,7 @@ static void piix4_pm_add_properties(PIIX4PMState *s)
> >>      object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
> >>                                    &acpi_disable_cmd, OBJ_PROP_FLAG_READ);
> >>      object_property_add_uint64_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
> >> -                                   &s->io_gpe.addr, OBJ_PROP_FLAG_READ);
> >> +                                   &s->io_gpe_qemu.addr, OBJ_PROP_FLAG_READ);
> >>      object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
> >>                                    &s->ar.gpe.len, OBJ_PROP_FLAG_READ);
> >>      object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
> >> @@ -558,7 +559,11 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> >>  {
> >>      memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s,
> >>                            "acpi-gpe0", GPE_LEN);
> >> -    memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
> >> +    memory_region_add_subregion(&s->io, GPE_OFS, &s->io_gpe);
> >> +
> >> +    memory_region_init_alias(&s->io_gpe_qemu, OBJECT(s), "acpi-gpe0-qemu",
> >> +                             &s->io_gpe, 0, memory_region_size(&s->io_gpe));
> >> +    memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe_qemu);
> >>  
> >>      if (s->use_acpi_hotplug_bridge || s->use_acpi_root_pci_hotplug) {
> >>          acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,  
> >  
> 

[PATCH 6/7] hw/acpi: Trace GPE access in all device models, not just PIIX4

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/acpi/core.c       | 5 +++++
 hw/acpi/piix4.c      | 3 ---
 hw/acpi/trace-events | 8 ++++----
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index 6da275c599..a33e410e69 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -32,6 +32,7 @@
 #include "qemu/module.h"
 #include "qemu/option.h"
 #include "sysemu/runstate.h"
+#include "trace.h"
 
 struct acpi_table_header {
     uint16_t _length;         /* our length, not actual part of the hdr */
@@ -686,6 +687,8 @@ void acpi_gpe_ioport_writeb(ACPIREGS *ar, uint32_t addr, uint32_t val)
 {
     uint8_t *cur;
 
+    trace_acpi_gpe_ioport_writeb(addr, val);
+
     cur = acpi_gpe_ioport_get_ptr(ar, addr);
     if (addr < ar->gpe.len / 2) {
         /* GPE_STS */
@@ -709,6 +712,8 @@ uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr)
         val = *cur;
     }
 
+    trace_acpi_gpe_ioport_readb(addr, val);
+
     return val;
 }
 
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 836f9026b1..b65ddb8e44 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -45,7 +45,6 @@
 #include "hw/acpi/acpi_dev_interface.h"
 #include "migration/vmstate.h"
 #include "hw/core/cpu.h"
-#include "trace.h"
 #include "qom/object.h"
 
 #define GPE_BASE 0xafe0
@@ -510,7 +509,6 @@ static uint64_t gpe_readb(void *opaque, hwaddr addr, unsigned width)
     PIIX4PMState *s = opaque;
     uint32_t val = acpi_gpe_ioport_readb(&s->ar, addr);
 
-    trace_piix4_gpe_readb(addr, width, val);
     return val;
 }
 
@@ -519,7 +517,6 @@ static void gpe_writeb(void *opaque, hwaddr addr, uint64_t val,
 {
     PIIX4PMState *s = opaque;
 
-    trace_piix4_gpe_writeb(addr, width, val);
     acpi_gpe_ioport_writeb(&s->ar, addr, val);
     acpi_update_sci(&s->ar, s->irq);
 }
diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events
index 78e0a8670e..159937ddb9 100644
--- a/hw/acpi/trace-events
+++ b/hw/acpi/trace-events
@@ -17,6 +17,10 @@ mhp_acpi_clear_remove_evt(uint32_t slot) "slot[0x%"PRIx32"] clear remove event"
 mhp_acpi_pc_dimm_deleted(uint32_t slot) "slot[0x%"PRIx32"] pc-dimm deleted"
 mhp_acpi_pc_dimm_delete_failed(uint32_t slot) "slot[0x%"PRIx32"] pc-dimm delete failed"
 
+# core.c
+acpi_gpe_ioport_readb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " ==> 0x%" PRIx8
+acpi_gpe_ioport_writeb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " <== 0x%" PRIx8
+
 # cpu.c
 cpuhp_acpi_invalid_idx_selected(uint32_t idx) "0x%"PRIx32
 cpuhp_acpi_read_flags(uint32_t idx, uint8_t flags) "idx[0x%"PRIx32"] flags: 0x%"PRIx8
@@ -48,10 +52,6 @@ acpi_pci_sel_read(uint32_t val) "%" PRIu32
 acpi_pci_ej_write(uint64_t addr, uint64_t data) "0x%" PRIx64 " <== %" PRIu64
 acpi_pci_sel_write(uint64_t addr, uint64_t data) "0x%" PRIx64 " <== %" PRIu64
 
-# piix4.c
-piix4_gpe_readb(uint64_t addr, unsigned width, uint64_t val) "addr: 0x%" PRIx64 " width: %d ==> 0x%" PRIx64
-piix4_gpe_writeb(uint64_t addr, unsigned width, uint64_t val) "addr: 0x%" PRIx64 " width: %d <== 0x%" PRIx64
-
 # tco.c
 tco_timer_reload(int ticks, int msec) "ticks=%d (%d ms)"
 tco_timer_expired(int timeouts_no, bool strap, bool no_reboot) "timeouts_no=%d no_reboot=%d/%d"
-- 
2.39.1

Re: [PATCH 6/7] hw/acpi: Trace GPE access in all device models, not just PIIX4

[Philippe Mathieu-Daudé]

On 22/1/23 18:07, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/acpi/core.c       | 5 +++++
>   hw/acpi/piix4.c      | 3 ---
>   hw/acpi/trace-events | 8 ++++----
>   3 files changed, 9 insertions(+), 7 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 6/7] hw/acpi: Trace GPE access in all device models, not just PIIX4

[Igor Mammedov]

On Sun, 22 Jan 2023 18:07:23 +0100
Bernhard Beschow <shentey@gmail.com> wrote:

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/acpi/core.c       | 5 +++++
>  hw/acpi/piix4.c      | 3 ---
>  hw/acpi/trace-events | 8 ++++----
>  3 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index 6da275c599..a33e410e69 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -32,6 +32,7 @@
>  #include "qemu/module.h"
>  #include "qemu/option.h"
>  #include "sysemu/runstate.h"
> +#include "trace.h"
>  
>  struct acpi_table_header {
>      uint16_t _length;         /* our length, not actual part of the hdr */
> @@ -686,6 +687,8 @@ void acpi_gpe_ioport_writeb(ACPIREGS *ar, uint32_t addr, uint32_t val)
>  {
>      uint8_t *cur;
>  
> +    trace_acpi_gpe_ioport_writeb(addr, val);
> +
>      cur = acpi_gpe_ioport_get_ptr(ar, addr);
>      if (addr < ar->gpe.len / 2) {
>          /* GPE_STS */
> @@ -709,6 +712,8 @@ uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr)
>          val = *cur;
>      }
>  
> +    trace_acpi_gpe_ioport_readb(addr, val);
> +
>      return val;
>  }
>  
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 836f9026b1..b65ddb8e44 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -45,7 +45,6 @@
>  #include "hw/acpi/acpi_dev_interface.h"
>  #include "migration/vmstate.h"
>  #include "hw/core/cpu.h"
> -#include "trace.h"
>  #include "qom/object.h"
>  
>  #define GPE_BASE 0xafe0
> @@ -510,7 +509,6 @@ static uint64_t gpe_readb(void *opaque, hwaddr addr, unsigned width)
>      PIIX4PMState *s = opaque;
>      uint32_t val = acpi_gpe_ioport_readb(&s->ar, addr);
>  
> -    trace_piix4_gpe_readb(addr, width, val);
>      return val;
>  }
>  
> @@ -519,7 +517,6 @@ static void gpe_writeb(void *opaque, hwaddr addr, uint64_t val,
>  {
>      PIIX4PMState *s = opaque;
>  
> -    trace_piix4_gpe_writeb(addr, width, val);
>      acpi_gpe_ioport_writeb(&s->ar, addr, val);
>      acpi_update_sci(&s->ar, s->irq);
>  }
> diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events
> index 78e0a8670e..159937ddb9 100644
> --- a/hw/acpi/trace-events
> +++ b/hw/acpi/trace-events
> @@ -17,6 +17,10 @@ mhp_acpi_clear_remove_evt(uint32_t slot) "slot[0x%"PRIx32"] clear remove event"
>  mhp_acpi_pc_dimm_deleted(uint32_t slot) "slot[0x%"PRIx32"] pc-dimm deleted"
>  mhp_acpi_pc_dimm_delete_failed(uint32_t slot) "slot[0x%"PRIx32"] pc-dimm delete failed"
>  
> +# core.c
> +acpi_gpe_ioport_readb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " ==> 0x%" PRIx8
> +acpi_gpe_ioport_writeb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " <== 0x%" PRIx8
> +
>  # cpu.c
>  cpuhp_acpi_invalid_idx_selected(uint32_t idx) "0x%"PRIx32
>  cpuhp_acpi_read_flags(uint32_t idx, uint8_t flags) "idx[0x%"PRIx32"] flags: 0x%"PRIx8
> @@ -48,10 +52,6 @@ acpi_pci_sel_read(uint32_t val) "%" PRIu32
>  acpi_pci_ej_write(uint64_t addr, uint64_t data) "0x%" PRIx64 " <== %" PRIu64
>  acpi_pci_sel_write(uint64_t addr, uint64_t data) "0x%" PRIx64 " <== %" PRIu64
>  
> -# piix4.c
> -piix4_gpe_readb(uint64_t addr, unsigned width, uint64_t val) "addr: 0x%" PRIx64 " width: %d ==> 0x%" PRIx64
> -piix4_gpe_writeb(uint64_t addr, unsigned width, uint64_t val) "addr: 0x%" PRIx64 " width: %d <== 0x%" PRIx64
> -
>  # tco.c
>  tco_timer_reload(int ticks, int msec) "ticks=%d (%d ms)"
>  tco_timer_expired(int timeouts_no, bool strap, bool no_reboot) "timeouts_no=%d no_reboot=%d/%d"

[PATCH 7/7] hw/acpi/core: Trace enable and status registers of GPE separately

[Bernhard Beschow]

The bit positions of both registers are related. Tracing the registers
independently results in the same offsets across these registers which
eases debugging.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/acpi/core.c       | 10 +++++++---
 hw/acpi/trace-events |  6 ++++--
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index a33e410e69..cc33605d61 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -687,13 +687,13 @@ void acpi_gpe_ioport_writeb(ACPIREGS *ar, uint32_t addr, uint32_t val)
 {
     uint8_t *cur;
 
-    trace_acpi_gpe_ioport_writeb(addr, val);
-
     cur = acpi_gpe_ioport_get_ptr(ar, addr);
     if (addr < ar->gpe.len / 2) {
+        trace_acpi_gpe_sts_ioport_writeb(addr, val);
         /* GPE_STS */
         *cur = (*cur) & ~val;
     } else if (addr < ar->gpe.len) {
+        trace_acpi_gpe_en_ioport_writeb(addr - (ar->gpe.len / 2), val);
         /* GPE_EN */
         *cur = val;
     } else {
@@ -712,7 +712,11 @@ uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr)
         val = *cur;
     }
 
-    trace_acpi_gpe_ioport_readb(addr, val);
+    if (addr < ar->gpe.len / 2) {
+        trace_acpi_gpe_sts_ioport_readb(addr, val);
+    } else {
+        trace_acpi_gpe_en_ioport_readb(addr - (ar->gpe.len / 2), val);
+    }
 
     return val;
 }
diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events
index 159937ddb9..d387adfb0b 100644
--- a/hw/acpi/trace-events
+++ b/hw/acpi/trace-events
@@ -18,8 +18,10 @@ mhp_acpi_pc_dimm_deleted(uint32_t slot) "slot[0x%"PRIx32"] pc-dimm deleted"
 mhp_acpi_pc_dimm_delete_failed(uint32_t slot) "slot[0x%"PRIx32"] pc-dimm delete failed"
 
 # core.c
-acpi_gpe_ioport_readb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " ==> 0x%" PRIx8
-acpi_gpe_ioport_writeb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " <== 0x%" PRIx8
+acpi_gpe_sts_ioport_readb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " ==> 0x%" PRIx8
+acpi_gpe_en_ioport_readb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " ==> 0x%" PRIx8
+acpi_gpe_sts_ioport_writeb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " <== 0x%" PRIx8
+acpi_gpe_en_ioport_writeb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " <== 0x%" PRIx8
 
 # cpu.c
 cpuhp_acpi_invalid_idx_selected(uint32_t idx) "0x%"PRIx32
-- 
2.39.1

Re: [PATCH 7/7] hw/acpi/core: Trace enable and status registers of GPE separately

[Igor Mammedov]

On Sun, 22 Jan 2023 18:07:24 +0100
Bernhard Beschow <shentey@gmail.com> wrote:

> The bit positions of both registers are related. Tracing the registers
> independently results in the same offsets across these registers which
> eases debugging.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Acked-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/acpi/core.c       | 10 +++++++---
>  hw/acpi/trace-events |  6 ++++--
>  2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index a33e410e69..cc33605d61 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -687,13 +687,13 @@ void acpi_gpe_ioport_writeb(ACPIREGS *ar, uint32_t addr, uint32_t val)
>  {
>      uint8_t *cur;
>  
> -    trace_acpi_gpe_ioport_writeb(addr, val);
> -
>      cur = acpi_gpe_ioport_get_ptr(ar, addr);
>      if (addr < ar->gpe.len / 2) {
> +        trace_acpi_gpe_sts_ioport_writeb(addr, val);
>          /* GPE_STS */
>          *cur = (*cur) & ~val;
>      } else if (addr < ar->gpe.len) {
> +        trace_acpi_gpe_en_ioport_writeb(addr - (ar->gpe.len / 2), val);
>          /* GPE_EN */
>          *cur = val;
>      } else {
> @@ -712,7 +712,11 @@ uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr)
>          val = *cur;
>      }
>  
> -    trace_acpi_gpe_ioport_readb(addr, val);
> +    if (addr < ar->gpe.len / 2) {
> +        trace_acpi_gpe_sts_ioport_readb(addr, val);
> +    } else {
> +        trace_acpi_gpe_en_ioport_readb(addr - (ar->gpe.len / 2), val);
> +    }
>  
>      return val;
>  }
> diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events
> index 159937ddb9..d387adfb0b 100644
> --- a/hw/acpi/trace-events
> +++ b/hw/acpi/trace-events
> @@ -18,8 +18,10 @@ mhp_acpi_pc_dimm_deleted(uint32_t slot) "slot[0x%"PRIx32"] pc-dimm deleted"
>  mhp_acpi_pc_dimm_delete_failed(uint32_t slot) "slot[0x%"PRIx32"] pc-dimm delete failed"
>  
>  # core.c
> -acpi_gpe_ioport_readb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " ==> 0x%" PRIx8
> -acpi_gpe_ioport_writeb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " <== 0x%" PRIx8
> +acpi_gpe_sts_ioport_readb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " ==> 0x%" PRIx8
> +acpi_gpe_en_ioport_readb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " ==> 0x%" PRIx8
> +acpi_gpe_sts_ioport_writeb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " <== 0x%" PRIx8
> +acpi_gpe_en_ioport_writeb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " <== 0x%" PRIx8
>  
>  # cpu.c
>  cpuhp_acpi_invalid_idx_selected(uint32_t idx) "0x%"PRIx32

Re: [PATCH 0/7] ACPI controller cleanup

[Igor Mammedov]

On Sun, 22 Jan 2023 18:07:17 +0100
Bernhard Beschow <shentey@gmail.com> wrote:

> This series brings the PIIX4 PM device closer to reality and resolves some
> redundant code along the way.

I'm done with this series review

> 
> Testing done:
> - `make check`
> - Starting a live CD under pc and q35 machines and check that the GPE accesses
>   are traced
> 
> Bernhard Beschow (7):
>   hw/acpi/{ich9,piix4}: Reuse existing attributes for QOM properties
>   hw/acpi/ich9: Remove unneeded assignments
>   hw/acpi/{ich9,piix4}: Resolve redundant io_base address attributes
>   hw/acpi/ich9: Use ICH9_PMIO_GPE0_STS just once
>   hw/acpi/piix4: Fix offset of GPE0 registers
>   hw/acpi: Trace GPE access in all device models, not just PIIX4
>   hw/acpi/core: Trace enable and status registers of GPE separately
> 
>  include/hw/acpi/ich9.h  |  1 -
>  include/hw/acpi/piix4.h |  3 +--
>  hw/acpi/core.c          |  9 +++++++++
>  hw/acpi/ich9.c          | 26 ++++++++++++--------------
>  hw/acpi/piix4.c         | 31 ++++++++++++++++---------------
>  hw/acpi/trace-events    | 10 ++++++----
>  6 files changed, 44 insertions(+), 36 deletions(-)
>