[PATCH v2] hw/i386/acpi-build: Remove build-time assertion on PIIX/ICH9 reset registers being identical

[PATCH v2] hw/i386/acpi-build: Remove build-time assertion on PIIX/ICH9 reset registers being identical

[Bernhard Beschow]

Commit 6103451aeb74 ("hw/i386: Build-time assertion on pc/q35 reset register
being identical.") introduced a build-time check where the addresses of the
reset registers are expected to be equal. Back then rev3 of the FADT was used
which required the reset register to be populated and there was common code.
In commit 3a3fcc75f92a ("pc: acpi: force FADT rev1 for 440fx based machine
types") the FADT was downgraded to rev1 for PIIX where the reset register isn't
available. Thus, there is no need for the assertion any longer, so remove it.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Ani Sinha <anisinha@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

---

v2:
* Fix justification of commit message (Ani)
* Fix typo (Ani, Philippe)
---
 hw/i386/acpi-build.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 95199c8900..6fff1901f5 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -56,7 +56,6 @@
 
 /* Supported chipsets: */
 #include "hw/southbridge/ich9.h"
-#include "hw/southbridge/piix.h"
 #include "hw/acpi/pcihp.h"
 #include "hw/i386/fw_cfg.h"
 #include "hw/i386/pc.h"
@@ -242,10 +241,6 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
     pm->pcihp_io_len =
         object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
 
-    /* The above need not be conditional on machine type because the reset port
-     * happens to be the same on PIIX (pc) and ICH9 (q35). */
-    QEMU_BUILD_BUG_ON(ICH9_RST_CNT_IOPORT != PIIX_RCR_IOPORT);
-
     /* Fill in optional s3/s4 related properties */
     o = object_property_get_qobject(obj, ACPI_PM_PROP_S3_DISABLED, NULL);
     if (o) {
-- 
2.42.0

Re: [PATCH v2] hw/i386/acpi-build: Remove build-time assertion on PIIX/ICH9 reset registers being identical

[Ani Sinha]

> On 04-Oct-2023, at 2:53 PM, Bernhard Beschow <shentey@gmail.com> wrote:
> 
> Commit 6103451aeb74 ("hw/i386: Build-time assertion on pc/q35 reset register
> being identical.") introduced a build-time check where the addresses of the
> reset registers are expected to be equal. Back then rev3 of the FADT was used
> which required the reset register to be populated and there was common code.
> In commit 3a3fcc75f92a ("pc: acpi: force FADT rev1 for 440fx based machine
> types") the FADT was downgraded to rev1 for PIIX where the reset register isn't
> available. Thus, there is no need for the assertion any longer, so remove it.

Yes this description looks much better. For the records,
Comparing rev 2 https://uefi.org/sites/default/files/resources/ACPI_2.pdf, section 5.2.8 page 102, table 5-8 vs 
Rev 1 https://uefi.org/sites/default/files/resources/ACPI_1_Errata_B.pdf section 5.2.5, page 117, table 5-7

We can see that in version 1, the FADT offset goes only to 112 + 4 (flags) = 116 and in rev 2, the reset register is at offset 116, right after the flags offset. So it all makes sense. I440fx would never have the reset register unless it starts supporting version 3 of FADT.

> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Ani Sinha <anisinha@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> ---
> 
> v2:
> * Fix justification of commit message (Ani)
> * Fix typo (Ani, Philippe)
> ---
> hw/i386/acpi-build.c | 5 -----
> 1 file changed, 5 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 95199c8900..6fff1901f5 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -56,7 +56,6 @@
> 
> /* Supported chipsets: */
> #include "hw/southbridge/ich9.h"
> -#include "hw/southbridge/piix.h"
> #include "hw/acpi/pcihp.h"
> #include "hw/i386/fw_cfg.h"
> #include "hw/i386/pc.h"
> @@ -242,10 +241,6 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
>     pm->pcihp_io_len =
>         object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> 
> -    /* The above need not be conditional on machine type because the reset port
> -     * happens to be the same on PIIX (pc) and ICH9 (q35). */
> -    QEMU_BUILD_BUG_ON(ICH9_RST_CNT_IOPORT != PIIX_RCR_IOPORT);
> -
>     /* Fill in optional s3/s4 related properties */
>     o = object_property_get_qobject(obj, ACPI_PM_PROP_S3_DISABLED, NULL);
>     if (o) {
> -- 
> 2.42.0
>