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,