| Message ID | 20230122170724.21868-6-shentey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ACPI controller cleanup | expand |
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,
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, >
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, > > >
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,
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. 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. 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(-)