Re: [PATCH v3 3/4] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table

[Liav Albani]

On 2/27/22 08:56, Ani Sinha wrote:

On Sat, 26 Feb 2022, Liav Albani wrote:

This can allow the guest OS to determine more easily if i8042 controller
is present in the system or not, so it doesn't need to do probing of the
controller, but just initialize it immediately, before enumerating the
ACPI AML namespace.

Signed-off-by: Liav Albani <liav...@gmail.com>
---
  hw/acpi/aml-build.c         | 7 ++++++-
  hw/i386/acpi-build.c        | 8 ++++++++
  hw/i386/acpi-microvm.c      | 9 +++++++++
  include/hw/acpi/acpi-defs.h | 1 +
  4 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 8966e16320..ef5f4cad87 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2152,7 +2152,12 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const 
AcpiFadtData *f,
      build_append_int_noprefix(tbl, 0, 1); /* DAY_ALRM */
      build_append_int_noprefix(tbl, 0, 1); /* MON_ALRM */
      build_append_int_noprefix(tbl, f->rtc_century, 1); /* CENTURY */
-    build_append_int_noprefix(tbl, 0, 2); /* IAPC_BOOT_ARCH */
+    /* IAPC_BOOT_ARCH */
+    if (f->rev == 1) {
+        build_append_int_noprefix(tbl, 0, 2);
+    } else {
+        build_append_int_noprefix(tbl, f->iapc_boot_arch, 2);
+    }

Like a suggested in the previous iteration, please add a comment here to
specify why you are adding this only for rev !=1 . Also please mention
that your change only affects q35 machines since i440fx uses rev 1 (ref to
acpi_get_pm_info()).

      build_append_int_noprefix(tbl, 0, 1); /* Reserved */
      build_append_int_noprefix(tbl, f->flags, 4); /* Flags */

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index ebd47aa26f..65dbc1ec36 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -192,6 +192,14 @@ static void init_common_fadt_data(MachineState *ms, Object 
*o,
              .address = object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK, 
NULL)
          },
      };
+    /*
+     * second bit of 16 but

wow, you even retained my typo here! :-)

Yeah, I figured this after I sent the patch series, sorry for that mistake!

IAPC_BOOT_ARCH indicates presence of 8042 or
+     * equivalent micro controller. See table 5-10 of APCI spec version 2.0
+     * (the earliest acpi revision that supports this).
+     */
+
+    fadt.iapc_boot_arch = isa_check_device_existence("i8042") ? 0x0002 : 
0x0000;
+
      *data = fadt;
  }

diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
index 68ca7e7fc2..e5f89164be 100644
--- a/hw/i386/acpi-microvm.c
+++ b/hw/i386/acpi-microvm.c
@@ -189,6 +189,15 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
          .reset_val = ACPI_GED_RESET_VALUE,
      };

+    /*
+     * second bit of 16 but IAPC_BOOT_ARCH indicates presence of 8042 or

ditto as above.

No problem, I'll send a v4 soon :)

  +     * equivalent micro controller. See table 5-10 of APCI spec version 2.0

+     * (the earliest acpi revision that supports this).
+     */
+
+    pmfadt.iapc_boot_arch = isa_check_device_existence("i8042") ? 0x0002
+                            : 0x0000;
+
      table_offsets = g_array_new(false, true /* clear */,
                                          sizeof(uint32_t));
      bios_linker_loader_alloc(tables->linker,
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index c97e8633ad..2b42e4192b 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -77,6 +77,7 @@ typedef struct AcpiFadtData {
      uint16_t plvl2_lat;        /* P_LVL2_LAT */
      uint16_t plvl3_lat;        /* P_LVL3_LAT */
      uint16_t arm_boot_arch;    /* ARM_BOOT_ARCH */
+    uint16_t iapc_boot_arch;   /* IAPC_BOOT_ARCH */
      uint8_t minor_ver;         /* FADT Minor Version */

      /*
--
2.35.1