|
From: | Liav Albani |
Subject: | Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table |
Date: | Tue, 1 Mar 2022 21:11:09 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.6.1 |
On 3/1/22 13:19, Michael S. Tsirkin wrote:
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?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.cindex 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.
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?+ *data = fadt; }diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.cindex 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 *//*
[Prev in Thread] | Current Thread | [Next in Thread] |