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,  
> >  
>