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

[PATCH v2 0/2] 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 (2):
  hw/isa: add function to check for existence of device by its type
  hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT
    table

 hw/acpi/aml-build.c         |  7 ++++++-
 hw/i386/acpi-build.c        |  5 +++++
 hw/i386/acpi-microvm.c      |  5 +++++
 hw/isa/isa-bus.c            | 23 +++++++++++++++++++++++
 include/hw/acpi/acpi-defs.h |  1 +
 include/hw/isa/isa.h        |  1 +
 6 files changed, 41 insertions(+), 1 deletion(-)

-- 
2.35.1

[PATCH v2 1/2] 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 v2 2/2] 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        | 5 +++++
 hw/i386/acpi-microvm.c      | 5 +++++
 include/hw/acpi/acpi-defs.h | 1 +
 4 files changed, 17 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..5dc625b8d8 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -192,6 +192,11 @@ static void init_common_fadt_data(MachineState *ms, Object *o,
             .address = object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK, NULL)
         },
     };
+    if (isa_check_device_existence("i8042")) {
+        /* Indicates if i8042 is present or not */
+        fadt.iapc_boot_arch = (1 << 1);
+    }
+        
     *data = fadt;
 }
 
diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
index 68ca7e7fc2..756c69b3b0 100644
--- a/hw/i386/acpi-microvm.c
+++ b/hw/i386/acpi-microvm.c
@@ -189,6 +189,11 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
         .reset_val = ACPI_GED_RESET_VALUE,
     };
 
+    if (isa_check_device_existence("i8042")) {
+        /* Indicates if i8042 is present or not */
+        pmfadt.iapc_boot_arch = (1 << 1);
+    }
+
     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 v2 2/2] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table

[Liav Albani]

> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index ebd47aa26f..5dc625b8d8 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -192,6 +192,11 @@ static void init_common_fadt_data(MachineState *ms, Object *o,
>               .address = object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK, NULL)
>           },
>       };
> +    if (isa_check_device_existence("i8042")) {
> +        /* Indicates if i8042 is present or not */
> +        fadt.iapc_boot_arch = (1 << 1);
> +    }
> +
Looking into this, it seems I messed up with the spaces here. So, I 
could fix this and send a v3, but want to see if there are other 
comments as well so we can get them fixed and this patch merged (or at 
least in a pull request) before the soft feature freeze in 8.3.2022 :)

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

[Ani Sinha]

On Mon, 21 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        | 5 +++++
>  hw/i386/acpi-microvm.c      | 5 +++++
>  include/hw/acpi/acpi-defs.h | 1 +
>  4 files changed, 17 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);
> +    }

So your change will only apply for q35 machines and not for pc types. You
should write a comment saying that this is not defined in acpi spec 1.0
where revision == 1 also applies.
I see that IAPC boot arch is defined as
old as ACPI version 2:

https://uefi.org/sites/default/files/resources/ACPI_2.pdf
Section 5.2.8

On a unrelatd note, I see FADT revision is hardcoded to 3 even as old as
ACPI version 2. *Except* in ACPI version 1b, it is hardcoded to 1 which
w2k seems to like :-) (table 5-5 in
https://uefi.org/sites/default/files/resources/ACPI_1_Errata_B.pdf) .
I will add a comment in the code related to this.


>      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..5dc625b8d8 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -192,6 +192,11 @@ static void init_common_fadt_data(MachineState *ms, Object *o,
>              .address = object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK, NULL)
>          },
>      };
> +    if (isa_check_device_existence("i8042")) {
> +        /* Indicates if i8042 is present or not */
> +        fadt.iapc_boot_arch = (1 << 1);
> +    }
> +
>      *data = fadt;
>  }
>
> diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
> index 68ca7e7fc2..756c69b3b0 100644
> --- a/hw/i386/acpi-microvm.c
> +++ b/hw/i386/acpi-microvm.c
> @@ -189,6 +189,11 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
>          .reset_val = ACPI_GED_RESET_VALUE,
>      };
>
> +    if (isa_check_device_existence("i8042")) {
> +        /* Indicates if i8042 is present or not */
> +        pmfadt.iapc_boot_arch = (1 << 1);
> +    }
> +
>      table_offsets = g_array_new(false, true /* clear */,
>                                          sizeof(uint32_t));


We should do the same thing for arm architecture as well?
hw/arm/virt-acpi-build.c .


>      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 v2 2/2] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table

[Ani Sinha]

>      };
> +    if (isa_check_device_existence("i8042")) {
> +        /* Indicates if i8042 is present or not */
> +        fadt.iapc_boot_arch = (1 << 1);
> +    }
> +
>      *data = fadt;
>  }
>

We do these things slightly differently. how about

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

> diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
> index 68ca7e7fc2..756c69b3b0 100644
> --- a/hw/i386/acpi-microvm.c
> +++ b/hw/i386/acpi-microvm.c
> @@ -189,6 +189,11 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
>          .reset_val = ACPI_GED_RESET_VALUE,
>      };
>
> +    if (isa_check_device_existence("i8042")) {
> +        /* Indicates if i8042 is present or not */
> +        pmfadt.iapc_boot_arch = (1 << 1);
> +    }
> +

Ditto.

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

[Ani Sinha]

> >
> > diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
> > index 68ca7e7fc2..756c69b3b0 100644
> > --- a/hw/i386/acpi-microvm.c
> > +++ b/hw/i386/acpi-microvm.c
> > @@ -189,6 +189,11 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
> >          .reset_val = ACPI_GED_RESET_VALUE,
> >      };
> >
> > +    if (isa_check_device_existence("i8042")) {
> > +        /* Indicates if i8042 is present or not */
> > +        pmfadt.iapc_boot_arch = (1 << 1);
> > +    }
> > +
> >      table_offsets = g_array_new(false, true /* clear */,
> >                                          sizeof(uint32_t));
>
>
> We should do the same thing for arm architecture as well?
> hw/arm/virt-acpi-build.c .

Probably not since the spec says
"These flags pertain only to IA-PC platforms. On other system
architectures, the entire field should be set
to 0."

adding qemu-arm for confirmation.

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

[Ani Sinha]

On Mon, 21 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.
>
> 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 (2):
>   hw/isa: add function to check for existence of device by its type
>   hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT
>     table
>
>  hw/acpi/aml-build.c         |  7 ++++++-
>  hw/i386/acpi-build.c        |  5 +++++
>  hw/i386/acpi-microvm.c      |  5 +++++
>  hw/isa/isa-bus.c            | 23 +++++++++++++++++++++++
>  include/hw/acpi/acpi-defs.h |  1 +
>  include/hw/isa/isa.h        |  1 +
>  6 files changed, 41 insertions(+), 1 deletion(-)
>

This change breaks bios-tables-test.c:

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


acpi-test: Warning! FACP binary file mismatch. Actual
[aml:/tmp/aml-QXU0H1], Expected [aml:tests/data/acpi/q35/FACP].
See source file tests/qtest/bios-tables-test.c for instructions on how to
update expected files.
# GLib-DEBUG: posix_spawn avoided (fd close requested)
# GLib-DEBUG: posix_spawn avoided (fd close requested)
acpi-test: Warning! FACP mismatch. Actual [asl:/tmp/asl-9AV0H1.dsl,
aml:/tmp/aml-QXU0H1], Expected [asl:/tmp/asl-03P0H1.dsl,
aml:tests/data/acpi/q35/FACP].
**
ERROR:../tests/qtest/bios-tables-test.c:532:test_acpi_asl: assertion
failed: (all_tables_match)
Bail out! ERROR:../tests/qtest/bios-tables-test.c:532:test_acpi_asl:
assertion failed: (all_tables_match)
Aborted (core dumped)

Please fix it. The instrctions are in the header of
tests/test/bios-tables-test.c as the above indicates.

You can check the failure by running it something like this:

QTEST_QEMU_BINARY=/home/ani/workspace/qemu/build/qemu-system-x86_64
V=1 ./tests/qtest/bios-tables-test

V=1 will dump the asl diff between expected and actual.