diff mbox series

[5/7] hw/acpi/piix4: Fix offset of GPE0 registers

Message ID 20230122170724.21868-6-shentey@gmail.com
State New
Headers show
Series ACPI controller cleanup | expand

Commit Message

Bernhard Beschow Jan. 22, 2023, 5:07 p.m. UTC 
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(-)

Comments

Igor Mammedov Jan. 25, 2023, 3:55 p.m. UTC | #1 
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,
Bernhard Beschow Jan. 29, 2023, 2:55 p.m. UTC | #2 
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,
>
Igor Mammedov March 2, 2023, 2:31 p.m. UTC | #3 
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 mbox series

Patch

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,