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

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

[Liav Albani]

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.

To allow "flexible" indication, I don't hardcode the bit at location 1
as on in the IA-PC boot flags, but try to search for i8042 on the ISA
bus to verify it exists in the system.

Why this is useful you might ask - this patch allows the guest OS to
probe and use the i8042 controller without decoding the ACPI AML blob
at all. For example, as a developer of the SerenityOS kernel, I might
want to allow people to not try to decode the ACPI AML namespace (for
now, we still don't support ACPI AML as it's a work in progress), but
still to not probe for the i8042 but just use it after looking in the
IA-PC boot flags in the ACPI FADT table.

A note about this version of the patch series: I changed the assertion
checking if the ISA bus exists to a if statement, because I can see how
in the future someone might want to run a x86 machine without an ISA bus
so we should not assert if someone calls the ISA check device existence
function but return FALSE gracefully.
If someone thinks this is wrong, I'm more than happy to discuss and fix
the code :)

Liav Albani (4):
  hw/isa: add function to check for existence of device by its type
  tests/acpi: i386: allow FACP acpi table changes
  hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT
    table
  tests/acpi: i386: update FACP table differences

 hw/acpi/aml-build.c            |   7 ++++++-
 hw/i386/acpi-build.c           |   8 ++++++++
 hw/i386/acpi-microvm.c         |   9 +++++++++
 hw/isa/isa-bus.c               |  23 +++++++++++++++++++++++
 include/hw/acpi/acpi-defs.h    |   1 +
 include/hw/isa/isa.h           |   1 +
 tests/data/acpi/q35/FACP       | Bin 244 -> 244 bytes
 tests/data/acpi/q35/FACP.nosmm | Bin 244 -> 244 bytes
 tests/data/acpi/q35/FACP.slic  | Bin 244 -> 244 bytes
 tests/data/acpi/q35/FACP.xapic | Bin 244 -> 244 bytes
 10 files changed, 48 insertions(+), 1 deletion(-)

-- 
2.35.1

[PATCH v3 1/4] hw/isa: add function to check for existence of device by its type

[Liav Albani]

This function enumerates all attached ISA devices in the machine, and
tries to compare a given device type name to the enumerated devices.
For example, this can help other code to determine if a i8042 controller
exists in the machine.

Signed-off-by: Liav Albani <liavalb@gmail.com>
---
 hw/isa/isa-bus.c     | 23 +++++++++++++++++++++++
 include/hw/isa/isa.h |  1 +
 2 files changed, 24 insertions(+)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 6c31398dda..663aa36d29 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -222,6 +222,29 @@ void isa_build_aml(ISABus *bus, Aml *scope)
     }
 }
 
+bool isa_check_device_existence(const char *typename)
+{
+    /*
+     * If there's no ISA bus, we know for sure that the checked ISA device type
+     * doesn't exist in the machine.
+     */
+    if (isabus == NULL) {
+        return false;
+    }
+
+    BusChild *kid;
+    ISADevice *dev;
+
+    QTAILQ_FOREACH(kid, &isabus->parent_obj.children, sibling) {
+        dev = ISA_DEVICE(kid->child);
+        const char *object_type = object_get_typename(OBJECT(dev));
+        if (object_type && strcmp(object_type, typename) == 0) {
+            return true;
+        }
+    }
+    return false;
+}
+
 static void isabus_dev_print(Monitor *mon, DeviceState *dev, int indent)
 {
     ISADevice *d = ISA_DEVICE(dev);
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index d4417b34b6..65f0c7e28c 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -99,6 +99,7 @@ IsaDma *isa_get_dma(ISABus *bus, int nchan);
 MemoryRegion *isa_address_space(ISADevice *dev);
 MemoryRegion *isa_address_space_io(ISADevice *dev);
 ISADevice *isa_new(const char *name);
+bool isa_check_device_existence(const char *typename);
 ISADevice *isa_try_new(const char *name);
 bool isa_realize_and_unref(ISADevice *dev, ISABus *bus, Error **errp);
 ISADevice *isa_create_simple(ISABus *bus, const char *name);
-- 
2.35.1

[PATCH v3 2/4] tests/acpi: i386: allow FACP acpi table changes

[Liav Albani]

The FACP table is going to be changed for x86/q35 machines. To be sure
the following changes are not breaking any QEMU test this change follows
step 2 from the bios-tables-test.c guide on changes that affect ACPI
tables.

Signed-off-by: Liav Albani <liavalb@gmail.com>
---
 tests/qtest/bios-tables-test-allowed-diff.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..7570e39369 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,5 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/q35/FACP",
+"tests/data/acpi/q35/FACP.nosmm",
+"tests/data/acpi/q35/FACP.slic",
+"tests/data/acpi/q35/FACP.xapic",
-- 
2.35.1

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

[Liav Albani]

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 <liavalb@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);
+    }
     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 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
+     * 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

[PATCH v3 4/4] tests/acpi: i386: update FACP table differences

[Liav Albani]

After changing the IAPC boot flags register to indicate support of i8042
in the machine chipset to help the guest OS to determine its existence
"faster", we need to have the updated FACP ACPI binary images in tree.

@@ -1,32 +1,32 @@
 /*
  * Intel ACPI Component Architecture
  * AML/ASL+ Disassembler version 20211217 (64-bit version)
  * Copyright (c) 2000 - 2021 Intel Corporation
  *
- * Disassembly of tests/data/acpi/q35/FACP, Wed Feb 23 22:37:39 2022
+ * Disassembly of /tmp/aml-BBFBI1, Wed Feb 23 22:37:39 2022
  *
  * ACPI Data Table [FACP]
  *
  * Format: [HexOffset DecimalOffset ByteLength]  FieldName : FieldValue (in hex)
  */

 [000h 0000   4]                    Signature : "FACP"    [Fixed ACPI Description Table (FADT)]
 [004h 0004   4]                 Table Length : 000000F4
 [008h 0008   1]                     Revision : 03
-[009h 0009   1]                     Checksum : B9
+[009h 0009   1]                     Checksum : B7
 [00Ah 0010   6]                       Oem ID : "BOCHS "
 [010h 0016   8]                 Oem Table ID : "BXPC    "
 [018h 0024   4]                 Oem Revision : 00000001
 [01Ch 0028   4]              Asl Compiler ID : "BXPC"
 [020h 0032   4]        Asl Compiler Revision : 00000001

 [024h 0036   4]                 FACS Address : 00000000
 [028h 0040   4]                 DSDT Address : 00000000
 [02Ch 0044   1]                        Model : 01
 [02Dh 0045   1]                   PM Profile : 00 [Unspecified]
 [02Eh 0046   2]                SCI Interrupt : 0009
 [030h 0048   4]             SMI Command Port : 000000B2
 [034h 0052   1]            ACPI Enable Value : 02
 [035h 0053   1]           ACPI Disable Value : 03
 [036h 0054   1]               S4BIOS Command : 00
 [037h 0055   1]              P-State Control : 00
@@ -42,35 +42,35 @@
 [059h 0089   1]     PM1 Control Block Length : 02
 [05Ah 0090   1]     PM2 Control Block Length : 00
 [05Bh 0091   1]        PM Timer Block Length : 04
 [05Ch 0092   1]            GPE0 Block Length : 10
 [05Dh 0093   1]            GPE1 Block Length : 00
 [05Eh 0094   1]             GPE1 Base Offset : 00
 [05Fh 0095   1]                 _CST Support : 00
 [060h 0096   2]                   C2 Latency : 0FFF
 [062h 0098   2]                   C3 Latency : 0FFF
 [064h 0100   2]               CPU Cache Size : 0000
 [066h 0102   2]           Cache Flush Stride : 0000
 [068h 0104   1]            Duty Cycle Offset : 00
 [069h 0105   1]             Duty Cycle Width : 00
 [06Ah 0106   1]          RTC Day Alarm Index : 00
 [06Bh 0107   1]        RTC Month Alarm Index : 00
 [06Ch 0108   1]            RTC Century Index : 32
-[06Dh 0109   2]   Boot Flags (decoded below) : 0000
+[06Dh 0109   2]   Boot Flags (decoded below) : 0002
                Legacy Devices Supported (V2) : 0
-            8042 Present on ports 60/64 (V2) : 0
+            8042 Present on ports 60/64 (V2) : 1
                         VGA Not Present (V4) : 0
                       MSI Not Supported (V4) : 0
                 PCIe ASPM Not Supported (V4) : 0
                    CMOS RTC Not Present (V5) : 0
 [06Fh 0111   1]                     Reserved : 00
 [070h 0112   4]        Flags (decoded below) : 000084A5
       WBINVD instruction is operational (V1) : 1
               WBINVD flushes all caches (V1) : 0
                     All CPUs support C1 (V1) : 1
                   C2 works on MP system (V1) : 0
             Control Method Power Button (V1) : 0
             Control Method Sleep Button (V1) : 1
         RTC wake not in fixed reg space (V1) : 0
             RTC can wake system from S4 (V1) : 1
                         32-bit PM Timer (V1) : 0
                       Docking Supported (V1) : 0
@@ -148,32 +148,32 @@
 [0DCh 0220   1]                     Space ID : 01 [SystemIO]
 [0DDh 0221   1]                    Bit Width : 80
 [0DEh 0222   1]                   Bit Offset : 00
 [0DFh 0223   1]         Encoded Access Width : 00 [Undefined/Legacy]
 [0E0h 0224   8]                      Address : 0000000000000620

 [0E8h 0232  12]                   GPE1 Block : [Generic Address Structure]
 [0E8h 0232   1]                     Space ID : 00 [SystemMemory]
 [0E9h 0233   1]                    Bit Width : 00
 [0EAh 0234   1]                   Bit Offset : 00
 [0EBh 0235   1]         Encoded Access Width : 00 [Undefined/Legacy]
 [0ECh 0236   8]                      Address : 0000000000000000

 Raw Table Data: Length 244 (0xF4)

-    0000: 46 41 43 50 F4 00 00 00 03 B9 42 4F 43 48 53 20  // FACP......BOCHS
+    0000: 46 41 43 50 F4 00 00 00 03 B7 42 4F 43 48 53 20  // FACP......BOCHS
     0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43  // BXPC    ....BXPC
     0020: 01 00 00 00 00 00 00 00 00 00 00 00 01 00 09 00  // ................
     0030: B2 00 00 00 02 03 00 00 00 06 00 00 00 00 00 00  // ................
     0040: 04 06 00 00 00 00 00 00 00 00 00 00 08 06 00 00  // ................
     0050: 20 06 00 00 00 00 00 00 04 02 00 04 10 00 00 00  //  ...............
-    0060: FF 0F FF 0F 00 00 00 00 00 00 00 00 32 00 00 00  // ............2...
+    0060: FF 0F FF 0F 00 00 00 00 00 00 00 00 32 02 00 00  // ............2...
     0070: A5 84 00 00 01 08 00 00 F9 0C 00 00 00 00 00 00  // ................
     0080: 0F 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  // ................
     0090: 00 00 00 00 01 20 00 00 00 06 00 00 00 00 00 00  // ..... ..........
     00A0: 00 00 00 00 00 00 00 00 00 00 00 00 01 10 00 00  // ................
     00B0: 04 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00  // ................
     00C0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  // ................
     00D0: 01 20 00 00 08 06 00 00 00 00 00 00 01 80 00 00  // . ..............
     00E0: 20 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00  //  ...............
     00F0: 00 00 00 00                                      // ....
**

Signed-off-by: Liav Albani <liavalb@gmail.com>
---
 tests/data/acpi/q35/FACP                    | Bin 244 -> 244 bytes
 tests/data/acpi/q35/FACP.nosmm              | Bin 244 -> 244 bytes
 tests/data/acpi/q35/FACP.slic               | Bin 244 -> 244 bytes
 tests/data/acpi/q35/FACP.xapic              | Bin 244 -> 244 bytes
 tests/qtest/bios-tables-test-allowed-diff.h |   4 ----
 5 files changed, 4 deletions(-)

diff --git a/tests/data/acpi/q35/FACP b/tests/data/acpi/q35/FACP
index f6a864cc863c7763f6c09d3814ad184a658fa0a0..a8f6a8961109d01059aceef9f1869cde09a2f10c 100644
GIT binary patch
delta 23
ecmeyu_=S<n&CxmF3j+fK^Y)2c$&5@B^V$GgGY3Ne

delta 23
ecmeyu_=S<n&CxmF3j+fK^UjG}$&3sW^V$GgJqJSo

diff --git a/tests/data/acpi/q35/FACP.nosmm b/tests/data/acpi/q35/FACP.nosmm
index 6a9aa5f370eb9af6a03dc739d8a159be58fdee01..c4e6d18ee5fc64159160d4589aa96b4d648c913a 100644
GIT binary patch
delta 23
ecmeyu_=S<n&CxmF3j+fKbKXR*WJacmd2Ik#q6Yc^

delta 23
ecmeyu_=S<n&CxmF3j+fKbHPNeWJZRGd2Ik#tOoi3

diff --git a/tests/data/acpi/q35/FACP.slic b/tests/data/acpi/q35/FACP.slic
index 15986e095cf2db7ee92f7ce113c1d46d54018c62..48bbb1cf5ad0ceda1d2f6d56edf5c1e207bd1a04 100644
GIT binary patch
delta 23
ecmeyu_=S<n&CxmF3j+fK^M#3A$&5@B^V$Gh6bD=Y

delta 23
ecmeyu_=S<n&CxmF3j+fK^QDPg$&3sW^V$Gh9tT_i

diff --git a/tests/data/acpi/q35/FACP.xapic b/tests/data/acpi/q35/FACP.xapic
index 2d3659c9c6753d07c3d48742343cb8e8cc034de7..31fa5dd19c213034eef4eeefa6a04e61dadd8a2a 100644
GIT binary patch
delta 23
ecmeyu_=S<n&CxmF3j+fK^X7?M$&5@B^V$Gg4+lR0

delta 23
ecmeyu_=S<n&CxmF3j+fK^VW%6$&3sW^V$Gg83#WA

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index 7570e39369..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,5 +1 @@
 /* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/q35/FACP",
-"tests/data/acpi/q35/FACP.nosmm",
-"tests/data/acpi/q35/FACP.slic",
-"tests/data/acpi/q35/FACP.xapic",
-- 
2.35.1

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

[Ani Sinha]

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 <liavalb@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! :-)


>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.

 +     * 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
>
>

Re: [PATCH v3 2/4] tests/acpi: i386: allow FACP acpi table changes

[Ani Sinha]

On Sat, 26 Feb 2022, Liav Albani wrote:

> The FACP table is going to be changed for x86/q35 machines. To be sure
> the following changes are not breaking any QEMU test this change follows
> step 2 from the bios-tables-test.c guide on changes that affect ACPI
> tables.
>
> Signed-off-by: Liav Albani <liavalb@gmail.com>

Acked-by: Ani Sinha <ani@anisinha.ca>

> ---
>  tests/qtest/bios-tables-test-allowed-diff.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index dfb8523c8b..7570e39369 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1 +1,5 @@
>  /* List of comma-separated changed AML files to ignore */
> +"tests/data/acpi/q35/FACP",
> +"tests/data/acpi/q35/FACP.nosmm",
> +"tests/data/acpi/q35/FACP.slic",
> +"tests/data/acpi/q35/FACP.xapic",
> --
> 2.35.1
>
>

Re: [PATCH v3 4/4] tests/acpi: i386: update FACP table differences

[Ani Sinha]

On Sat, 26 Feb 2022, Liav Albani wrote:

> After changing the IAPC boot flags register to indicate support of i8042
> in the machine chipset to help the guest OS to determine its existence
> "faster", we need to have the updated FACP ACPI binary images in tree.
>
> @@ -1,32 +1,32 @@
>  /*
>   * Intel ACPI Component Architecture
>   * AML/ASL+ Disassembler version 20211217 (64-bit version)
>   * Copyright (c) 2000 - 2021 Intel Corporation
>   *
> - * Disassembly of tests/data/acpi/q35/FACP, Wed Feb 23 22:37:39 2022
> + * Disassembly of /tmp/aml-BBFBI1, Wed Feb 23 22:37:39 2022
>   *
>   * ACPI Data Table [FACP]
>   *
>   * Format: [HexOffset DecimalOffset ByteLength]  FieldName : FieldValue (in hex)
>   */
>
>  [000h 0000   4]                    Signature : "FACP"    [Fixed ACPI Description Table (FADT)]
>  [004h 0004   4]                 Table Length : 000000F4
>  [008h 0008   1]                     Revision : 03
> -[009h 0009   1]                     Checksum : B9
> +[009h 0009   1]                     Checksum : B7
>  [00Ah 0010   6]                       Oem ID : "BOCHS "
>  [010h 0016   8]                 Oem Table ID : "BXPC    "
>  [018h 0024   4]                 Oem Revision : 00000001
>  [01Ch 0028   4]              Asl Compiler ID : "BXPC"
>  [020h 0032   4]        Asl Compiler Revision : 00000001
>
>  [024h 0036   4]                 FACS Address : 00000000
>  [028h 0040   4]                 DSDT Address : 00000000
>  [02Ch 0044   1]                        Model : 01
>  [02Dh 0045   1]                   PM Profile : 00 [Unspecified]
>  [02Eh 0046   2]                SCI Interrupt : 0009
>  [030h 0048   4]             SMI Command Port : 000000B2
>  [034h 0052   1]            ACPI Enable Value : 02
>  [035h 0053   1]           ACPI Disable Value : 03
>  [036h 0054   1]               S4BIOS Command : 00
>  [037h 0055   1]              P-State Control : 00
> @@ -42,35 +42,35 @@
>  [059h 0089   1]     PM1 Control Block Length : 02
>  [05Ah 0090   1]     PM2 Control Block Length : 00
>  [05Bh 0091   1]        PM Timer Block Length : 04
>  [05Ch 0092   1]            GPE0 Block Length : 10
>  [05Dh 0093   1]            GPE1 Block Length : 00
>  [05Eh 0094   1]             GPE1 Base Offset : 00
>  [05Fh 0095   1]                 _CST Support : 00
>  [060h 0096   2]                   C2 Latency : 0FFF
>  [062h 0098   2]                   C3 Latency : 0FFF
>  [064h 0100   2]               CPU Cache Size : 0000
>  [066h 0102   2]           Cache Flush Stride : 0000
>  [068h 0104   1]            Duty Cycle Offset : 00
>  [069h 0105   1]             Duty Cycle Width : 00
>  [06Ah 0106   1]          RTC Day Alarm Index : 00
>  [06Bh 0107   1]        RTC Month Alarm Index : 00
>  [06Ch 0108   1]            RTC Century Index : 32
> -[06Dh 0109   2]   Boot Flags (decoded below) : 0000
> +[06Dh 0109   2]   Boot Flags (decoded below) : 0002
>                 Legacy Devices Supported (V2) : 0
> -            8042 Present on ports 60/64 (V2) : 0
> +            8042 Present on ports 60/64 (V2) : 1
>                          VGA Not Present (V4) : 0
>                        MSI Not Supported (V4) : 0
>                  PCIe ASPM Not Supported (V4) : 0
>                     CMOS RTC Not Present (V5) : 0
>  [06Fh 0111   1]                     Reserved : 00
>  [070h 0112   4]        Flags (decoded below) : 000084A5
>        WBINVD instruction is operational (V1) : 1
>                WBINVD flushes all caches (V1) : 0
>                      All CPUs support C1 (V1) : 1
>                    C2 works on MP system (V1) : 0
>              Control Method Power Button (V1) : 0
>              Control Method Sleep Button (V1) : 1
>          RTC wake not in fixed reg space (V1) : 0
>              RTC can wake system from S4 (V1) : 1
>                          32-bit PM Timer (V1) : 0
>                        Docking Supported (V1) : 0
> @@ -148,32 +148,32 @@
>  [0DCh 0220   1]                     Space ID : 01 [SystemIO]
>  [0DDh 0221   1]                    Bit Width : 80
>  [0DEh 0222   1]                   Bit Offset : 00
>  [0DFh 0223   1]         Encoded Access Width : 00 [Undefined/Legacy]
>  [0E0h 0224   8]                      Address : 0000000000000620
>
>  [0E8h 0232  12]                   GPE1 Block : [Generic Address Structure]
>  [0E8h 0232   1]                     Space ID : 00 [SystemMemory]
>  [0E9h 0233   1]                    Bit Width : 00
>  [0EAh 0234   1]                   Bit Offset : 00
>  [0EBh 0235   1]         Encoded Access Width : 00 [Undefined/Legacy]
>  [0ECh 0236   8]                      Address : 0000000000000000
>
>  Raw Table Data: Length 244 (0xF4)
>
> -    0000: 46 41 43 50 F4 00 00 00 03 B9 42 4F 43 48 53 20  // FACP......BOCHS
> +    0000: 46 41 43 50 F4 00 00 00 03 B7 42 4F 43 48 53 20  // FACP......BOCHS
>      0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43  // BXPC    ....BXPC
>      0020: 01 00 00 00 00 00 00 00 00 00 00 00 01 00 09 00  // ................
>      0030: B2 00 00 00 02 03 00 00 00 06 00 00 00 00 00 00  // ................
>      0040: 04 06 00 00 00 00 00 00 00 00 00 00 08 06 00 00  // ................
>      0050: 20 06 00 00 00 00 00 00 04 02 00 04 10 00 00 00  //  ...............
> -    0060: FF 0F FF 0F 00 00 00 00 00 00 00 00 32 00 00 00  // ............2...
> +    0060: FF 0F FF 0F 00 00 00 00 00 00 00 00 32 02 00 00  // ............2...
>      0070: A5 84 00 00 01 08 00 00 F9 0C 00 00 00 00 00 00  // ................
>      0080: 0F 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  // ................
>      0090: 00 00 00 00 01 20 00 00 00 06 00 00 00 00 00 00  // ..... ..........
>      00A0: 00 00 00 00 00 00 00 00 00 00 00 00 01 10 00 00  // ................
>      00B0: 04 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00  // ................
>      00C0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  // ................
>      00D0: 01 20 00 00 08 06 00 00 00 00 00 00 01 80 00 00  // . ..............
>      00E0: 20 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00  //  ...............
>      00F0: 00 00 00 00                                      // ....
> **
>
> Signed-off-by: Liav Albani <liavalb@gmail.com>

Acked-by: Ani Sinha <ani@anisinha.ca>

> ---
>  tests/data/acpi/q35/FACP                    | Bin 244 -> 244 bytes
>  tests/data/acpi/q35/FACP.nosmm              | Bin 244 -> 244 bytes
>  tests/data/acpi/q35/FACP.slic               | Bin 244 -> 244 bytes
>  tests/data/acpi/q35/FACP.xapic              | Bin 244 -> 244 bytes
>  tests/qtest/bios-tables-test-allowed-diff.h |   4 ----
>  5 files changed, 4 deletions(-)

Re: [PATCH v3 1/4] hw/isa: add function to check for existence of device by its type

[Ani Sinha]

On Sat, 26 Feb 2022, Liav Albani wrote:

> This function enumerates all attached ISA devices in the machine, and
> tries to compare a given device type name to the enumerated devices.
> For example, this can help other code to determine if a i8042 controller
> exists in the machine.
>
> Signed-off-by: Liav Albani <liavalb@gmail.com>
> ---
>  hw/isa/isa-bus.c     | 23 +++++++++++++++++++++++
>  include/hw/isa/isa.h |  1 +
>  2 files changed, 24 insertions(+)
>
> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
> index 6c31398dda..663aa36d29 100644
> --- a/hw/isa/isa-bus.c
> +++ b/hw/isa/isa-bus.c
> @@ -222,6 +222,29 @@ void isa_build_aml(ISABus *bus, Aml *scope)
>      }
>  }
>
> +bool isa_check_device_existence(const char *typename)
> +{
> +    /*
> +     * If there's no ISA bus, we know for sure that the checked ISA device type
> +     * doesn't exist in the machine.
> +     */
> +    if (isabus == NULL) {

nit: I would do if (!isabus) instead to keep uniformity with other parts
of the code.

> +        return false;
> +    }
> +
> +    BusChild *kid;
> +    ISADevice *dev;
> +
> +    QTAILQ_FOREACH(kid, &isabus->parent_obj.children, sibling) {
> +        dev = ISA_DEVICE(kid->child);
> +        const char *object_type = object_get_typename(OBJECT(dev));
> +        if (object_type && strcmp(object_type, typename) == 0) {

nit: I would do !strcmp() instead.

> +            return true;
> +        }
> +    }
> +    return false;
> +}
> +
>  static void isabus_dev_print(Monitor *mon, DeviceState *dev, int indent)
>  {
>      ISADevice *d = ISA_DEVICE(dev);
> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
> index d4417b34b6..65f0c7e28c 100644
> --- a/include/hw/isa/isa.h
> +++ b/include/hw/isa/isa.h
> @@ -99,6 +99,7 @@ IsaDma *isa_get_dma(ISABus *bus, int nchan);
>  MemoryRegion *isa_address_space(ISADevice *dev);
>  MemoryRegion *isa_address_space_io(ISADevice *dev);
>  ISADevice *isa_new(const char *name);
> +bool isa_check_device_existence(const char *typename);

Please provide documentation for this function in line with other
functions like isa_register_ioport() and isa_register_portio_list()  in
the same header.

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

[Bernhard Beschow]

Am 26. Februar 2022 06:30:18 UTC schrieb Liav Albani <liavalb@gmail.com>:
>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 <liavalb@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);
>+    }
>     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 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;

Couldn't qdev_find_recursive() be used here instead? This would also make patch 1 unneccessary. Same below.

Best regards
Bernhard

>+
>     *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
>+     * 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 */
> 
>     /*

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 12:48, Bernhard Beschow wrote:
> Am 26. Februar 2022 06:30:18 UTC schrieb Liav Albani <liavalb@gmail.com>:
>> 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 <liavalb@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);
>> +    }
>>      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 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;
> Couldn't qdev_find_recursive() be used here instead? This would also make patch 1 unneccessary. Same below.
>
> Best regards
> Bernhard

I tried it first, but because it tries to find the ID of a device 
instead of a type (I look for i8042 type which is a string of the device 
type), it didn't work as expected. We don't compare DeviceState id, but 
ObjectClass type->name here :)

With my patch we could just find the device without any problem whatsoever.

Best regards,
Liav

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 <liavalb@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
>>
>>

Re: [PATCH v3 1/4] hw/isa: add function to check for existence of device by its type

[Liav Albani]

On 2/27/22 09:27, Ani Sinha wrote:
>
> On Sat, 26 Feb 2022, Liav Albani wrote:
>
>> This function enumerates all attached ISA devices in the machine, and
>> tries to compare a given device type name to the enumerated devices.
>> For example, this can help other code to determine if a i8042 controller
>> exists in the machine.
>>
>> Signed-off-by: Liav Albani <liavalb@gmail.com>
>> ---
>>   hw/isa/isa-bus.c     | 23 +++++++++++++++++++++++
>>   include/hw/isa/isa.h |  1 +
>>   2 files changed, 24 insertions(+)
>>
>> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
>> index 6c31398dda..663aa36d29 100644
>> --- a/hw/isa/isa-bus.c
>> +++ b/hw/isa/isa-bus.c
>> @@ -222,6 +222,29 @@ void isa_build_aml(ISABus *bus, Aml *scope)
>>       }
>>   }
>>
>> +bool isa_check_device_existence(const char *typename)
>> +{
>> +    /*
>> +     * If there's no ISA bus, we know for sure that the checked ISA device type
>> +     * doesn't exist in the machine.
>> +     */
>> +    if (isabus == NULL) {
> nit: I would do if (!isabus) instead to keep uniformity with other parts
> of the code.
Hmm, OK, I'll change it because it seems really fine to do that this way 
too :)
>
>> +        return false;
>> +    }
>> +
>> +    BusChild *kid;
>> +    ISADevice *dev;
>> +
>> +    QTAILQ_FOREACH(kid, &isabus->parent_obj.children, sibling) {
>> +        dev = ISA_DEVICE(kid->child);
>> +        const char *object_type = object_get_typename(OBJECT(dev));
>> +        if (object_type && strcmp(object_type, typename) == 0) {
> nit: I would do !strcmp() instead.
>
Hmm, OK, I'll change it because it seems really fine to do that this way 
too :)
>> +            return true;
>> +        }
>> +    }
>> +    return false;
>> +}
>> +
>>   static void isabus_dev_print(Monitor *mon, DeviceState *dev, int indent)
>>   {
>>       ISADevice *d = ISA_DEVICE(dev);
>> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
>> index d4417b34b6..65f0c7e28c 100644
>> --- a/include/hw/isa/isa.h
>> +++ b/include/hw/isa/isa.h
>> @@ -99,6 +99,7 @@ IsaDma *isa_get_dma(ISABus *bus, int nchan);
>>   MemoryRegion *isa_address_space(ISADevice *dev);
>>   MemoryRegion *isa_address_space_io(ISADevice *dev);
>>   ISADevice *isa_new(const char *name);
>> +bool isa_check_device_existence(const char *typename);
> Please provide documentation for this function in line with other
> functions like isa_register_ioport() and isa_register_portio_list()  in
> the same header.

Ah, I see what you mean - I'll write short descriptive documentation 
like what there's for other functions :)

Thanks for the suggestions!

Best regards,
Liav

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

[Bernhard Beschow]

Am 27. Februar 2022 18:58:18 UTC schrieb Liav Albani <liavalb@gmail.com>:
>
>On 2/27/22 12:48, Bernhard Beschow wrote:
>> Am 26. Februar 2022 06:30:18 UTC schrieb Liav Albani <liavalb@gmail.com>:
>>> 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 <liavalb@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);
>>> +    }
>>>      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 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;
>> Couldn't qdev_find_recursive() be used here instead? This would also make patch 1 unneccessary. Same below.
>>
>> Best regards
>> Bernhard
>
>I tried it first, but because it tries to find the ID of a device 
>instead of a type (I look for i8042 type which is a string of the device 
>type), it didn't work as expected. We don't compare DeviceState id, but 
>ObjectClass type->name here :)

I see. What about object_resolve_path_type()? It takes a typename parameter. It even tells you if the match is ambiguous if you care.

Best regards,
Bernhard

>
>With my patch we could just find the device without any problem whatsoever.
>
>Best regards,
>Liav
>

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

[Ani Sinha]

On Mon, Feb 28, 2022 at 3:03 AM Bernhard Beschow <shentey@gmail.com> wrote:
>
> Am 27. Februar 2022 18:58:18 UTC schrieb Liav Albani <liavalb@gmail.com>:
> >
> >On 2/27/22 12:48, Bernhard Beschow wrote:
> >> Am 26. Februar 2022 06:30:18 UTC schrieb Liav Albani <liavalb@gmail.com>:
> >>> 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 <liavalb@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);
> >>> +    }
> >>>      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 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;
> >> Couldn't qdev_find_recursive() be used here instead? This would also make patch 1 unneccessary. Same below.
> >>
> >> Best regards
> >> Bernhard
> >
> >I tried it first, but because it tries to find the ID of a device
> >instead of a type (I look for i8042 type which is a string of the device
> >type), it didn't work as expected. We don't compare DeviceState id, but
> >ObjectClass type->name here :)
>
> I see. What about object_resolve_path_type()? It takes a typename parameter. It even tells you if the match is ambiguous if you care.

Yes this is a good suggestion and it will likely work.
You can get rid of your first patch and only make the following change:

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 65dbc1ec36..d82c39490c 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -38,6 +38,7 @@
 #include "hw/nvram/fw_cfg.h"
 #include "hw/acpi/bios-linker-loader.h"
 #include "hw/isa/isa.h"
+#include "hw/input/i8042.h"
 #include "hw/block/fdc.h"
 #include "hw/acpi/memory_hotplug.h"
 #include "sysemu/tpm.h"
@@ -198,7 +199,7 @@ static void init_common_fadt_data(MachineState
*ms, Object *o,
      * (the earliest acpi revision that supports this).
      */

-    fadt.iapc_boot_arch = isa_check_device_existence("i8042") ?
0x0002 : 0x0000;
+    fadt.iapc_boot_arch = object_resolve_path_type("", TYPE_I8042,
NULL) ? 0x0002 : 0x0000;

     *data = fadt;
 }
diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
index e5f89164be..502ae61a17 100644
--- a/hw/i386/acpi-microvm.c
+++ b/hw/i386/acpi-microvm.c
@@ -33,6 +33,7 @@
 #include "hw/acpi/erst.h"
 #include "hw/i386/fw_cfg.h"
 #include "hw/i386/microvm.h"
+#include "hw/input/i8042.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pcie_host.h"
 #include "hw/usb/xhci.h"
@@ -195,7 +196,7 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
      * (the earliest acpi revision that supports this).
      */

-    pmfadt.iapc_boot_arch = isa_check_device_existence("i8042") ? 0x0002
+    pmfadt.iapc_boot_arch = object_resolve_path_type("", TYPE_I8042,
NULL) ? 0x0002
                             : 0x0000;

     table_offsets = g_array_new(false, true /* clear */,

Please re-test your change.

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

[Ani Sinha]

> > >ObjectClass type->name here :)
> >
> > I see. What about object_resolve_path_type()? It takes a typename parameter. It even tells you if the match is ambiguous if you care.
>
> Yes this is a good suggestion and it will likely work.
> You can get rid of your first patch and only make the following change:
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 65dbc1ec36..d82c39490c 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -38,6 +38,7 @@
>  #include "hw/nvram/fw_cfg.h"
>  #include "hw/acpi/bios-linker-loader.h"
>  #include "hw/isa/isa.h"
> +#include "hw/input/i8042.h"
>  #include "hw/block/fdc.h"
>  #include "hw/acpi/memory_hotplug.h"
>  #include "sysemu/tpm.h"
> @@ -198,7 +199,7 @@ static void init_common_fadt_data(MachineState
> *ms, Object *o,
>       * (the earliest acpi revision that supports this).
>       */
>
> -    fadt.iapc_boot_arch = isa_check_device_existence("i8042") ?
> 0x0002 : 0x0000;
> +    fadt.iapc_boot_arch = object_resolve_path_type("", TYPE_I8042,
> NULL) ? 0x0002 : 0x0000;


This might be incorrect if there are more than one device of that type.
You need to check for ambiguity as well.

>
>      *data = fadt;
>  }
> diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
> index e5f89164be..502ae61a17 100644
> --- a/hw/i386/acpi-microvm.c
> +++ b/hw/i386/acpi-microvm.c
> @@ -33,6 +33,7 @@
>  #include "hw/acpi/erst.h"
>  #include "hw/i386/fw_cfg.h"
>  #include "hw/i386/microvm.h"
> +#include "hw/input/i8042.h"
>  #include "hw/pci/pci.h"
>  #include "hw/pci/pcie_host.h"
>  #include "hw/usb/xhci.h"
> @@ -195,7 +196,7 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
>       * (the earliest acpi revision that supports this).
>       */
>
> -    pmfadt.iapc_boot_arch = isa_check_device_existence("i8042") ? 0x0002
> +    pmfadt.iapc_boot_arch = object_resolve_path_type("", TYPE_I8042,
> NULL) ? 0x0002
>                              : 0x0000;
>

Ditto.

>      table_offsets = g_array_new(false, true /* clear */,
>
> Please re-test your change.
>

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

[Ani Sinha]

On Mon, Feb 28, 2022 at 12:26 PM Ani Sinha <ani@anisinha.ca> wrote:
>
>
>
> > > >ObjectClass type->name here :)
> > >
> > > I see. What about object_resolve_path_type()? It takes a typename parameter. It even tells you if the match is ambiguous if you care.
> >
> > Yes this is a good suggestion and it will likely work.
> > You can get rid of your first patch and only make the following change:
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 65dbc1ec36..d82c39490c 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -38,6 +38,7 @@
> >  #include "hw/nvram/fw_cfg.h"
> >  #include "hw/acpi/bios-linker-loader.h"
> >  #include "hw/isa/isa.h"
> > +#include "hw/input/i8042.h"
> >  #include "hw/block/fdc.h"
> >  #include "hw/acpi/memory_hotplug.h"
> >  #include "sysemu/tpm.h"
> > @@ -198,7 +199,7 @@ static void init_common_fadt_data(MachineState
> > *ms, Object *o,
> >       * (the earliest acpi revision that supports this).
> >       */
> >
> > -    fadt.iapc_boot_arch = isa_check_device_existence("i8042") ?
> > 0x0002 : 0x0000;
> > +    fadt.iapc_boot_arch = object_resolve_path_type("", TYPE_I8042,
> > NULL) ? 0x0002 : 0x0000;
>
>
> This might be incorrect if there are more than one device of that type.
> You need to check for ambiguity as well.

exactly one is added by default, the ISA bus with or without -nodefaults:

~/workspace/qemu/build$ echo -e "info qtree\r\nquit\r\n" |
./qemu-system-x86_64 -monitor stdio 2>/dev/null | grep 8042
          dev: i8042, id ""
~/workspace/qemu/build$ echo -e "info qtree\r\nquit\r\n" |
./qemu-system-x86_64 -nodefaults -monitor stdio 2>/dev/null | grep
8042
          dev: i8042, id ""