Am 25. Januar 2023 15:55:01 UTC schrieb Igor Mammedov <imamm...@redhat.com>:
>On Sun, 22 Jan 2023 18:07:22 +0100
>Bernhard Beschow <shen...@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 <shen...@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,
>
