On Sun, 29 Jan 2023 14:55:06 +0000
Bernhard Beschow <shen...@gmail.com> wrote:
> 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?
I don't think it's worth of effort, io_gpe is not worse that others.
>
> >> 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,
> >
>
