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.

This change only applies to the x86/q35 machine type, as it uses FACP
ACPI table with revision higher than 1, which should implement at least
ACPI 2.0 features within the table, hence it can also set the IA-PC boot
flags register according to the ACPI 2.0 specification.

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

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 8966e16320..2085905b83 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2152,7 +2152,16 @@ 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 */
+    /*
+     * This register is not defined in ACPI spec version 1.0, where the FACP
+     * revision == 1 also applies. Therefore, just ignore setting this register.
+     */
+    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..c72c7bb9bb 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"
@@ -192,6 +193,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 of the IAPC_BOOT_ARCH register indicates i8042 presence
+     * 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 = 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 68ca7e7fc2..4bc72b1672 100644
--- a/hw/i386/acpi-microvm.c
+++ b/hw/i386/acpi-microvm.c
@@ -31,6 +31,7 @@
 #include "hw/acpi/generic_event_device.h"
 #include "hw/acpi/utils.h"
 #include "hw/acpi/erst.h"
+#include "hw/input/i8042.h"
 #include "hw/i386/fw_cfg.h"
 #include "hw/i386/microvm.h"
 #include "hw/pci/pci.h"
@@ -189,6 +190,14 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
         .reset_val = ACPI_GED_RESET_VALUE,
     };
 
+    /*
+     * second bit of 16 of the IAPC_BOOT_ARCH register indicates i8042 presence
+     * 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 = object_resolve_path_type("", TYPE_I8042, NULL) ?
+                            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

On Mon, 28 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.
>
> This change only applies to the x86/q35 machine type, as it uses FACP
> ACPI table with revision higher than 1, which should implement at least
> ACPI 2.0 features within the table, hence it can also set the IA-PC boot
> flags register according to the ACPI 2.0 specification.
>
> Signed-off-by: Liav Albani <liavalb@gmail.com>
> ---
>  hw/acpi/aml-build.c         | 11 ++++++++++-
>  hw/i386/acpi-build.c        |  9 +++++++++
>  hw/i386/acpi-microvm.c      |  9 +++++++++
>  include/hw/acpi/acpi-defs.h |  1 +
>  4 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 8966e16320..2085905b83 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -2152,7 +2152,16 @@ 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 */
> +    /*
> +     * This register is not defined in ACPI spec version 1.0, where the FACP

I'd say "this IAPC_BOOT_ARCH register" to be more specific.

> +     * revision == 1 also applies. Therefore, just ignore setting this register.
> +     */
> +    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..c72c7bb9bb 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"
> @@ -192,6 +193,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 of the IAPC_BOOT_ARCH register indicates i8042 presence

again typo here.

> +     * 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 = object_resolve_path_type("", TYPE_I8042, NULL) ?
> +                            0x0002 : 0x0000;

I thought I said we need to make sure the logic still applied when there
are more than one device of this type. Please fix this.

> +
>      *data = fadt;
>  }
>
> diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
> index 68ca7e7fc2..4bc72b1672 100644
> --- a/hw/i386/acpi-microvm.c
> +++ b/hw/i386/acpi-microvm.c
> @@ -31,6 +31,7 @@
>  #include "hw/acpi/generic_event_device.h"
>  #include "hw/acpi/utils.h"
>  #include "hw/acpi/erst.h"
> +#include "hw/input/i8042.h"
>  #include "hw/i386/fw_cfg.h"
>  #include "hw/i386/microvm.h"
>  #include "hw/pci/pci.h"
> @@ -189,6 +190,14 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
>          .reset_val = ACPI_GED_RESET_VALUE,
>      };
>
> +    /*
> +     * second bit of 16 of the IAPC_BOOT_ARCH register indicates i8042 presence
> +     * 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 = object_resolve_path_type("", TYPE_I8042, NULL) ?
> +                            0x0002 : 0x0000;


Ditto.

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

On Tue, 1 Mar 2022 08:29:05 +0530 (IST)
Ani Sinha <ani@anisinha.ca> wrote:

> On Mon, 28 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.
> >
> > This change only applies to the x86/q35 machine type, as it uses FACP
> > ACPI table with revision higher than 1, which should implement at least
> > ACPI 2.0 features within the table, hence it can also set the IA-PC boot
> > flags register according to the ACPI 2.0 specification.
> >
> > Signed-off-by: Liav Albani <liavalb@gmail.com>
> > ---
> >  hw/acpi/aml-build.c         | 11 ++++++++++-
> >  hw/i386/acpi-build.c        |  9 +++++++++
> >  hw/i386/acpi-microvm.c      |  9 +++++++++
> >  include/hw/acpi/acpi-defs.h |  1 +
> >  4 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index 8966e16320..2085905b83 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -2152,7 +2152,16 @@ 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 */
> > +    /*
> > +     * This register is not defined in ACPI spec version 1.0, where the FACP  
> 
> I'd say "this IAPC_BOOT_ARCH register" to be more specific.
> 
> > +     * revision == 1 also applies. Therefore, just ignore setting this register.
> > +     */

I'd drop this comment altogether, like it's done with the rest of the fields  in this function

> > +    if (f->rev == 1) {
> > +        build_append_int_noprefix(tbl, 0, 2);
> > +    } else {
maybe add here
/* Since ACPI 2.0 */

> > +        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..c72c7bb9bb 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"
> > @@ -192,6 +193,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 of the IAPC_BOOT_ARCH register indicates i8042 presence  
> 
> again typo here.
> 
> > +     * 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 = object_resolve_path_type("", TYPE_I8042, NULL) ?
> > +                            0x0002 : 0x0000;  
> 
> I thought I said we need to make sure the logic still applied when there
> are more than one device of this type. Please fix this.
> 
> > +
> >      *data = fadt;
> >  }
> >
> > diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
> > index 68ca7e7fc2..4bc72b1672 100644
> > --- a/hw/i386/acpi-microvm.c
> > +++ b/hw/i386/acpi-microvm.c
> > @@ -31,6 +31,7 @@
> >  #include "hw/acpi/generic_event_device.h"
> >  #include "hw/acpi/utils.h"
> >  #include "hw/acpi/erst.h"
> > +#include "hw/input/i8042.h"
> >  #include "hw/i386/fw_cfg.h"
> >  #include "hw/i386/microvm.h"
> >  #include "hw/pci/pci.h"
> > @@ -189,6 +190,14 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
> >          .reset_val = ACPI_GED_RESET_VALUE,
> >      };
> >
> > +    /*
> > +     * second bit of 16 of the IAPC_BOOT_ARCH register indicates i8042 presence
> > +     * 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 = object_resolve_path_type("", TYPE_I8042, NULL) ?
> > +                            0x0002 : 0x0000;  
> 
> 
> Ditto.
> 
> > +
> >      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
> >
> >  
>

On Mon, 28 Feb 2022 22:17:32 +0200
Liav Albani <liavalb@gmail.com> 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.
> 
> This change only applies to the x86/q35 machine type, as it uses FACP
> ACPI table with revision higher than 1, which should implement at least
> ACPI 2.0 features within the table, hence it can also set the IA-PC boot
> flags register according to the ACPI 2.0 specification.
> 
> Signed-off-by: Liav Albani <liavalb@gmail.com>
> ---
>  hw/acpi/aml-build.c         | 11 ++++++++++-
>  hw/i386/acpi-build.c        |  9 +++++++++
>  hw/i386/acpi-microvm.c      |  9 +++++++++
commit message says it's q35 specific, so wy it touched microvm anc piix4?

>  include/hw/acpi/acpi-defs.h |  1 +
>  4 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 8966e16320..2085905b83 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -2152,7 +2152,16 @@ 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 */
> +    /*
> +     * This register is not defined in ACPI spec version 1.0, where the FACP
> +     * revision == 1 also applies. Therefore, just ignore setting this register.
> +     */
> +    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..c72c7bb9bb 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"
> @@ -192,6 +193,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 of the IAPC_BOOT_ARCH register indicates i8042 presence
> +     * or equivalent micro controller. See table 5-10 of APCI spec version 2.0
> +     * (the earliest acpi revision that supports this).

 /* APCI spec version 2.0, Table 5-10 */

is sufficient, the rest could be read from spec/

> +     */
> +    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 68ca7e7fc2..4bc72b1672 100644
> --- a/hw/i386/acpi-microvm.c
> +++ b/hw/i386/acpi-microvm.c
> @@ -31,6 +31,7 @@
>  #include "hw/acpi/generic_event_device.h"
>  #include "hw/acpi/utils.h"
>  #include "hw/acpi/erst.h"
> +#include "hw/input/i8042.h"
>  #include "hw/i386/fw_cfg.h"
>  #include "hw/i386/microvm.h"
>  #include "hw/pci/pci.h"
> @@ -189,6 +190,14 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
>          .reset_val = ACPI_GED_RESET_VALUE,
>      };
>  
> +    /*
> +     * second bit of 16 of the IAPC_BOOT_ARCH register indicates i8042 presence
> +     * 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 = object_resolve_path_type("", TYPE_I8042, NULL) ?
> +                            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 */
>  
>      /*

On Tue, 1 Mar 2022, Igor Mammedov wrote:

> On Mon, 28 Feb 2022 22:17:32 +0200
> Liav Albani <liavalb@gmail.com> 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.
> >
> > This change only applies to the x86/q35 machine type, as it uses FACP
> > ACPI table with revision higher than 1, which should implement at least
> > ACPI 2.0 features within the table, hence it can also set the IA-PC boot
> > flags register according to the ACPI 2.0 specification.
> >
> > Signed-off-by: Liav Albani <liavalb@gmail.com>
> > ---
> >  hw/acpi/aml-build.c         | 11 ++++++++++-
> >  hw/i386/acpi-build.c        |  9 +++++++++
> >  hw/i386/acpi-microvm.c      |  9 +++++++++
> commit message says it's q35 specific, so wy it touched microvm anc piix4?

Igor is correct. Although I see that currently there are no 8042 devices
for microvms, maybe we should be conservative and add the code to detect
the device anyway. In that case, the change could affect microvms too when
such devices get added in the future.


echo -e "info qtree\r\nquit\r\n" | ./qemu-system-x86_64 -machine microvm
-monitor stdio 2>/dev/null | grep 8042

<empty>

On Tue, 1 Mar 2022 15:22:17 +0530 (IST)
Ani Sinha <ani@anisinha.ca> wrote:

> On Tue, 1 Mar 2022, Igor Mammedov wrote:
> 
> > On Mon, 28 Feb 2022 22:17:32 +0200
> > Liav Albani <liavalb@gmail.com> 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.
> > >
> > > This change only applies to the x86/q35 machine type, as it uses FACP
> > > ACPI table with revision higher than 1, which should implement at least
> > > ACPI 2.0 features within the table, hence it can also set the IA-PC boot
> > > flags register according to the ACPI 2.0 specification.
> > >
> > > Signed-off-by: Liav Albani <liavalb@gmail.com>
> > > ---
> > >  hw/acpi/aml-build.c         | 11 ++++++++++-
> > >  hw/i386/acpi-build.c        |  9 +++++++++
> > >  hw/i386/acpi-microvm.c      |  9 +++++++++  
> > commit message says it's q35 specific, so wy it touched microvm anc piix4?  
> 
> Igor is correct. Although I see that currently there are no 8042 devices
> for microvms, maybe we should be conservative and add the code to detect
> the device anyway. In that case, the change could affect microvms too when
> such devices get added in the future.

when that case actually arises implement it then, so I'd say don't generalize
that unless it's actually used within series.
Or planned to be used in near future in which case commit message should
mention that.

> 
> echo -e "info qtree\r\nquit\r\n" | ./qemu-system-x86_64 -machine microvm
> -monitor stdio 2>/dev/null | grep 8042
> 
> <empty>
>

On 3/1/22 11:52, Ani Sinha wrote:
>
> On Tue, 1 Mar 2022, Igor Mammedov wrote:
>
>> On Mon, 28 Feb 2022 22:17:32 +0200
>> Liav Albani <liavalb@gmail.com> 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.
>>>
>>> This change only applies to the x86/q35 machine type, as it uses FACP
>>> ACPI table with revision higher than 1, which should implement at least
>>> ACPI 2.0 features within the table, hence it can also set the IA-PC boot
>>> flags register according to the ACPI 2.0 specification.
>>>
>>> Signed-off-by: Liav Albani <liavalb@gmail.com>
>>> ---
>>>   hw/acpi/aml-build.c         | 11 ++++++++++-
>>>   hw/i386/acpi-build.c        |  9 +++++++++
>>>   hw/i386/acpi-microvm.c      |  9 +++++++++
>> commit message says it's q35 specific, so wy it touched microvm anc piix4?
> Igor is correct. Although I see that currently there are no 8042 devices
> for microvms, maybe we should be conservative and add the code to detect
> the device anyway. In that case, the change could affect microvms too when
> such devices get added in the future.
>
>
> echo -e "info qtree\r\nquit\r\n" | ./qemu-system-x86_64 -machine microvm
> -monitor stdio 2>/dev/null | grep 8042
>
> <empty>

What about this?

echo -e "info qtree\r\nquit\r\n" | qemu-system-x86_64 -machine microvm 
-device i8042 -monitor stdio 2>/dev/null | grep 8042

Or this?

echo -e "info mtree\r\nquit\r\n" | qemu-system-x86_64 -machine microvm 
-device i8042 -monitor stdio 2>/dev/null | grep 8042

>

On Wed, Mar 2, 2022 at 12:50 AM Liav Albani <liavalb@gmail.com> wrote:
>
>
> On 3/1/22 11:52, Ani Sinha wrote:
> >
> > On Tue, 1 Mar 2022, Igor Mammedov wrote:
> >
> >> On Mon, 28 Feb 2022 22:17:32 +0200
> >> Liav Albani <liavalb@gmail.com> 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.
> >>>
> >>> This change only applies to the x86/q35 machine type, as it uses FACP
> >>> ACPI table with revision higher than 1, which should implement at least
> >>> ACPI 2.0 features within the table, hence it can also set the IA-PC boot
> >>> flags register according to the ACPI 2.0 specification.
> >>>
> >>> Signed-off-by: Liav Albani <liavalb@gmail.com>
> >>> ---
> >>>   hw/acpi/aml-build.c         | 11 ++++++++++-
> >>>   hw/i386/acpi-build.c        |  9 +++++++++
> >>>   hw/i386/acpi-microvm.c      |  9 +++++++++
> >> commit message says it's q35 specific, so wy it touched microvm anc piix4?
> > Igor is correct. Although I see that currently there are no 8042 devices
> > for microvms, maybe we should be conservative and add the code to detect
> > the device anyway. In that case, the change could affect microvms too when
> > such devices get added in the future.
> >
> >
> > echo -e "info qtree\r\nquit\r\n" | ./qemu-system-x86_64 -machine microvm
> > -monitor stdio 2>/dev/null | grep 8042
> >
> > <empty>
>
> What about this?
>
> echo -e "info qtree\r\nquit\r\n" | qemu-system-x86_64 -machine microvm
> -device i8042 -monitor stdio 2>/dev/null | grep 8042
>
> Or this?
>
> echo -e "info mtree\r\nquit\r\n" | qemu-system-x86_64 -machine microvm
> -device i8042 -monitor stdio 2>/dev/null | grep 8042

On both occasions you are explicitly adding the device.

On Wed, Mar 02, 2022 at 10:44:03AM +0530, Ani Sinha wrote:
> On Wed, Mar 2, 2022 at 12:50 AM Liav Albani <liavalb@gmail.com> wrote:
> >
> >
> > On 3/1/22 11:52, Ani Sinha wrote:
> > >
> > > On Tue, 1 Mar 2022, Igor Mammedov wrote:
> > >
> > >> On Mon, 28 Feb 2022 22:17:32 +0200
> > >> Liav Albani <liavalb@gmail.com> 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.
> > >>>
> > >>> This change only applies to the x86/q35 machine type, as it uses FACP
> > >>> ACPI table with revision higher than 1, which should implement at least
> > >>> ACPI 2.0 features within the table, hence it can also set the IA-PC boot
> > >>> flags register according to the ACPI 2.0 specification.
> > >>>
> > >>> Signed-off-by: Liav Albani <liavalb@gmail.com>
> > >>> ---
> > >>>   hw/acpi/aml-build.c         | 11 ++++++++++-
> > >>>   hw/i386/acpi-build.c        |  9 +++++++++
> > >>>   hw/i386/acpi-microvm.c      |  9 +++++++++
> > >> commit message says it's q35 specific, so wy it touched microvm anc piix4?
> > > Igor is correct. Although I see that currently there are no 8042 devices
> > > for microvms, maybe we should be conservative and add the code to detect
> > > the device anyway. In that case, the change could affect microvms too when
> > > such devices get added in the future.
> > >
> > >
> > > echo -e "info qtree\r\nquit\r\n" | ./qemu-system-x86_64 -machine microvm
> > > -monitor stdio 2>/dev/null | grep 8042
> > >
> > > <empty>
> >
> > What about this?
> >
> > echo -e "info qtree\r\nquit\r\n" | qemu-system-x86_64 -machine microvm
> > -device i8042 -monitor stdio 2>/dev/null | grep 8042
> >
> > Or this?
> >
> > echo -e "info mtree\r\nquit\r\n" | qemu-system-x86_64 -machine microvm
> > -device i8042 -monitor stdio 2>/dev/null | grep 8042
> 
> On both occasions you are explicitly adding the device.

Yes of course. It seems a bit cleaner to have -device i8042 -monitor
stdio give us the correct ACPI table even if there's no pressing need
for this ATM, simply because it's not much more code, and because if we
don't we risk guests trying to work around incorrect ACPI tables.
Let us however do this in a patch by its own with proper
documentation and motivation.

-- 
MST

On Wed, Mar 2, 2022 at 6:12 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Mar 02, 2022 at 10:44:03AM +0530, Ani Sinha wrote:
> > On Wed, Mar 2, 2022 at 12:50 AM Liav Albani <liavalb@gmail.com> wrote:
> > >
> > >
> > > On 3/1/22 11:52, Ani Sinha wrote:
> > > >
> > > > On Tue, 1 Mar 2022, Igor Mammedov wrote:
> > > >
> > > >> On Mon, 28 Feb 2022 22:17:32 +0200
> > > >> Liav Albani <liavalb@gmail.com> 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.
> > > >>>
> > > >>> This change only applies to the x86/q35 machine type, as it uses FACP
> > > >>> ACPI table with revision higher than 1, which should implement at least
> > > >>> ACPI 2.0 features within the table, hence it can also set the IA-PC boot
> > > >>> flags register according to the ACPI 2.0 specification.
> > > >>>
> > > >>> Signed-off-by: Liav Albani <liavalb@gmail.com>
> > > >>> ---
> > > >>>   hw/acpi/aml-build.c         | 11 ++++++++++-
> > > >>>   hw/i386/acpi-build.c        |  9 +++++++++
> > > >>>   hw/i386/acpi-microvm.c      |  9 +++++++++
> > > >> commit message says it's q35 specific, so wy it touched microvm anc piix4?
> > > > Igor is correct. Although I see that currently there are no 8042 devices
> > > > for microvms, maybe we should be conservative and add the code to detect
> > > > the device anyway. In that case, the change could affect microvms too when
> > > > such devices get added in the future.
> > > >
> > > >
> > > > echo -e "info qtree\r\nquit\r\n" | ./qemu-system-x86_64 -machine microvm
> > > > -monitor stdio 2>/dev/null | grep 8042
> > > >
> > > > <empty>
> > >
> > > What about this?
> > >
> > > echo -e "info qtree\r\nquit\r\n" | qemu-system-x86_64 -machine microvm
> > > -device i8042 -monitor stdio 2>/dev/null | grep 8042
> > >
> > > Or this?
> > >
> > > echo -e "info mtree\r\nquit\r\n" | qemu-system-x86_64 -machine microvm
> > > -device i8042 -monitor stdio 2>/dev/null | grep 8042
> >
> > On both occasions you are explicitly adding the device.
>
> Yes of course.

OK. I did not think for a "microvm" one would explicitly add more 8042
devices beyond what was added by default in the real life use case.

On 3/2/22 14:42, Michael S. Tsirkin wrote:
> On Wed, Mar 02, 2022 at 10:44:03AM +0530, Ani Sinha wrote:
>> On Wed, Mar 2, 2022 at 12:50 AM Liav Albani <liavalb@gmail.com> wrote:
>>>
>>> On 3/1/22 11:52, Ani Sinha wrote:
>>>> On Tue, 1 Mar 2022, Igor Mammedov wrote:
>>>>
>>>>> On Mon, 28 Feb 2022 22:17:32 +0200
>>>>> Liav Albani <liavalb@gmail.com> 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.
>>>>>>
>>>>>> This change only applies to the x86/q35 machine type, as it uses FACP
>>>>>> ACPI table with revision higher than 1, which should implement at least
>>>>>> ACPI 2.0 features within the table, hence it can also set the IA-PC boot
>>>>>> flags register according to the ACPI 2.0 specification.
>>>>>>
>>>>>> Signed-off-by: Liav Albani <liavalb@gmail.com>
>>>>>> ---
>>>>>>    hw/acpi/aml-build.c         | 11 ++++++++++-
>>>>>>    hw/i386/acpi-build.c        |  9 +++++++++
>>>>>>    hw/i386/acpi-microvm.c      |  9 +++++++++
>>>>> commit message says it's q35 specific, so wy it touched microvm anc piix4?
>>>> Igor is correct. Although I see that currently there are no 8042 devices
>>>> for microvms, maybe we should be conservative and add the code to detect
>>>> the device anyway. In that case, the change could affect microvms too when
>>>> such devices get added in the future.
>>>>
>>>>
>>>> echo -e "info qtree\r\nquit\r\n" | ./qemu-system-x86_64 -machine microvm
>>>> -monitor stdio 2>/dev/null | grep 8042
>>>>
>>>> <empty>
>>> What about this?
>>>
>>> echo -e "info qtree\r\nquit\r\n" | qemu-system-x86_64 -machine microvm
>>> -device i8042 -monitor stdio 2>/dev/null | grep 8042
>>>
>>> Or this?
>>>
>>> echo -e "info mtree\r\nquit\r\n" | qemu-system-x86_64 -machine microvm
>>> -device i8042 -monitor stdio 2>/dev/null | grep 8042
>> On both occasions you are explicitly adding the device.
> Yes of course. It seems a bit cleaner to have -device i8042 -monitor
> stdio give us the correct ACPI table even if there's no pressing need
> for this ATM, simply because it's not much more code, and because if we
> don't we risk guests trying to work around incorrect ACPI tables.
> Let us however do this in a patch by its own with proper
> documentation and motivation.
>
So if I understand how to do this now - I should drop the code for the 
MicroVM ACPI for now, letting only to change the Q35 FACP table, right? 
So if that's the case I should send it in a separate patch?

If that's the case, as suggested by you and Ani, I'll not add a separate 
function to reduce code duplication as there's no such thing in such case...

On Wed, Mar 2, 2022 at 21:13 Liav Albani <liavalb@gmail.com> wrote:

>
> On 3/2/22 14:42, Michael S. Tsirkin wrote:
> > On Wed, Mar 02, 2022 at 10:44:03AM +0530, Ani Sinha wrote:
> >> On Wed, Mar 2, 2022 at 12:50 AM Liav Albani <liavalb@gmail.com> wrote:
> >>>
> >>> On 3/1/22 11:52, Ani Sinha wrote:
> >>>> On Tue, 1 Mar 2022, Igor Mammedov wrote:
> >>>>
> >>>>> On Mon, 28 Feb 2022 22:17:32 +0200
> >>>>> Liav Albani <liavalb@gmail.com> 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.
> >>>>>>
> >>>>>> This change only applies to the x86/q35 machine type, as it uses
> FACP
> >>>>>> ACPI table with revision higher than 1, which should implement at
> least
> >>>>>> ACPI 2.0 features within the table, hence it can also set the IA-PC
> boot
> >>>>>> flags register according to the ACPI 2.0 specification.
> >>>>>>
> >>>>>> Signed-off-by: Liav Albani <liavalb@gmail.com>
> >>>>>> ---
> >>>>>>    hw/acpi/aml-build.c         | 11 ++++++++++-
> >>>>>>    hw/i386/acpi-build.c        |  9 +++++++++
> >>>>>>    hw/i386/acpi-microvm.c      |  9 +++++++++
> >>>>> commit message says it's q35 specific, so wy it touched microvm anc
> piix4?
> >>>> Igor is correct. Although I see that currently there are no 8042
> devices
> >>>> for microvms, maybe we should be conservative and add the code to
> detect
> >>>> the device anyway. In that case, the change could affect microvms too
> when
> >>>> such devices get added in the future.
> >>>>
> >>>>
> >>>> echo -e "info qtree\r\nquit\r\n" | ./qemu-system-x86_64 -machine
> microvm
> >>>> -monitor stdio 2>/dev/null | grep 8042
> >>>>
> >>>> <empty>
> >>> What about this?
> >>>
> >>> echo -e "info qtree\r\nquit\r\n" | qemu-system-x86_64 -machine microvm
> >>> -device i8042 -monitor stdio 2>/dev/null | grep 8042
> >>>
> >>> Or this?
> >>>
> >>> echo -e "info mtree\r\nquit\r\n" | qemu-system-x86_64 -machine microvm
> >>> -device i8042 -monitor stdio 2>/dev/null | grep 8042
> >> On both occasions you are explicitly adding the device.
> > Yes of course. It seems a bit cleaner to have -device i8042 -monitor
> > stdio give us the correct ACPI table even if there's no pressing need
> > for this ATM, simply because it's not much more code, and because if we
> > don't we risk guests trying to work around incorrect ACPI tables.
> > Let us however do this in a patch by its own with proper
> > documentation and motivation.
> >
> So if I understand how to do this now - I should drop the code for the
> MicroVM ACPI for now, letting only to change the Q35 FACP table, right?


Correct. Microvm changes can go in a separate patch.


> So if that's the case I should send it in a separate patch?


Yes.


>
> If that's the case, as suggested by you and Ani, I'll not add a separate
> function to reduce code duplication as there's no such thing in such
> case...


Please add the function regardless.


>
>

On Tue, Mar 01, 2022 at 09:43:54AM +0100, Igor Mammedov wrote:
> On Mon, 28 Feb 2022 22:17:32 +0200
> Liav Albani <liavalb@gmail.com> 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.
> > 
> > This change only applies to the x86/q35 machine type, as it uses FACP
> > ACPI table with revision higher than 1, which should implement at least
> > ACPI 2.0 features within the table, hence it can also set the IA-PC boot
> > flags register according to the ACPI 2.0 specification.
> > 
> > Signed-off-by: Liav Albani <liavalb@gmail.com>
> > ---
> >  hw/acpi/aml-build.c         | 11 ++++++++++-
> >  hw/i386/acpi-build.c        |  9 +++++++++
> >  hw/i386/acpi-microvm.c      |  9 +++++++++
> commit message says it's q35 specific, so wy it touched microvm anc piix4?
> 
> >  include/hw/acpi/acpi-defs.h |  1 +
> >  4 files changed, 29 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index 8966e16320..2085905b83 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -2152,7 +2152,16 @@ 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 */
> > +    /*
> > +     * This register is not defined in ACPI spec version 1.0, where the FACP
> > +     * revision == 1 also applies. Therefore, just ignore setting this register.
> > +     */
> > +    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..c72c7bb9bb 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"
> > @@ -192,6 +193,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 of the IAPC_BOOT_ARCH register indicates i8042 presence
> > +     * or equivalent micro controller. See table 5-10 of APCI spec version 2.0
> > +     * (the earliest acpi revision that supports this).
> 
>  /* APCI spec version 2.0, Table 5-10 */
> 
> is sufficient, the rest could be read from spec/

ACPI though, not APCI.
The comment can be shorter and more clearer, but I feel quoting spec
and including table name is a good idea actually, but pls quote verbatim:

/* ACPI spec version 2.0, Table 5-10: Fixed ACPI Description Table Boot Architecture Flags */
/* Bit offset 1 -  port 60 and 64 based keyboard controller, usually implemented as an 8042 or equivalent micro-controller. */

> 
> > +     */
> > +    fadt.iapc_boot_arch = object_resolve_path_type("", TYPE_I8042, NULL) ?
> > +                            0x0002 : 0x0000;

and make it 0x1 << 1 - clearer that this is bit 1. Leading zeroes are
not helpful since compiler does not check there's a correct number of
these.

> > +
> >      *data = fadt;
> >  }
> >  
> > diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
> > index 68ca7e7fc2..4bc72b1672 100644
> > --- a/hw/i386/acpi-microvm.c
> > +++ b/hw/i386/acpi-microvm.c
> > @@ -31,6 +31,7 @@
> >  #include "hw/acpi/generic_event_device.h"
> >  #include "hw/acpi/utils.h"
> >  #include "hw/acpi/erst.h"
> > +#include "hw/input/i8042.h"
> >  #include "hw/i386/fw_cfg.h"
> >  #include "hw/i386/microvm.h"
> >  #include "hw/pci/pci.h"
> > @@ -189,6 +190,14 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
> >          .reset_val = ACPI_GED_RESET_VALUE,
> >      };
> >  
> > +    /*
> > +     * second bit of 16 of the IAPC_BOOT_ARCH register indicates i8042 presence
> > +     * 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 = object_resolve_path_type("", TYPE_I8042, NULL) ?
> > +                            0x0002 : 0x0000;
> > +

let's avoid code duplication pls.

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

On 3/1/22 13:19, Michael S. Tsirkin wrote:
> On Tue, Mar 01, 2022 at 09:43:54AM +0100, Igor Mammedov wrote:
>> On Mon, 28 Feb 2022 22:17:32 +0200
>> Liav Albani <liavalb@gmail.com> 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.
>>>
>>> This change only applies to the x86/q35 machine type, as it uses FACP
>>> ACPI table with revision higher than 1, which should implement at least
>>> ACPI 2.0 features within the table, hence it can also set the IA-PC boot
>>> flags register according to the ACPI 2.0 specification.
>>>
>>> Signed-off-by: Liav Albani <liavalb@gmail.com>
>>> ---
>>>   hw/acpi/aml-build.c         | 11 ++++++++++-
>>>   hw/i386/acpi-build.c        |  9 +++++++++
>>>   hw/i386/acpi-microvm.c      |  9 +++++++++
>> commit message says it's q35 specific, so wy it touched microvm anc piix4?
>>
This affect only q35 machine type for now, but what happens if the 
MicroVM ACPI FACP table is updated to use a revision that supports IA-PC 
boot flags?
>>>   include/hw/acpi/acpi-defs.h |  1 +
>>>   4 files changed, 29 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>> index 8966e16320..2085905b83 100644
>>> --- a/hw/acpi/aml-build.c
>>> +++ b/hw/acpi/aml-build.c
>>> @@ -2152,7 +2152,16 @@ 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 */
>>> +    /*
>>> +     * This register is not defined in ACPI spec version 1.0, where the FACP
>>> +     * revision == 1 also applies. Therefore, just ignore setting this register.
>>> +     */
>>> +    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..c72c7bb9bb 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"
>>> @@ -192,6 +193,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 of the IAPC_BOOT_ARCH register indicates i8042 presence
>>> +     * or equivalent micro controller. See table 5-10 of APCI spec version 2.0
>>> +     * (the earliest acpi revision that supports this).
>>   /* APCI spec version 2.0, Table 5-10 */
>>
>> is sufficient, the rest could be read from spec/
> ACPI though, not APCI.
It will be fixed in version 5.
> The comment can be shorter and more clearer, but I feel quoting spec
> and including table name is a good idea actually, but pls quote verbatim:
>
> /* ACPI spec version 2.0, Table 5-10: Fixed ACPI Description Table Boot Architecture Flags */
> /* Bit offset 1 -  port 60 and 64 based keyboard controller, usually implemented as an 8042 or equivalent micro-controller. */
>
>>> +     */
>>> +    fadt.iapc_boot_arch = object_resolve_path_type("", TYPE_I8042, NULL) ?
>>> +                            0x0002 : 0x0000;
> and make it 0x1 << 1 - clearer that this is bit 1. Leading zeroes are
> not helpful since compiler does not check there's a correct number of
> these.
It will be fixed in version 5.
>>> +
>>>       *data = fadt;
>>>   }
>>>   
>>> diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
>>> index 68ca7e7fc2..4bc72b1672 100644
>>> --- a/hw/i386/acpi-microvm.c
>>> +++ b/hw/i386/acpi-microvm.c
>>> @@ -31,6 +31,7 @@
>>>   #include "hw/acpi/generic_event_device.h"
>>>   #include "hw/acpi/utils.h"
>>>   #include "hw/acpi/erst.h"
>>> +#include "hw/input/i8042.h"
>>>   #include "hw/i386/fw_cfg.h"
>>>   #include "hw/i386/microvm.h"
>>>   #include "hw/pci/pci.h"
>>> @@ -189,6 +190,14 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
>>>           .reset_val = ACPI_GED_RESET_VALUE,
>>>       };
>>>   
>>> +    /*
>>> +     * second bit of 16 of the IAPC_BOOT_ARCH register indicates i8042 presence
>>> +     * 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 = object_resolve_path_type("", TYPE_I8042, NULL) ?
>>> +                            0x0002 : 0x0000;
>>> +
> let's avoid code duplication pls.
>
What do you suggest to do with this? Creating a separate function to do 
this for us so we can call it from both locations?
>>>       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 */
>>>   
>>>       /*

On Tue, Mar 01, 2022 at 09:11:09PM +0200, Liav Albani wrote:
> 
> On 3/1/22 13:19, Michael S. Tsirkin wrote:
> > On Tue, Mar 01, 2022 at 09:43:54AM +0100, Igor Mammedov wrote:
> > > On Mon, 28 Feb 2022 22:17:32 +0200
> > > Liav Albani <liavalb@gmail.com> 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.
> > > > 
> > > > This change only applies to the x86/q35 machine type, as it uses FACP
> > > > ACPI table with revision higher than 1, which should implement at least
> > > > ACPI 2.0 features within the table, hence it can also set the IA-PC boot
> > > > flags register according to the ACPI 2.0 specification.
> > > > 
> > > > Signed-off-by: Liav Albani <liavalb@gmail.com>
> > > > ---
> > > >   hw/acpi/aml-build.c         | 11 ++++++++++-
> > > >   hw/i386/acpi-build.c        |  9 +++++++++
> > > >   hw/i386/acpi-microvm.c      |  9 +++++++++
> > > commit message says it's q35 specific, so wy it touched microvm anc piix4?
> > > 
> This affect only q35 machine type for now, but what happens if the MicroVM
> ACPI FACP table is updated to use a revision that supports IA-PC boot flags?
> > > >   include/hw/acpi/acpi-defs.h |  1 +
> > > >   4 files changed, 29 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > > > index 8966e16320..2085905b83 100644
> > > > --- a/hw/acpi/aml-build.c
> > > > +++ b/hw/acpi/aml-build.c
> > > > @@ -2152,7 +2152,16 @@ 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 */
> > > > +    /*
> > > > +     * This register is not defined in ACPI spec version 1.0, where the FACP
> > > > +     * revision == 1 also applies. Therefore, just ignore setting this register.
> > > > +     */
> > > > +    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..c72c7bb9bb 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"
> > > > @@ -192,6 +193,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 of the IAPC_BOOT_ARCH register indicates i8042 presence
> > > > +     * or equivalent micro controller. See table 5-10 of APCI spec version 2.0
> > > > +     * (the earliest acpi revision that supports this).
> > >   /* APCI spec version 2.0, Table 5-10 */
> > > 
> > > is sufficient, the rest could be read from spec/
> > ACPI though, not APCI.
> It will be fixed in version 5.
> > The comment can be shorter and more clearer, but I feel quoting spec
> > and including table name is a good idea actually, but pls quote verbatim:
> > 
> > /* ACPI spec version 2.0, Table 5-10: Fixed ACPI Description Table Boot Architecture Flags */
> > /* Bit offset 1 -  port 60 and 64 based keyboard controller, usually implemented as an 8042 or equivalent micro-controller. */
> > 
> > > > +     */
> > > > +    fadt.iapc_boot_arch = object_resolve_path_type("", TYPE_I8042, NULL) ?
> > > > +                            0x0002 : 0x0000;
> > and make it 0x1 << 1 - clearer that this is bit 1. Leading zeroes are
> > not helpful since compiler does not check there's a correct number of
> > these.
> It will be fixed in version 5.
> > > > +
> > > >       *data = fadt;
> > > >   }
> > > > diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
> > > > index 68ca7e7fc2..4bc72b1672 100644
> > > > --- a/hw/i386/acpi-microvm.c
> > > > +++ b/hw/i386/acpi-microvm.c
> > > > @@ -31,6 +31,7 @@
> > > >   #include "hw/acpi/generic_event_device.h"
> > > >   #include "hw/acpi/utils.h"
> > > >   #include "hw/acpi/erst.h"
> > > > +#include "hw/input/i8042.h"
> > > >   #include "hw/i386/fw_cfg.h"
> > > >   #include "hw/i386/microvm.h"
> > > >   #include "hw/pci/pci.h"
> > > > @@ -189,6 +190,14 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
> > > >           .reset_val = ACPI_GED_RESET_VALUE,
> > > >       };
> > > > +    /*
> > > > +     * second bit of 16 of the IAPC_BOOT_ARCH register indicates i8042 presence
> > > > +     * 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 = object_resolve_path_type("", TYPE_I8042, NULL) ?
> > > > +                            0x0002 : 0x0000;
> > > > +
> > let's avoid code duplication pls.
> > 
> What do you suggest to do with this? Creating a separate function to do this
> for us so we can call it from both locations?

Sounds reasonable.

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

On Wed, Mar 2, 2022 at 12:41 AM Liav Albani <liavalb@gmail.com> wrote:
>
>
> On 3/1/22 13:19, Michael S. Tsirkin wrote:
> > On Tue, Mar 01, 2022 at 09:43:54AM +0100, Igor Mammedov wrote:
> >> On Mon, 28 Feb 2022 22:17:32 +0200
> >> Liav Albani <liavalb@gmail.com> 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.
> >>>
> >>> This change only applies to the x86/q35 machine type, as it uses FACP
> >>> ACPI table with revision higher than 1, which should implement at least
> >>> ACPI 2.0 features within the table, hence it can also set the IA-PC boot
> >>> flags register according to the ACPI 2.0 specification.
> >>>
> >>> Signed-off-by: Liav Albani <liavalb@gmail.com>
> >>> ---
> >>>   hw/acpi/aml-build.c         | 11 ++++++++++-
> >>>   hw/i386/acpi-build.c        |  9 +++++++++
> >>>   hw/i386/acpi-microvm.c      |  9 +++++++++
> >> commit message says it's q35 specific, so wy it touched microvm anc piix4?
> >>
> This affect only q35 machine type for now, but what happens if the
> MicroVM ACPI FACP table is updated to use a revision that supports IA-PC
> boot flags?

microvm FACP table uses version 5. See function acpi_build_microvm().
It supports that flag already. What Igor was trying to say (and he can
correct me if I am wrong) is that lets address microvm when the need
arises, unless we already envision today that we would need IA-PC boot
flag update even for microvms in the near future. In other words, lets
not touch microvms if we do not have to, at present.

On Tue, 1 Mar 2022 06:19:51 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Mar 01, 2022 at 09:43:54AM +0100, Igor Mammedov wrote:
> > On Mon, 28 Feb 2022 22:17:32 +0200
> > Liav Albani <liavalb@gmail.com> 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.
> > > 
> > > This change only applies to the x86/q35 machine type, as it uses FACP
> > > ACPI table with revision higher than 1, which should implement at least
> > > ACPI 2.0 features within the table, hence it can also set the IA-PC boot
> > > flags register according to the ACPI 2.0 specification.
> > > 
> > > Signed-off-by: Liav Albani <liavalb@gmail.com>
> > > ---
> > >  hw/acpi/aml-build.c         | 11 ++++++++++-
> > >  hw/i386/acpi-build.c        |  9 +++++++++
> > >  hw/i386/acpi-microvm.c      |  9 +++++++++  
> > commit message says it's q35 specific, so wy it touched microvm anc piix4?
> >   
> > >  include/hw/acpi/acpi-defs.h |  1 +
> > >  4 files changed, 29 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > > index 8966e16320..2085905b83 100644
> > > --- a/hw/acpi/aml-build.c
> > > +++ b/hw/acpi/aml-build.c
> > > @@ -2152,7 +2152,16 @@ 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 */
> > > +    /*
> > > +     * This register is not defined in ACPI spec version 1.0, where the FACP
> > > +     * revision == 1 also applies. Therefore, just ignore setting this register.
> > > +     */
> > > +    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..c72c7bb9bb 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"
> > > @@ -192,6 +193,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 of the IAPC_BOOT_ARCH register indicates i8042 presence
> > > +     * or equivalent micro controller. See table 5-10 of APCI spec version 2.0
> > > +     * (the earliest acpi revision that supports this).  
> > 
> >  /* APCI spec version 2.0, Table 5-10 */
> > 
> > is sufficient, the rest could be read from spec/  
> 
> ACPI though, not APCI.
> The comment can be shorter and more clearer,

> but I feel quoting spec
> and including table name is a good idea actually, but pls quote verbatim:
I don't do that  and don't ask it from others.

The reason being that pointing where to look in spec and having
verbatim copy of field name is sufficient for looking it up and
QEMU does not endup with half of spec copied in (+unintentional mistakes).
(As reviewer I will check if whatever written in patch actually matches
spec anyways)

That's why I typically use
  'spec ver, verbatim field name[, chapter/table name]'
policy. The later optional part is usually used for pointing
to values description.
 
> /* ACPI spec version 2.0, Table 5-10: Fixed ACPI Description Table Boot Architecture Flags */
> /* Bit offset 1 -  port 60 and 64 based keyboard controller, usually implemented as an 8042 or equivalent micro-controller. */
> 
> >   
> > > +     */
> > > +    fadt.iapc_boot_arch = object_resolve_path_type("", TYPE_I8042, NULL) ?
> > > +                            0x0002 : 0x0000;  
> 
> and make it 0x1 << 1 - clearer that this is bit 1. Leading zeroes are
> not helpful since compiler does not check there's a correct number of
> these.
> 
> > > +
> > >      *data = fadt;
> > >  }
> > >  
> > > diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
> > > index 68ca7e7fc2..4bc72b1672 100644
> > > --- a/hw/i386/acpi-microvm.c
> > > +++ b/hw/i386/acpi-microvm.c
> > > @@ -31,6 +31,7 @@
> > >  #include "hw/acpi/generic_event_device.h"
> > >  #include "hw/acpi/utils.h"
> > >  #include "hw/acpi/erst.h"
> > > +#include "hw/input/i8042.h"
> > >  #include "hw/i386/fw_cfg.h"
> > >  #include "hw/i386/microvm.h"
> > >  #include "hw/pci/pci.h"
> > > @@ -189,6 +190,14 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
> > >          .reset_val = ACPI_GED_RESET_VALUE,
> > >      };
> > >  
> > > +    /*
> > > +     * second bit of 16 of the IAPC_BOOT_ARCH register indicates i8042 presence
> > > +     * 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 = object_resolve_path_type("", TYPE_I8042, NULL) ?
> > > +                            0x0002 : 0x0000;
> > > +  
> 
> let's avoid code duplication pls.
> 
> > >      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 */
> > >  
> > >      /*  
>

On Tue, Mar 01, 2022 at 12:47:57PM +0100, Igor Mammedov wrote:
> On Tue, 1 Mar 2022 06:19:51 -0500
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Mar 01, 2022 at 09:43:54AM +0100, Igor Mammedov wrote:
> > > On Mon, 28 Feb 2022 22:17:32 +0200
> > > Liav Albani <liavalb@gmail.com> 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.
> > > > 
> > > > This change only applies to the x86/q35 machine type, as it uses FACP
> > > > ACPI table with revision higher than 1, which should implement at least
> > > > ACPI 2.0 features within the table, hence it can also set the IA-PC boot
> > > > flags register according to the ACPI 2.0 specification.
> > > > 
> > > > Signed-off-by: Liav Albani <liavalb@gmail.com>
> > > > ---
> > > >  hw/acpi/aml-build.c         | 11 ++++++++++-
> > > >  hw/i386/acpi-build.c        |  9 +++++++++
> > > >  hw/i386/acpi-microvm.c      |  9 +++++++++  
> > > commit message says it's q35 specific, so wy it touched microvm anc piix4?
> > >   
> > > >  include/hw/acpi/acpi-defs.h |  1 +
> > > >  4 files changed, 29 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > > > index 8966e16320..2085905b83 100644
> > > > --- a/hw/acpi/aml-build.c
> > > > +++ b/hw/acpi/aml-build.c
> > > > @@ -2152,7 +2152,16 @@ 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 */
> > > > +    /*
> > > > +     * This register is not defined in ACPI spec version 1.0, where the FACP
> > > > +     * revision == 1 also applies. Therefore, just ignore setting this register.
> > > > +     */
> > > > +    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..c72c7bb9bb 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"
> > > > @@ -192,6 +193,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 of the IAPC_BOOT_ARCH register indicates i8042 presence
> > > > +     * or equivalent micro controller. See table 5-10 of APCI spec version 2.0
> > > > +     * (the earliest acpi revision that supports this).  
> > > 
> > >  /* APCI spec version 2.0, Table 5-10 */
> > > 
> > > is sufficient, the rest could be read from spec/  
> > 
> > ACPI though, not APCI.
> > The comment can be shorter and more clearer,
> 
> > but I feel quoting spec
> > and including table name is a good idea actually, but pls quote verbatim:
> I don't do that  and don't ask it from others.
> 
> The reason being that pointing where to look in spec and having
> verbatim copy of field name is sufficient
> for looking it up and
> QEMU does not endup with half of spec copied in (+unintentional mistakes).
> (As reviewer I will check if whatever written in patch actually matches
> spec anyways)
> 
> That's why I typically use
>   'spec ver, verbatim field name[, chapter/table name]'
> policy. The later optional part is usually used for pointing
> to values description.


Ok but here the field name was not listed verbatim, and table name
is missing. It is actually 8042 and table name is Fixed ACPI Description
Table Boot Architecture Flags.


>  
> > /* ACPI spec version 2.0, Table 5-10: Fixed ACPI Description Table Boot Architecture Flags */
> > /* Bit offset 1 -  port 60 and 64 based keyboard controller, usually implemented as an 8042 or equivalent micro-controller. */
> > 
> > >   
> > > > +     */
> > > > +    fadt.iapc_boot_arch = object_resolve_path_type("", TYPE_I8042, NULL) ?
> > > > +                            0x0002 : 0x0000;  
> > 
> > and make it 0x1 << 1 - clearer that this is bit 1. Leading zeroes are
> > not helpful since compiler does not check there's a correct number of
> > these.
> > 
> > > > +
> > > >      *data = fadt;
> > > >  }
> > > >  
> > > > diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
> > > > index 68ca7e7fc2..4bc72b1672 100644
> > > > --- a/hw/i386/acpi-microvm.c
> > > > +++ b/hw/i386/acpi-microvm.c
> > > > @@ -31,6 +31,7 @@
> > > >  #include "hw/acpi/generic_event_device.h"
> > > >  #include "hw/acpi/utils.h"
> > > >  #include "hw/acpi/erst.h"
> > > > +#include "hw/input/i8042.h"
> > > >  #include "hw/i386/fw_cfg.h"
> > > >  #include "hw/i386/microvm.h"
> > > >  #include "hw/pci/pci.h"
> > > > @@ -189,6 +190,14 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
> > > >          .reset_val = ACPI_GED_RESET_VALUE,
> > > >      };
> > > >  
> > > > +    /*
> > > > +     * second bit of 16 of the IAPC_BOOT_ARCH register indicates i8042 presence
> > > > +     * 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 = object_resolve_path_type("", TYPE_I8042, NULL) ?
> > > > +                            0x0002 : 0x0000;
> > > > +  
> > 
> > let's avoid code duplication pls.
> > 
> > > >      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 */
> > > >  
> > > >      /*  
> >

>>> but I feel quoting spec
>>> and including table name is a good idea actually, but pls quote verbatim:
>> I don't do that  and don't ask it from others.
>>
>> The reason being that pointing where to look in spec and having
>> verbatim copy of field name is sufficient
>> for looking it up and
>> QEMU does not endup with half of spec copied in (+unintentional mistakes).
>> (As reviewer I will check if whatever written in patch actually matches
>> spec anyways)
>>
>> That's why I typically use
>>    'spec ver, verbatim field name[, chapter/table name]'
>> policy. The later optional part is usually used for pointing
>> to values description.
>
> Ok but here the field name was not listed verbatim, and table name
> is missing. It is actually 8042 and table name is Fixed ACPI Description
> Table Boot Architecture Flags.
So, in which route should I go with this? I could add a reference to the 
ACPI spec, but can write and explain more if you want me to.

On Wed, 2 Mar 2022 17:45:58 +0200
Liav Albani <liavalb@gmail.com> wrote:

> >>> but I feel quoting spec
> >>> and including table name is a good idea actually, but pls quote verbatim:  
> >> I don't do that  and don't ask it from others.
> >>
> >> The reason being that pointing where to look in spec and having
> >> verbatim copy of field name is sufficient
> >> for looking it up and
> >> QEMU does not endup with half of spec copied in (+unintentional mistakes).
> >> (As reviewer I will check if whatever written in patch actually matches
> >> spec anyways)
> >>
> >> That's why I typically use
> >>    'spec ver, verbatim field name[, chapter/table name]'
> >> policy. The later optional part is usually used for pointing
> >> to values description.  
> >
> > Ok but here the field name was not listed verbatim, and table name
> > is missing. It is actually 8042 and table name is Fixed ACPI Description
> > Table Boot Architecture Flags.  
> So, in which route should I go with this? I could add a reference to the 
> ACPI spec, but can write and explain more if you want me to.
A reference to spec is sufficient, as long as it is unambiguous and lets
a reviewer easily find it within the spec