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

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

Posted by Bernhard Beschow 7 months ago

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 the code to generate AML for
the reset register in the FADT was common. However, since commit 937d1b58714b
("pc: acpi: isolate FADT specific data into AcpiFadtData structure") the AML
gets generated for ICH9 only. There is no need any loger for the assertion, so
remove it.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 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 v3] hw/i386/acpi-build: Remove build-time assertion on PIIX/ICH9 reset registers being identical

Posted by Ani Sinha 7 months ago


> On 04-Oct-2023, at 2:46 AM, 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 the code to generate AML for
> the reset register in the FADT was common. However, since commit 937d1b58714b
> ("pc: acpi: isolate FADT specific data into AcpiFadtData structure") the AML
> gets generated for ICH9 only.

This isn’t quite true. See 3a3fcc75f92ab0d71ba (" pc: acpi: force FADT rev1 for 440fx based machine types”) where the fadt table size for i440fx is no longer *fadt but offsetof(typeof(*fadt), reset_register). The above commit simply makes sure we do not populate reset_register etc for i440fx since its not used anyway.


> There is no need any loger for the assertion, so
Typo                  ^^^^^
            
> remove it.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Other than the above, I agree with the change. So ..

Reviewed-by: Ani Sinha <anisinha@redhat.com>

> ---
> 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 v3] hw/i386/acpi-build: Remove build-time assertion on PIIX/ICH9 reset registers being identical

Posted by Philippe Mathieu-Daudé 7 months ago

On 3/10/23 23:16, Bernhard Beschow 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 the code to generate AML for
> the reset register in the FADT was common. However, since commit 937d1b58714b
> ("pc: acpi: isolate FADT specific data into AcpiFadtData structure") the AML
> gets generated for ICH9 only. There is no need any loger for the assertion, so

"longer"

> remove it.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/i386/acpi-build.c | 5 -----
>   1 file changed, 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

Posted by Bernhard Beschow 7 months ago

The iteration in the subject should have been 1, not 3...

Am 3. Oktober 2023 21:16:58 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>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 the code to generate AML for
>the reset register in the FADT was common. However, since commit 937d1b58714b
>("pc: acpi: isolate FADT specific data into AcpiFadtData structure") the AML
>gets generated for ICH9 only. There is no need any loger for the assertion, so
>remove it.
>
>Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>---
> 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) {