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