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

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

Posted by Liav Albani 3 years, 11 months ago

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

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

Posted by Ani Sinha 3 years, 11 months ago


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

Posted by Liav Albani 3 years, 11 months ago

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

Posted by Bernhard Beschow 3 years, 11 months ago

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

Posted by Liav Albani 3 years, 11 months ago

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

Posted by Bernhard Beschow 3 years, 11 months ago

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

Posted by Ani Sinha 3 years, 11 months ago

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

Posted by Ani Sinha 3 years, 11 months ago


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

Posted by Ani Sinha 3 years, 11 months ago

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