* [PATCH v4 0/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
@ 2022-02-28 20:17 Liav Albani
2022-02-28 20:17 ` [PATCH v4 1/3] tests/acpi: i386: allow FACP acpi table changes Liav Albani
` (3 more replies)
0 siblings, 4 replies; 32+ messages in thread
From: Liav Albani @ 2022-02-28 20:17 UTC (permalink / raw)
To: qemu-devel; +Cc: ani, imammedo, shentey, Liav Albani, mst
This can allow the guest OS to determine more easily if i8042 controller
is present in the system or not, so it doesn't need to do probing of the
controller, but just initialize it immediately, before enumerating the
ACPI AML namespace.
To allow "flexible" indication, I don't hardcode the bit at location 1
as on in the IA-PC boot flags, but try to search for i8042 on the ISA
bus to verify it exists in the system.
Why this is useful you might ask - this patch allows the guest OS to
probe and use the i8042 controller without decoding the ACPI AML blob
at all. For example, as a developer of the SerenityOS kernel, I might
want to allow people to not try to decode the ACPI AML namespace (for
now, we still don't support ACPI AML as it's a work in progress), but
still to not probe for the i8042 but just use it after looking in the
IA-PC boot flags in the ACPI FADT table.
Liav Albani (3):
tests/acpi: i386: allow FACP acpi table changes
hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT
table
tests/acpi: i386: update FACP table differences
hw/acpi/aml-build.c | 11 ++++++++++-
hw/i386/acpi-build.c | 9 +++++++++
hw/i386/acpi-microvm.c | 9 +++++++++
include/hw/acpi/acpi-defs.h | 1 +
tests/data/acpi/q35/FACP | Bin 244 -> 244 bytes
tests/data/acpi/q35/FACP.nosmm | Bin 244 -> 244 bytes
tests/data/acpi/q35/FACP.slic | Bin 244 -> 244 bytes
tests/data/acpi/q35/FACP.xapic | Bin 244 -> 244 bytes
8 files changed, 29 insertions(+), 1 deletion(-)
--
2.35.1
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v4 1/3] tests/acpi: i386: allow FACP acpi table changes
2022-02-28 20:17 [PATCH v4 0/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table Liav Albani
@ 2022-02-28 20:17 ` Liav Albani
2022-03-01 2:55 ` Ani Sinha
2022-02-28 20:17 ` [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table Liav Albani
` (2 subsequent siblings)
3 siblings, 1 reply; 32+ messages in thread
From: Liav Albani @ 2022-02-28 20:17 UTC (permalink / raw)
To: qemu-devel; +Cc: ani, imammedo, shentey, Liav Albani, mst
The FACP table is going to be changed for x86/q35 machines. To be sure
the following changes are not breaking any QEMU test this change follows
step 2 from the bios-tables-test.c guide on changes that affect ACPI
tables.
Signed-off-by: Liav Albani <liavalb@gmail.com>
---
tests/qtest/bios-tables-test-allowed-diff.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..7570e39369 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,5 @@
/* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/q35/FACP",
+"tests/data/acpi/q35/FACP.nosmm",
+"tests/data/acpi/q35/FACP.slic",
+"tests/data/acpi/q35/FACP.xapic",
--
2.35.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-02-28 20:17 [PATCH v4 0/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table Liav Albani
2022-02-28 20:17 ` [PATCH v4 1/3] tests/acpi: i386: allow FACP acpi table changes Liav Albani
@ 2022-02-28 20:17 ` Liav Albani
2022-03-01 2:59 ` Ani Sinha
2022-03-01 8:43 ` Igor Mammedov
2022-02-28 20:17 ` [PATCH v4 3/3] tests/acpi: i386: update FACP table differences Liav Albani
2022-03-04 10:26 ` [PATCH v4 0/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table Michael S. Tsirkin
3 siblings, 2 replies; 32+ messages in thread
From: Liav Albani @ 2022-02-28 20:17 UTC (permalink / raw)
To: qemu-devel; +Cc: ani, imammedo, shentey, Liav Albani, mst
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
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH v4 3/3] tests/acpi: i386: update FACP table differences
2022-02-28 20:17 [PATCH v4 0/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table Liav Albani
2022-02-28 20:17 ` [PATCH v4 1/3] tests/acpi: i386: allow FACP acpi table changes Liav Albani
2022-02-28 20:17 ` [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table Liav Albani
@ 2022-02-28 20:17 ` Liav Albani
2022-03-01 2:59 ` Ani Sinha
2022-03-04 10:26 ` [PATCH v4 0/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table Michael S. Tsirkin
3 siblings, 1 reply; 32+ messages in thread
From: Liav Albani @ 2022-02-28 20:17 UTC (permalink / raw)
To: qemu-devel; +Cc: ani, imammedo, shentey, Liav Albani, mst
After changing the IAPC boot flags register to indicate support of i8042
in the machine chipset to help the guest OS to determine its existence
"faster", we need to have the updated FACP ACPI binary images in tree.
@@ -1,32 +1,32 @@
/*
* Intel ACPI Component Architecture
* AML/ASL+ Disassembler version 20211217 (64-bit version)
* Copyright (c) 2000 - 2021 Intel Corporation
*
- * Disassembly of tests/data/acpi/q35/FACP, Wed Feb 23 22:37:39 2022
+ * Disassembly of /tmp/aml-BBFBI1, Wed Feb 23 22:37:39 2022
*
* ACPI Data Table [FACP]
*
* Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue (in hex)
*/
[000h 0000 4] Signature : "FACP" [Fixed ACPI Description Table (FADT)]
[004h 0004 4] Table Length : 000000F4
[008h 0008 1] Revision : 03
-[009h 0009 1] Checksum : B9
+[009h 0009 1] Checksum : B7
[00Ah 0010 6] Oem ID : "BOCHS "
[010h 0016 8] Oem Table ID : "BXPC "
[018h 0024 4] Oem Revision : 00000001
[01Ch 0028 4] Asl Compiler ID : "BXPC"
[020h 0032 4] Asl Compiler Revision : 00000001
[024h 0036 4] FACS Address : 00000000
[028h 0040 4] DSDT Address : 00000000
[02Ch 0044 1] Model : 01
[02Dh 0045 1] PM Profile : 00 [Unspecified]
[02Eh 0046 2] SCI Interrupt : 0009
[030h 0048 4] SMI Command Port : 000000B2
[034h 0052 1] ACPI Enable Value : 02
[035h 0053 1] ACPI Disable Value : 03
[036h 0054 1] S4BIOS Command : 00
[037h 0055 1] P-State Control : 00
@@ -42,35 +42,35 @@
[059h 0089 1] PM1 Control Block Length : 02
[05Ah 0090 1] PM2 Control Block Length : 00
[05Bh 0091 1] PM Timer Block Length : 04
[05Ch 0092 1] GPE0 Block Length : 10
[05Dh 0093 1] GPE1 Block Length : 00
[05Eh 0094 1] GPE1 Base Offset : 00
[05Fh 0095 1] _CST Support : 00
[060h 0096 2] C2 Latency : 0FFF
[062h 0098 2] C3 Latency : 0FFF
[064h 0100 2] CPU Cache Size : 0000
[066h 0102 2] Cache Flush Stride : 0000
[068h 0104 1] Duty Cycle Offset : 00
[069h 0105 1] Duty Cycle Width : 00
[06Ah 0106 1] RTC Day Alarm Index : 00
[06Bh 0107 1] RTC Month Alarm Index : 00
[06Ch 0108 1] RTC Century Index : 32
-[06Dh 0109 2] Boot Flags (decoded below) : 0000
+[06Dh 0109 2] Boot Flags (decoded below) : 0002
Legacy Devices Supported (V2) : 0
- 8042 Present on ports 60/64 (V2) : 0
+ 8042 Present on ports 60/64 (V2) : 1
VGA Not Present (V4) : 0
MSI Not Supported (V4) : 0
PCIe ASPM Not Supported (V4) : 0
CMOS RTC Not Present (V5) : 0
[06Fh 0111 1] Reserved : 00
[070h 0112 4] Flags (decoded below) : 000084A5
WBINVD instruction is operational (V1) : 1
WBINVD flushes all caches (V1) : 0
All CPUs support C1 (V1) : 1
C2 works on MP system (V1) : 0
Control Method Power Button (V1) : 0
Control Method Sleep Button (V1) : 1
RTC wake not in fixed reg space (V1) : 0
RTC can wake system from S4 (V1) : 1
32-bit PM Timer (V1) : 0
Docking Supported (V1) : 0
@@ -148,32 +148,32 @@
[0DCh 0220 1] Space ID : 01 [SystemIO]
[0DDh 0221 1] Bit Width : 80
[0DEh 0222 1] Bit Offset : 00
[0DFh 0223 1] Encoded Access Width : 00 [Undefined/Legacy]
[0E0h 0224 8] Address : 0000000000000620
[0E8h 0232 12] GPE1 Block : [Generic Address Structure]
[0E8h 0232 1] Space ID : 00 [SystemMemory]
[0E9h 0233 1] Bit Width : 00
[0EAh 0234 1] Bit Offset : 00
[0EBh 0235 1] Encoded Access Width : 00 [Undefined/Legacy]
[0ECh 0236 8] Address : 0000000000000000
Raw Table Data: Length 244 (0xF4)
- 0000: 46 41 43 50 F4 00 00 00 03 B9 42 4F 43 48 53 20 // FACP......BOCHS
+ 0000: 46 41 43 50 F4 00 00 00 03 B7 42 4F 43 48 53 20 // FACP......BOCHS
0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43 // BXPC ....BXPC
0020: 01 00 00 00 00 00 00 00 00 00 00 00 01 00 09 00 // ................
0030: B2 00 00 00 02 03 00 00 00 06 00 00 00 00 00 00 // ................
0040: 04 06 00 00 00 00 00 00 00 00 00 00 08 06 00 00 // ................
0050: 20 06 00 00 00 00 00 00 04 02 00 04 10 00 00 00 // ...............
- 0060: FF 0F FF 0F 00 00 00 00 00 00 00 00 32 00 00 00 // ............2...
+ 0060: FF 0F FF 0F 00 00 00 00 00 00 00 00 32 02 00 00 // ............2...
0070: A5 84 00 00 01 08 00 00 F9 0C 00 00 00 00 00 00 // ................
0080: 0F 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
0090: 00 00 00 00 01 20 00 00 00 06 00 00 00 00 00 00 // ..... ..........
00A0: 00 00 00 00 00 00 00 00 00 00 00 00 01 10 00 00 // ................
00B0: 04 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
00C0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
00D0: 01 20 00 00 08 06 00 00 00 00 00 00 01 80 00 00 // . ..............
00E0: 20 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ...............
00F0: 00 00 00 00 // ....
**
Signed-off-by: Liav Albani <liavalb@gmail.com>
---
tests/data/acpi/q35/FACP | Bin 244 -> 244 bytes
tests/data/acpi/q35/FACP.nosmm | Bin 244 -> 244 bytes
tests/data/acpi/q35/FACP.slic | Bin 244 -> 244 bytes
tests/data/acpi/q35/FACP.xapic | Bin 244 -> 244 bytes
tests/qtest/bios-tables-test-allowed-diff.h | 4 ----
5 files changed, 4 deletions(-)
diff --git a/tests/data/acpi/q35/FACP b/tests/data/acpi/q35/FACP
index f6a864cc863c7763f6c09d3814ad184a658fa0a0..a8f6a8961109d01059aceef9f1869cde09a2f10c 100644
GIT binary patch
delta 23
ecmeyu_=S<n&CxmF3j+fK^Y)2c$&5@B^V$GgGY3Ne
delta 23
ecmeyu_=S<n&CxmF3j+fK^UjG}$&3sW^V$GgJqJSo
diff --git a/tests/data/acpi/q35/FACP.nosmm b/tests/data/acpi/q35/FACP.nosmm
index 6a9aa5f370eb9af6a03dc739d8a159be58fdee01..c4e6d18ee5fc64159160d4589aa96b4d648c913a 100644
GIT binary patch
delta 23
ecmeyu_=S<n&CxmF3j+fKbKXR*WJacmd2Ik#q6Yc^
delta 23
ecmeyu_=S<n&CxmF3j+fKbHPNeWJZRGd2Ik#tOoi3
diff --git a/tests/data/acpi/q35/FACP.slic b/tests/data/acpi/q35/FACP.slic
index 15986e095cf2db7ee92f7ce113c1d46d54018c62..48bbb1cf5ad0ceda1d2f6d56edf5c1e207bd1a04 100644
GIT binary patch
delta 23
ecmeyu_=S<n&CxmF3j+fK^M#3A$&5@B^V$Gh6bD=Y
delta 23
ecmeyu_=S<n&CxmF3j+fK^QDPg$&3sW^V$Gh9tT_i
diff --git a/tests/data/acpi/q35/FACP.xapic b/tests/data/acpi/q35/FACP.xapic
index 2d3659c9c6753d07c3d48742343cb8e8cc034de7..31fa5dd19c213034eef4eeefa6a04e61dadd8a2a 100644
GIT binary patch
delta 23
ecmeyu_=S<n&CxmF3j+fK^X7?M$&5@B^V$Gg4+lR0
delta 23
ecmeyu_=S<n&CxmF3j+fK^VW%6$&3sW^V$Gg83#WA
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index 7570e39369..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,5 +1 @@
/* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/q35/FACP",
-"tests/data/acpi/q35/FACP.nosmm",
-"tests/data/acpi/q35/FACP.slic",
-"tests/data/acpi/q35/FACP.xapic",
--
2.35.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH v4 1/3] tests/acpi: i386: allow FACP acpi table changes
2022-02-28 20:17 ` [PATCH v4 1/3] tests/acpi: i386: allow FACP acpi table changes Liav Albani
@ 2022-03-01 2:55 ` Ani Sinha
0 siblings, 0 replies; 32+ messages in thread
From: Ani Sinha @ 2022-03-01 2:55 UTC (permalink / raw)
To: Liav Albani; +Cc: ani, imammedo, shentey, qemu-devel, mst
On Mon, 28 Feb 2022, Liav Albani wrote:
> The FACP table is going to be changed for x86/q35 machines. To be sure
> the following changes are not breaking any QEMU test this change follows
> step 2 from the bios-tables-test.c guide on changes that affect ACPI
> tables.
Please retain the tags in subsequent versions of the patch.
>
> Signed-off-by: Liav Albani <liavalb@gmail.com>
Acked-by: Ani Sinha <ani@anisinha.ca>
> ---
> tests/qtest/bios-tables-test-allowed-diff.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index dfb8523c8b..7570e39369 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1 +1,5 @@
> /* List of comma-separated changed AML files to ignore */
> +"tests/data/acpi/q35/FACP",
> +"tests/data/acpi/q35/FACP.nosmm",
> +"tests/data/acpi/q35/FACP.slic",
> +"tests/data/acpi/q35/FACP.xapic",
> --
> 2.35.1
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-02-28 20:17 ` [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table Liav Albani
@ 2022-03-01 2:59 ` Ani Sinha
2022-03-01 8:48 ` Igor Mammedov
2022-03-01 8:43 ` Igor Mammedov
1 sibling, 1 reply; 32+ messages in thread
From: Ani Sinha @ 2022-03-01 2:59 UTC (permalink / raw)
To: Liav Albani; +Cc: ani, imammedo, shentey, qemu-devel, mst
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
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 3/3] tests/acpi: i386: update FACP table differences
2022-02-28 20:17 ` [PATCH v4 3/3] tests/acpi: i386: update FACP table differences Liav Albani
@ 2022-03-01 2:59 ` Ani Sinha
2022-03-01 11:21 ` Michael S. Tsirkin
0 siblings, 1 reply; 32+ messages in thread
From: Ani Sinha @ 2022-03-01 2:59 UTC (permalink / raw)
To: Liav Albani; +Cc: ani, imammedo, shentey, qemu-devel, mst
On Mon, 28 Feb 2022, Liav Albani wrote:
> After changing the IAPC boot flags register to indicate support of i8042
> in the machine chipset to help the guest OS to determine its existence
> "faster", we need to have the updated FACP ACPI binary images in tree.
>
> @@ -1,32 +1,32 @@
> /*
> * Intel ACPI Component Architecture
> * AML/ASL+ Disassembler version 20211217 (64-bit version)
> * Copyright (c) 2000 - 2021 Intel Corporation
> *
> - * Disassembly of tests/data/acpi/q35/FACP, Wed Feb 23 22:37:39 2022
> + * Disassembly of /tmp/aml-BBFBI1, Wed Feb 23 22:37:39 2022
> *
> * ACPI Data Table [FACP]
> *
> * Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue (in hex)
> */
>
> [000h 0000 4] Signature : "FACP" [Fixed ACPI Description Table (FADT)]
> [004h 0004 4] Table Length : 000000F4
> [008h 0008 1] Revision : 03
> -[009h 0009 1] Checksum : B9
> +[009h 0009 1] Checksum : B7
> [00Ah 0010 6] Oem ID : "BOCHS "
> [010h 0016 8] Oem Table ID : "BXPC "
> [018h 0024 4] Oem Revision : 00000001
> [01Ch 0028 4] Asl Compiler ID : "BXPC"
> [020h 0032 4] Asl Compiler Revision : 00000001
>
> [024h 0036 4] FACS Address : 00000000
> [028h 0040 4] DSDT Address : 00000000
> [02Ch 0044 1] Model : 01
> [02Dh 0045 1] PM Profile : 00 [Unspecified]
> [02Eh 0046 2] SCI Interrupt : 0009
> [030h 0048 4] SMI Command Port : 000000B2
> [034h 0052 1] ACPI Enable Value : 02
> [035h 0053 1] ACPI Disable Value : 03
> [036h 0054 1] S4BIOS Command : 00
> [037h 0055 1] P-State Control : 00
> @@ -42,35 +42,35 @@
> [059h 0089 1] PM1 Control Block Length : 02
> [05Ah 0090 1] PM2 Control Block Length : 00
> [05Bh 0091 1] PM Timer Block Length : 04
> [05Ch 0092 1] GPE0 Block Length : 10
> [05Dh 0093 1] GPE1 Block Length : 00
> [05Eh 0094 1] GPE1 Base Offset : 00
> [05Fh 0095 1] _CST Support : 00
> [060h 0096 2] C2 Latency : 0FFF
> [062h 0098 2] C3 Latency : 0FFF
> [064h 0100 2] CPU Cache Size : 0000
> [066h 0102 2] Cache Flush Stride : 0000
> [068h 0104 1] Duty Cycle Offset : 00
> [069h 0105 1] Duty Cycle Width : 00
> [06Ah 0106 1] RTC Day Alarm Index : 00
> [06Bh 0107 1] RTC Month Alarm Index : 00
> [06Ch 0108 1] RTC Century Index : 32
> -[06Dh 0109 2] Boot Flags (decoded below) : 0000
> +[06Dh 0109 2] Boot Flags (decoded below) : 0002
> Legacy Devices Supported (V2) : 0
> - 8042 Present on ports 60/64 (V2) : 0
> + 8042 Present on ports 60/64 (V2) : 1
> VGA Not Present (V4) : 0
> MSI Not Supported (V4) : 0
> PCIe ASPM Not Supported (V4) : 0
> CMOS RTC Not Present (V5) : 0
> [06Fh 0111 1] Reserved : 00
> [070h 0112 4] Flags (decoded below) : 000084A5
> WBINVD instruction is operational (V1) : 1
> WBINVD flushes all caches (V1) : 0
> All CPUs support C1 (V1) : 1
> C2 works on MP system (V1) : 0
> Control Method Power Button (V1) : 0
> Control Method Sleep Button (V1) : 1
> RTC wake not in fixed reg space (V1) : 0
> RTC can wake system from S4 (V1) : 1
> 32-bit PM Timer (V1) : 0
> Docking Supported (V1) : 0
> @@ -148,32 +148,32 @@
> [0DCh 0220 1] Space ID : 01 [SystemIO]
> [0DDh 0221 1] Bit Width : 80
> [0DEh 0222 1] Bit Offset : 00
> [0DFh 0223 1] Encoded Access Width : 00 [Undefined/Legacy]
> [0E0h 0224 8] Address : 0000000000000620
>
> [0E8h 0232 12] GPE1 Block : [Generic Address Structure]
> [0E8h 0232 1] Space ID : 00 [SystemMemory]
> [0E9h 0233 1] Bit Width : 00
> [0EAh 0234 1] Bit Offset : 00
> [0EBh 0235 1] Encoded Access Width : 00 [Undefined/Legacy]
> [0ECh 0236 8] Address : 0000000000000000
>
> Raw Table Data: Length 244 (0xF4)
>
> - 0000: 46 41 43 50 F4 00 00 00 03 B9 42 4F 43 48 53 20 // FACP......BOCHS
> + 0000: 46 41 43 50 F4 00 00 00 03 B7 42 4F 43 48 53 20 // FACP......BOCHS
> 0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43 // BXPC ....BXPC
> 0020: 01 00 00 00 00 00 00 00 00 00 00 00 01 00 09 00 // ................
> 0030: B2 00 00 00 02 03 00 00 00 06 00 00 00 00 00 00 // ................
> 0040: 04 06 00 00 00 00 00 00 00 00 00 00 08 06 00 00 // ................
> 0050: 20 06 00 00 00 00 00 00 04 02 00 04 10 00 00 00 // ...............
> - 0060: FF 0F FF 0F 00 00 00 00 00 00 00 00 32 00 00 00 // ............2...
> + 0060: FF 0F FF 0F 00 00 00 00 00 00 00 00 32 02 00 00 // ............2...
> 0070: A5 84 00 00 01 08 00 00 F9 0C 00 00 00 00 00 00 // ................
> 0080: 0F 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> 0090: 00 00 00 00 01 20 00 00 00 06 00 00 00 00 00 00 // ..... ..........
> 00A0: 00 00 00 00 00 00 00 00 00 00 00 00 01 10 00 00 // ................
> 00B0: 04 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> 00C0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> 00D0: 01 20 00 00 08 06 00 00 00 00 00 00 01 80 00 00 // . ..............
> 00E0: 20 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ...............
> 00F0: 00 00 00 00 // ....
> **
Please retain tags from earlier revisions of the patch.
>
> Signed-off-by: Liav Albani <liavalb@gmail.com>
Acked-by: Ani Sinha <ani@anisinha.ca>
> ---
> tests/data/acpi/q35/FACP | Bin 244 -> 244 bytes
> tests/data/acpi/q35/FACP.nosmm | Bin 244 -> 244 bytes
> tests/data/acpi/q35/FACP.slic | Bin 244 -> 244 bytes
> tests/data/acpi/q35/FACP.xapic | Bin 244 -> 244 bytes
> tests/qtest/bios-tables-test-allowed-diff.h | 4 ----
> 5 files changed, 4 deletions(-)
>
> diff --git a/tests/data/acpi/q35/FACP b/tests/data/acpi/q35/FACP
> index f6a864cc863c7763f6c09d3814ad184a658fa0a0..a8f6a8961109d01059aceef9f1869cde09a2f10c 100644
> GIT binary patch
> delta 23
> ecmeyu_=S<n&CxmF3j+fK^Y)2c$&5@B^V$GgGY3Ne
>
> delta 23
> ecmeyu_=S<n&CxmF3j+fK^UjG}$&3sW^V$GgJqJSo
>
> diff --git a/tests/data/acpi/q35/FACP.nosmm b/tests/data/acpi/q35/FACP.nosmm
> index 6a9aa5f370eb9af6a03dc739d8a159be58fdee01..c4e6d18ee5fc64159160d4589aa96b4d648c913a 100644
> GIT binary patch
> delta 23
> ecmeyu_=S<n&CxmF3j+fKbKXR*WJacmd2Ik#q6Yc^
>
> delta 23
> ecmeyu_=S<n&CxmF3j+fKbHPNeWJZRGd2Ik#tOoi3
>
> diff --git a/tests/data/acpi/q35/FACP.slic b/tests/data/acpi/q35/FACP.slic
> index 15986e095cf2db7ee92f7ce113c1d46d54018c62..48bbb1cf5ad0ceda1d2f6d56edf5c1e207bd1a04 100644
> GIT binary patch
> delta 23
> ecmeyu_=S<n&CxmF3j+fK^M#3A$&5@B^V$Gh6bD=Y
>
> delta 23
> ecmeyu_=S<n&CxmF3j+fK^QDPg$&3sW^V$Gh9tT_i
>
> diff --git a/tests/data/acpi/q35/FACP.xapic b/tests/data/acpi/q35/FACP.xapic
> index 2d3659c9c6753d07c3d48742343cb8e8cc034de7..31fa5dd19c213034eef4eeefa6a04e61dadd8a2a 100644
> GIT binary patch
> delta 23
> ecmeyu_=S<n&CxmF3j+fK^X7?M$&5@B^V$Gg4+lR0
>
> delta 23
> ecmeyu_=S<n&CxmF3j+fK^VW%6$&3sW^V$Gg83#WA
>
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index 7570e39369..dfb8523c8b 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1,5 +1 @@
> /* List of comma-separated changed AML files to ignore */
> -"tests/data/acpi/q35/FACP",
> -"tests/data/acpi/q35/FACP.nosmm",
> -"tests/data/acpi/q35/FACP.slic",
> -"tests/data/acpi/q35/FACP.xapic",
> --
> 2.35.1
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-02-28 20:17 ` [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table Liav Albani
2022-03-01 2:59 ` Ani Sinha
@ 2022-03-01 8:43 ` Igor Mammedov
2022-03-01 9:52 ` Ani Sinha
2022-03-01 11:19 ` Michael S. Tsirkin
1 sibling, 2 replies; 32+ messages in thread
From: Igor Mammedov @ 2022-03-01 8:43 UTC (permalink / raw)
To: Liav Albani; +Cc: ani, shentey, qemu-devel, mst
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 */
>
> /*
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-03-01 2:59 ` Ani Sinha
@ 2022-03-01 8:48 ` Igor Mammedov
0 siblings, 0 replies; 32+ messages in thread
From: Igor Mammedov @ 2022-03-01 8:48 UTC (permalink / raw)
To: Ani Sinha; +Cc: mst, shentey, Liav Albani, qemu-devel
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
> >
> >
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-03-01 8:43 ` Igor Mammedov
@ 2022-03-01 9:52 ` Ani Sinha
2022-03-01 11:51 ` Igor Mammedov
2022-03-01 19:20 ` Liav Albani
2022-03-01 11:19 ` Michael S. Tsirkin
1 sibling, 2 replies; 32+ messages in thread
From: Ani Sinha @ 2022-03-01 9:52 UTC (permalink / raw)
To: Igor Mammedov; +Cc: ani, mst, shentey, Liav Albani, qemu-devel
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>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-03-01 8:43 ` Igor Mammedov
2022-03-01 9:52 ` Ani Sinha
@ 2022-03-01 11:19 ` Michael S. Tsirkin
2022-03-01 11:47 ` Igor Mammedov
2022-03-01 19:11 ` Liav Albani
1 sibling, 2 replies; 32+ messages in thread
From: Michael S. Tsirkin @ 2022-03-01 11:19 UTC (permalink / raw)
To: Igor Mammedov; +Cc: ani, shentey, Liav Albani, qemu-devel
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 */
> >
> > /*
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 3/3] tests/acpi: i386: update FACP table differences
2022-03-01 2:59 ` Ani Sinha
@ 2022-03-01 11:21 ` Michael S. Tsirkin
2022-03-01 19:13 ` Liav Albani
0 siblings, 1 reply; 32+ messages in thread
From: Michael S. Tsirkin @ 2022-03-01 11:21 UTC (permalink / raw)
To: Ani Sinha; +Cc: imammedo, shentey, Liav Albani, qemu-devel
On Tue, Mar 01, 2022 at 08:29:57AM +0530, Ani Sinha wrote:
>
>
> On Mon, 28 Feb 2022, Liav Albani wrote:
>
> > After changing the IAPC boot flags register to indicate support of i8042
> > in the machine chipset to help the guest OS to determine its existence
> > "faster", we need to have the updated FACP ACPI binary images in tree.
> >
> > @@ -1,32 +1,32 @@
> > /*
> > * Intel ACPI Component Architecture
> > * AML/ASL+ Disassembler version 20211217 (64-bit version)
> > * Copyright (c) 2000 - 2021 Intel Corporation
> > *
> > - * Disassembly of tests/data/acpi/q35/FACP, Wed Feb 23 22:37:39 2022
> > + * Disassembly of /tmp/aml-BBFBI1, Wed Feb 23 22:37:39 2022
cut this out pls
> > *
> > * ACPI Data Table [FACP]
> > *
> > * Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue (in hex)
> > */
> >
> > [000h 0000 4] Signature : "FACP" [Fixed ACPI Description Table (FADT)]
> > [004h 0004 4] Table Length : 000000F4
> > [008h 0008 1] Revision : 03
> > -[009h 0009 1] Checksum : B9
> > +[009h 0009 1] Checksum : B7
and this
> > [00Ah 0010 6] Oem ID : "BOCHS "
> > [010h 0016 8] Oem Table ID : "BXPC "
> > [018h 0024 4] Oem Revision : 00000001
> > [01Ch 0028 4] Asl Compiler ID : "BXPC"
> > [020h 0032 4] Asl Compiler Revision : 00000001
> >
> > [024h 0036 4] FACS Address : 00000000
> > [028h 0040 4] DSDT Address : 00000000
> > [02Ch 0044 1] Model : 01
> > [02Dh 0045 1] PM Profile : 00 [Unspecified]
> > [02Eh 0046 2] SCI Interrupt : 0009
> > [030h 0048 4] SMI Command Port : 000000B2
> > [034h 0052 1] ACPI Enable Value : 02
> > [035h 0053 1] ACPI Disable Value : 03
> > [036h 0054 1] S4BIOS Command : 00
> > [037h 0055 1] P-State Control : 00
> > @@ -42,35 +42,35 @@
> > [059h 0089 1] PM1 Control Block Length : 02
> > [05Ah 0090 1] PM2 Control Block Length : 00
> > [05Bh 0091 1] PM Timer Block Length : 04
> > [05Ch 0092 1] GPE0 Block Length : 10
> > [05Dh 0093 1] GPE1 Block Length : 00
> > [05Eh 0094 1] GPE1 Base Offset : 00
> > [05Fh 0095 1] _CST Support : 00
> > [060h 0096 2] C2 Latency : 0FFF
> > [062h 0098 2] C3 Latency : 0FFF
> > [064h 0100 2] CPU Cache Size : 0000
> > [066h 0102 2] Cache Flush Stride : 0000
> > [068h 0104 1] Duty Cycle Offset : 00
> > [069h 0105 1] Duty Cycle Width : 00
> > [06Ah 0106 1] RTC Day Alarm Index : 00
> > [06Bh 0107 1] RTC Month Alarm Index : 00
> > [06Ch 0108 1] RTC Century Index : 32
> > -[06Dh 0109 2] Boot Flags (decoded below) : 0000
> > +[06Dh 0109 2] Boot Flags (decoded below) : 0002
> > Legacy Devices Supported (V2) : 0
> > - 8042 Present on ports 60/64 (V2) : 0
> > + 8042 Present on ports 60/64 (V2) : 1
> > VGA Not Present (V4) : 0
> > MSI Not Supported (V4) : 0
> > PCIe ASPM Not Supported (V4) : 0
> > CMOS RTC Not Present (V5) : 0
leaving just this
> > [06Fh 0111 1] Reserved : 00
> > [070h 0112 4] Flags (decoded below) : 000084A5
> > WBINVD instruction is operational (V1) : 1
> > WBINVD flushes all caches (V1) : 0
> > All CPUs support C1 (V1) : 1
> > C2 works on MP system (V1) : 0
> > Control Method Power Button (V1) : 0
> > Control Method Sleep Button (V1) : 1
> > RTC wake not in fixed reg space (V1) : 0
> > RTC can wake system from S4 (V1) : 1
> > 32-bit PM Timer (V1) : 0
> > Docking Supported (V1) : 0
> > @@ -148,32 +148,32 @@
> > [0DCh 0220 1] Space ID : 01 [SystemIO]
> > [0DDh 0221 1] Bit Width : 80
> > [0DEh 0222 1] Bit Offset : 00
> > [0DFh 0223 1] Encoded Access Width : 00 [Undefined/Legacy]
> > [0E0h 0224 8] Address : 0000000000000620
> >
> > [0E8h 0232 12] GPE1 Block : [Generic Address Structure]
> > [0E8h 0232 1] Space ID : 00 [SystemMemory]
> > [0E9h 0233 1] Bit Width : 00
> > [0EAh 0234 1] Bit Offset : 00
> > [0EBh 0235 1] Encoded Access Width : 00 [Undefined/Legacy]
> > [0ECh 0236 8] Address : 0000000000000000
> >
> > Raw Table Data: Length 244 (0xF4)
> >
> > - 0000: 46 41 43 50 F4 00 00 00 03 B9 42 4F 43 48 53 20 // FACP......BOCHS
> > + 0000: 46 41 43 50 F4 00 00 00 03 B7 42 4F 43 48 53 20 // FACP......BOCHS
> > 0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43 // BXPC ....BXPC
> > 0020: 01 00 00 00 00 00 00 00 00 00 00 00 01 00 09 00 // ................
> > 0030: B2 00 00 00 02 03 00 00 00 06 00 00 00 00 00 00 // ................
> > 0040: 04 06 00 00 00 00 00 00 00 00 00 00 08 06 00 00 // ................
> > 0050: 20 06 00 00 00 00 00 00 04 02 00 04 10 00 00 00 // ...............
> > - 0060: FF 0F FF 0F 00 00 00 00 00 00 00 00 32 00 00 00 // ............2...
> > + 0060: FF 0F FF 0F 00 00 00 00 00 00 00 00 32 02 00 00 // ............2...
> > 0070: A5 84 00 00 01 08 00 00 F9 0C 00 00 00 00 00 00 // ................
> > 0080: 0F 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> > 0090: 00 00 00 00 01 20 00 00 00 06 00 00 00 00 00 00 // ..... ..........
> > 00A0: 00 00 00 00 00 00 00 00 00 00 00 00 01 10 00 00 // ................
> > 00B0: 04 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> > 00C0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> > 00D0: 01 20 00 00 08 06 00 00 00 00 00 00 01 80 00 00 // . ..............
> > 00E0: 20 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ...............
> > 00F0: 00 00 00 00 // ....
this isn't helpful either and will be wrong if cherry picked.
> > **
>
> Please retain tags from earlier revisions of the patch.
>
> >
> > Signed-off-by: Liav Albani <liavalb@gmail.com>
>
> Acked-by: Ani Sinha <ani@anisinha.ca>
>
> > ---
> > tests/data/acpi/q35/FACP | Bin 244 -> 244 bytes
> > tests/data/acpi/q35/FACP.nosmm | Bin 244 -> 244 bytes
> > tests/data/acpi/q35/FACP.slic | Bin 244 -> 244 bytes
> > tests/data/acpi/q35/FACP.xapic | Bin 244 -> 244 bytes
> > tests/qtest/bios-tables-test-allowed-diff.h | 4 ----
> > 5 files changed, 4 deletions(-)
> >
> > diff --git a/tests/data/acpi/q35/FACP b/tests/data/acpi/q35/FACP
> > index f6a864cc863c7763f6c09d3814ad184a658fa0a0..a8f6a8961109d01059aceef9f1869cde09a2f10c 100644
> > GIT binary patch
> > delta 23
> > ecmeyu_=S<n&CxmF3j+fK^Y)2c$&5@B^V$GgGY3Ne
> >
> > delta 23
> > ecmeyu_=S<n&CxmF3j+fK^UjG}$&3sW^V$GgJqJSo
> >
> > diff --git a/tests/data/acpi/q35/FACP.nosmm b/tests/data/acpi/q35/FACP.nosmm
> > index 6a9aa5f370eb9af6a03dc739d8a159be58fdee01..c4e6d18ee5fc64159160d4589aa96b4d648c913a 100644
> > GIT binary patch
> > delta 23
> > ecmeyu_=S<n&CxmF3j+fKbKXR*WJacmd2Ik#q6Yc^
> >
> > delta 23
> > ecmeyu_=S<n&CxmF3j+fKbHPNeWJZRGd2Ik#tOoi3
> >
> > diff --git a/tests/data/acpi/q35/FACP.slic b/tests/data/acpi/q35/FACP.slic
> > index 15986e095cf2db7ee92f7ce113c1d46d54018c62..48bbb1cf5ad0ceda1d2f6d56edf5c1e207bd1a04 100644
> > GIT binary patch
> > delta 23
> > ecmeyu_=S<n&CxmF3j+fK^M#3A$&5@B^V$Gh6bD=Y
> >
> > delta 23
> > ecmeyu_=S<n&CxmF3j+fK^QDPg$&3sW^V$Gh9tT_i
> >
> > diff --git a/tests/data/acpi/q35/FACP.xapic b/tests/data/acpi/q35/FACP.xapic
> > index 2d3659c9c6753d07c3d48742343cb8e8cc034de7..31fa5dd19c213034eef4eeefa6a04e61dadd8a2a 100644
> > GIT binary patch
> > delta 23
> > ecmeyu_=S<n&CxmF3j+fK^X7?M$&5@B^V$Gg4+lR0
> >
> > delta 23
> > ecmeyu_=S<n&CxmF3j+fK^VW%6$&3sW^V$Gg83#WA
> >
> > diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> > index 7570e39369..dfb8523c8b 100644
> > --- a/tests/qtest/bios-tables-test-allowed-diff.h
> > +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> > @@ -1,5 +1 @@
> > /* List of comma-separated changed AML files to ignore */
> > -"tests/data/acpi/q35/FACP",
> > -"tests/data/acpi/q35/FACP.nosmm",
> > -"tests/data/acpi/q35/FACP.slic",
> > -"tests/data/acpi/q35/FACP.xapic",
> > --
> > 2.35.1
> >
> >
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-03-01 11:19 ` Michael S. Tsirkin
@ 2022-03-01 11:47 ` Igor Mammedov
2022-03-01 12:18 ` Michael S. Tsirkin
2022-03-01 19:11 ` Liav Albani
1 sibling, 1 reply; 32+ messages in thread
From: Igor Mammedov @ 2022-03-01 11:47 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: ani, shentey, Liav Albani, qemu-devel
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 */
> > >
> > > /*
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-03-01 9:52 ` Ani Sinha
@ 2022-03-01 11:51 ` Igor Mammedov
2022-03-01 19:20 ` Liav Albani
1 sibling, 0 replies; 32+ messages in thread
From: Igor Mammedov @ 2022-03-01 11:51 UTC (permalink / raw)
To: Ani Sinha; +Cc: mst, shentey, Liav Albani, qemu-devel
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>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-03-01 11:47 ` Igor Mammedov
@ 2022-03-01 12:18 ` Michael S. Tsirkin
2022-03-02 15:45 ` Liav Albani
0 siblings, 1 reply; 32+ messages in thread
From: Michael S. Tsirkin @ 2022-03-01 12:18 UTC (permalink / raw)
To: Igor Mammedov; +Cc: ani, shentey, Liav Albani, qemu-devel
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 */
> > > >
> > > > /*
> >
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-03-01 11:19 ` Michael S. Tsirkin
2022-03-01 11:47 ` Igor Mammedov
@ 2022-03-01 19:11 ` Liav Albani
2022-03-02 5:12 ` Ani Sinha
2022-03-02 8:47 ` Michael S. Tsirkin
1 sibling, 2 replies; 32+ messages in thread
From: Liav Albani @ 2022-03-01 19:11 UTC (permalink / raw)
To: Michael S. Tsirkin, Igor Mammedov; +Cc: ani, shentey, qemu-devel
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 */
>>>
>>> /*
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 3/3] tests/acpi: i386: update FACP table differences
2022-03-01 11:21 ` Michael S. Tsirkin
@ 2022-03-01 19:13 ` Liav Albani
2022-03-02 5:05 ` Ani Sinha
2022-03-02 8:48 ` Michael S. Tsirkin
0 siblings, 2 replies; 32+ messages in thread
From: Liav Albani @ 2022-03-01 19:13 UTC (permalink / raw)
To: Michael S. Tsirkin, Ani Sinha; +Cc: imammedo, shentey, qemu-devel
On 3/1/22 13:21, Michael S. Tsirkin wrote:
> On Tue, Mar 01, 2022 at 08:29:57AM +0530, Ani Sinha wrote:
>>
>> On Mon, 28 Feb 2022, Liav Albani wrote:
>>
>>> After changing the IAPC boot flags register to indicate support of i8042
>>> in the machine chipset to help the guest OS to determine its existence
>>> "faster", we need to have the updated FACP ACPI binary images in tree.
>>>
>>> @@ -1,32 +1,32 @@
>>> /*
>>> * Intel ACPI Component Architecture
>>> * AML/ASL+ Disassembler version 20211217 (64-bit version)
>>> * Copyright (c) 2000 - 2021 Intel Corporation
>>> *
>>> - * Disassembly of tests/data/acpi/q35/FACP, Wed Feb 23 22:37:39 2022
>>> + * Disassembly of /tmp/aml-BBFBI1, Wed Feb 23 22:37:39 2022
> cut this out pls
I see, this is indeed not very useful...
>>> *
>>> * ACPI Data Table [FACP]
>>> *
>>> * Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue (in hex)
>>> */
>>>
>>> [000h 0000 4] Signature : "FACP" [Fixed ACPI Description Table (FADT)]
>>> [004h 0004 4] Table Length : 000000F4
>>> [008h 0008 1] Revision : 03
>>> -[009h 0009 1] Checksum : B9
>>> +[009h 0009 1] Checksum : B7
> and this
>
>>> [00Ah 0010 6] Oem ID : "BOCHS "
>>> [010h 0016 8] Oem Table ID : "BXPC "
>>> [018h 0024 4] Oem Revision : 00000001
>>> [01Ch 0028 4] Asl Compiler ID : "BXPC"
>>> [020h 0032 4] Asl Compiler Revision : 00000001
>>>
>>> [024h 0036 4] FACS Address : 00000000
>>> [028h 0040 4] DSDT Address : 00000000
>>> [02Ch 0044 1] Model : 01
>>> [02Dh 0045 1] PM Profile : 00 [Unspecified]
>>> [02Eh 0046 2] SCI Interrupt : 0009
>>> [030h 0048 4] SMI Command Port : 000000B2
>>> [034h 0052 1] ACPI Enable Value : 02
>>> [035h 0053 1] ACPI Disable Value : 03
>>> [036h 0054 1] S4BIOS Command : 00
>>> [037h 0055 1] P-State Control : 00
>>> @@ -42,35 +42,35 @@
>>> [059h 0089 1] PM1 Control Block Length : 02
>>> [05Ah 0090 1] PM2 Control Block Length : 00
>>> [05Bh 0091 1] PM Timer Block Length : 04
>>> [05Ch 0092 1] GPE0 Block Length : 10
>>> [05Dh 0093 1] GPE1 Block Length : 00
>>> [05Eh 0094 1] GPE1 Base Offset : 00
>>> [05Fh 0095 1] _CST Support : 00
>>> [060h 0096 2] C2 Latency : 0FFF
>>> [062h 0098 2] C3 Latency : 0FFF
>>> [064h 0100 2] CPU Cache Size : 0000
>>> [066h 0102 2] Cache Flush Stride : 0000
>>> [068h 0104 1] Duty Cycle Offset : 00
>>> [069h 0105 1] Duty Cycle Width : 00
>>> [06Ah 0106 1] RTC Day Alarm Index : 00
>>> [06Bh 0107 1] RTC Month Alarm Index : 00
>>> [06Ch 0108 1] RTC Century Index : 32
>>> -[06Dh 0109 2] Boot Flags (decoded below) : 0000
>>> +[06Dh 0109 2] Boot Flags (decoded below) : 0002
>>> Legacy Devices Supported (V2) : 0
>>> - 8042 Present on ports 60/64 (V2) : 0
>>> + 8042 Present on ports 60/64 (V2) : 1
>>> VGA Not Present (V4) : 0
>>> MSI Not Supported (V4) : 0
>>> PCIe ASPM Not Supported (V4) : 0
>>> CMOS RTC Not Present (V5) : 0
>
> leaving just this
>
It will be fixed in version 5.
>>> [06Fh 0111 1] Reserved : 00
>>> [070h 0112 4] Flags (decoded below) : 000084A5
>>> WBINVD instruction is operational (V1) : 1
>>> WBINVD flushes all caches (V1) : 0
>>> All CPUs support C1 (V1) : 1
>>> C2 works on MP system (V1) : 0
>>> Control Method Power Button (V1) : 0
>>> Control Method Sleep Button (V1) : 1
>>> RTC wake not in fixed reg space (V1) : 0
>>> RTC can wake system from S4 (V1) : 1
>>> 32-bit PM Timer (V1) : 0
>>> Docking Supported (V1) : 0
>>> @@ -148,32 +148,32 @@
>>> [0DCh 0220 1] Space ID : 01 [SystemIO]
>>> [0DDh 0221 1] Bit Width : 80
>>> [0DEh 0222 1] Bit Offset : 00
>>> [0DFh 0223 1] Encoded Access Width : 00 [Undefined/Legacy]
>>> [0E0h 0224 8] Address : 0000000000000620
>>>
>>> [0E8h 0232 12] GPE1 Block : [Generic Address Structure]
>>> [0E8h 0232 1] Space ID : 00 [SystemMemory]
>>> [0E9h 0233 1] Bit Width : 00
>>> [0EAh 0234 1] Bit Offset : 00
>>> [0EBh 0235 1] Encoded Access Width : 00 [Undefined/Legacy]
>>> [0ECh 0236 8] Address : 0000000000000000
>>>
>>> Raw Table Data: Length 244 (0xF4)
>>>
>>> - 0000: 46 41 43 50 F4 00 00 00 03 B9 42 4F 43 48 53 20 // FACP......BOCHS
>>> + 0000: 46 41 43 50 F4 00 00 00 03 B7 42 4F 43 48 53 20 // FACP......BOCHS
>>> 0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43 // BXPC ....BXPC
>>> 0020: 01 00 00 00 00 00 00 00 00 00 00 00 01 00 09 00 // ................
>>> 0030: B2 00 00 00 02 03 00 00 00 06 00 00 00 00 00 00 // ................
>>> 0040: 04 06 00 00 00 00 00 00 00 00 00 00 08 06 00 00 // ................
>>> 0050: 20 06 00 00 00 00 00 00 04 02 00 04 10 00 00 00 // ...............
>>> - 0060: FF 0F FF 0F 00 00 00 00 00 00 00 00 32 00 00 00 // ............2...
>>> + 0060: FF 0F FF 0F 00 00 00 00 00 00 00 00 32 02 00 00 // ............2...
>>> 0070: A5 84 00 00 01 08 00 00 F9 0C 00 00 00 00 00 00 // ................
>>> 0080: 0F 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
>>> 0090: 00 00 00 00 01 20 00 00 00 06 00 00 00 00 00 00 // ..... ..........
>>> 00A0: 00 00 00 00 00 00 00 00 00 00 00 00 01 10 00 00 // ................
>>> 00B0: 04 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
>>> 00C0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
>>> 00D0: 01 20 00 00 08 06 00 00 00 00 00 00 01 80 00 00 // . ..............
>>> 00E0: 20 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ...............
>>> 00F0: 00 00 00 00 // ....
> this isn't helpful either and will be wrong if cherry picked.
It will be fixed in version 5 :)
I'll have to not retain the Ack sign of Ani, right?
>
>>> **
>> Please retain tags from earlier revisions of the patch.
>>
>>> Signed-off-by: Liav Albani <liavalb@gmail.com>
>> Acked-by: Ani Sinha <ani@anisinha.ca>
>>
>>> ---
>>> tests/data/acpi/q35/FACP | Bin 244 -> 244 bytes
>>> tests/data/acpi/q35/FACP.nosmm | Bin 244 -> 244 bytes
>>> tests/data/acpi/q35/FACP.slic | Bin 244 -> 244 bytes
>>> tests/data/acpi/q35/FACP.xapic | Bin 244 -> 244 bytes
>>> tests/qtest/bios-tables-test-allowed-diff.h | 4 ----
>>> 5 files changed, 4 deletions(-)
>>>
>>> diff --git a/tests/data/acpi/q35/FACP b/tests/data/acpi/q35/FACP
>>> index f6a864cc863c7763f6c09d3814ad184a658fa0a0..a8f6a8961109d01059aceef9f1869cde09a2f10c 100644
>>> GIT binary patch
>>> delta 23
>>> ecmeyu_=S<n&CxmF3j+fK^Y)2c$&5@B^V$GgGY3Ne
>>>
>>> delta 23
>>> ecmeyu_=S<n&CxmF3j+fK^UjG}$&3sW^V$GgJqJSo
>>>
>>> diff --git a/tests/data/acpi/q35/FACP.nosmm b/tests/data/acpi/q35/FACP.nosmm
>>> index 6a9aa5f370eb9af6a03dc739d8a159be58fdee01..c4e6d18ee5fc64159160d4589aa96b4d648c913a 100644
>>> GIT binary patch
>>> delta 23
>>> ecmeyu_=S<n&CxmF3j+fKbKXR*WJacmd2Ik#q6Yc^
>>>
>>> delta 23
>>> ecmeyu_=S<n&CxmF3j+fKbHPNeWJZRGd2Ik#tOoi3
>>>
>>> diff --git a/tests/data/acpi/q35/FACP.slic b/tests/data/acpi/q35/FACP.slic
>>> index 15986e095cf2db7ee92f7ce113c1d46d54018c62..48bbb1cf5ad0ceda1d2f6d56edf5c1e207bd1a04 100644
>>> GIT binary patch
>>> delta 23
>>> ecmeyu_=S<n&CxmF3j+fK^M#3A$&5@B^V$Gh6bD=Y
>>>
>>> delta 23
>>> ecmeyu_=S<n&CxmF3j+fK^QDPg$&3sW^V$Gh9tT_i
>>>
>>> diff --git a/tests/data/acpi/q35/FACP.xapic b/tests/data/acpi/q35/FACP.xapic
>>> index 2d3659c9c6753d07c3d48742343cb8e8cc034de7..31fa5dd19c213034eef4eeefa6a04e61dadd8a2a 100644
>>> GIT binary patch
>>> delta 23
>>> ecmeyu_=S<n&CxmF3j+fK^X7?M$&5@B^V$Gg4+lR0
>>>
>>> delta 23
>>> ecmeyu_=S<n&CxmF3j+fK^VW%6$&3sW^V$Gg83#WA
>>>
>>> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
>>> index 7570e39369..dfb8523c8b 100644
>>> --- a/tests/qtest/bios-tables-test-allowed-diff.h
>>> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
>>> @@ -1,5 +1 @@
>>> /* List of comma-separated changed AML files to ignore */
>>> -"tests/data/acpi/q35/FACP",
>>> -"tests/data/acpi/q35/FACP.nosmm",
>>> -"tests/data/acpi/q35/FACP.slic",
>>> -"tests/data/acpi/q35/FACP.xapic",
>>> --
>>> 2.35.1
>>>
>>>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-03-01 9:52 ` Ani Sinha
2022-03-01 11:51 ` Igor Mammedov
@ 2022-03-01 19:20 ` Liav Albani
2022-03-02 5:14 ` Ani Sinha
1 sibling, 1 reply; 32+ messages in thread
From: Liav Albani @ 2022-03-01 19:20 UTC (permalink / raw)
To: Ani Sinha, Igor Mammedov; +Cc: shentey, qemu-devel, mst
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
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 3/3] tests/acpi: i386: update FACP table differences
2022-03-01 19:13 ` Liav Albani
@ 2022-03-02 5:05 ` Ani Sinha
2022-03-02 8:48 ` Michael S. Tsirkin
1 sibling, 0 replies; 32+ messages in thread
From: Ani Sinha @ 2022-03-02 5:05 UTC (permalink / raw)
To: Liav Albani; +Cc: imammedo, shentey, qemu-devel, Michael S. Tsirkin
On Wed, Mar 2, 2022 at 12:43 AM Liav Albani <liavalb@gmail.com> wrote:
>
>
> On 3/1/22 13:21, Michael S. Tsirkin wrote:
> > On Tue, Mar 01, 2022 at 08:29:57AM +0530, Ani Sinha wrote:
> >>
> >> On Mon, 28 Feb 2022, Liav Albani wrote:
> >>
> >>> After changing the IAPC boot flags register to indicate support of i8042
> >>> in the machine chipset to help the guest OS to determine its existence
> >>> "faster", we need to have the updated FACP ACPI binary images in tree.
> >>>
> >>> @@ -1,32 +1,32 @@
> >>> /*
> >>> * Intel ACPI Component Architecture
> >>> * AML/ASL+ Disassembler version 20211217 (64-bit version)
> >>> * Copyright (c) 2000 - 2021 Intel Corporation
> >>> *
> >>> - * Disassembly of tests/data/acpi/q35/FACP, Wed Feb 23 22:37:39 2022
> >>> + * Disassembly of /tmp/aml-BBFBI1, Wed Feb 23 22:37:39 2022
> > cut this out pls
> I see, this is indeed not very useful...
> >>> *
> >>> * ACPI Data Table [FACP]
> >>> *
> >>> * Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue (in hex)
> >>> */
> >>>
> >>> [000h 0000 4] Signature : "FACP" [Fixed ACPI Description Table (FADT)]
> >>> [004h 0004 4] Table Length : 000000F4
> >>> [008h 0008 1] Revision : 03
> >>> -[009h 0009 1] Checksum : B9
> >>> +[009h 0009 1] Checksum : B7
> > and this
> >
> >>> [00Ah 0010 6] Oem ID : "BOCHS "
> >>> [010h 0016 8] Oem Table ID : "BXPC "
> >>> [018h 0024 4] Oem Revision : 00000001
> >>> [01Ch 0028 4] Asl Compiler ID : "BXPC"
> >>> [020h 0032 4] Asl Compiler Revision : 00000001
> >>>
> >>> [024h 0036 4] FACS Address : 00000000
> >>> [028h 0040 4] DSDT Address : 00000000
> >>> [02Ch 0044 1] Model : 01
> >>> [02Dh 0045 1] PM Profile : 00 [Unspecified]
> >>> [02Eh 0046 2] SCI Interrupt : 0009
> >>> [030h 0048 4] SMI Command Port : 000000B2
> >>> [034h 0052 1] ACPI Enable Value : 02
> >>> [035h 0053 1] ACPI Disable Value : 03
> >>> [036h 0054 1] S4BIOS Command : 00
> >>> [037h 0055 1] P-State Control : 00
> >>> @@ -42,35 +42,35 @@
> >>> [059h 0089 1] PM1 Control Block Length : 02
> >>> [05Ah 0090 1] PM2 Control Block Length : 00
> >>> [05Bh 0091 1] PM Timer Block Length : 04
> >>> [05Ch 0092 1] GPE0 Block Length : 10
> >>> [05Dh 0093 1] GPE1 Block Length : 00
> >>> [05Eh 0094 1] GPE1 Base Offset : 00
> >>> [05Fh 0095 1] _CST Support : 00
> >>> [060h 0096 2] C2 Latency : 0FFF
> >>> [062h 0098 2] C3 Latency : 0FFF
> >>> [064h 0100 2] CPU Cache Size : 0000
> >>> [066h 0102 2] Cache Flush Stride : 0000
> >>> [068h 0104 1] Duty Cycle Offset : 00
> >>> [069h 0105 1] Duty Cycle Width : 00
> >>> [06Ah 0106 1] RTC Day Alarm Index : 00
> >>> [06Bh 0107 1] RTC Month Alarm Index : 00
> >>> [06Ch 0108 1] RTC Century Index : 32
> >>> -[06Dh 0109 2] Boot Flags (decoded below) : 0000
> >>> +[06Dh 0109 2] Boot Flags (decoded below) : 0002
> >>> Legacy Devices Supported (V2) : 0
> >>> - 8042 Present on ports 60/64 (V2) : 0
> >>> + 8042 Present on ports 60/64 (V2) : 1
> >>> VGA Not Present (V4) : 0
> >>> MSI Not Supported (V4) : 0
> >>> PCIe ASPM Not Supported (V4) : 0
> >>> CMOS RTC Not Present (V5) : 0
> >
> > leaving just this
> >
> It will be fixed in version 5.
> >>> [06Fh 0111 1] Reserved : 00
> >>> [070h 0112 4] Flags (decoded below) : 000084A5
> >>> WBINVD instruction is operational (V1) : 1
> >>> WBINVD flushes all caches (V1) : 0
> >>> All CPUs support C1 (V1) : 1
> >>> C2 works on MP system (V1) : 0
> >>> Control Method Power Button (V1) : 0
> >>> Control Method Sleep Button (V1) : 1
> >>> RTC wake not in fixed reg space (V1) : 0
> >>> RTC can wake system from S4 (V1) : 1
> >>> 32-bit PM Timer (V1) : 0
> >>> Docking Supported (V1) : 0
> >>> @@ -148,32 +148,32 @@
> >>> [0DCh 0220 1] Space ID : 01 [SystemIO]
> >>> [0DDh 0221 1] Bit Width : 80
> >>> [0DEh 0222 1] Bit Offset : 00
> >>> [0DFh 0223 1] Encoded Access Width : 00 [Undefined/Legacy]
> >>> [0E0h 0224 8] Address : 0000000000000620
> >>>
> >>> [0E8h 0232 12] GPE1 Block : [Generic Address Structure]
> >>> [0E8h 0232 1] Space ID : 00 [SystemMemory]
> >>> [0E9h 0233 1] Bit Width : 00
> >>> [0EAh 0234 1] Bit Offset : 00
> >>> [0EBh 0235 1] Encoded Access Width : 00 [Undefined/Legacy]
> >>> [0ECh 0236 8] Address : 0000000000000000
> >>>
> >>> Raw Table Data: Length 244 (0xF4)
> >>>
> >>> - 0000: 46 41 43 50 F4 00 00 00 03 B9 42 4F 43 48 53 20 // FACP......BOCHS
> >>> + 0000: 46 41 43 50 F4 00 00 00 03 B7 42 4F 43 48 53 20 // FACP......BOCHS
> >>> 0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43 // BXPC ....BXPC
> >>> 0020: 01 00 00 00 00 00 00 00 00 00 00 00 01 00 09 00 // ................
> >>> 0030: B2 00 00 00 02 03 00 00 00 06 00 00 00 00 00 00 // ................
> >>> 0040: 04 06 00 00 00 00 00 00 00 00 00 00 08 06 00 00 // ................
> >>> 0050: 20 06 00 00 00 00 00 00 04 02 00 04 10 00 00 00 // ...............
> >>> - 0060: FF 0F FF 0F 00 00 00 00 00 00 00 00 32 00 00 00 // ............2...
> >>> + 0060: FF 0F FF 0F 00 00 00 00 00 00 00 00 32 02 00 00 // ............2...
> >>> 0070: A5 84 00 00 01 08 00 00 F9 0C 00 00 00 00 00 00 // ................
> >>> 0080: 0F 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> >>> 0090: 00 00 00 00 01 20 00 00 00 06 00 00 00 00 00 00 // ..... ..........
> >>> 00A0: 00 00 00 00 00 00 00 00 00 00 00 00 01 10 00 00 // ................
> >>> 00B0: 04 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> >>> 00C0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> >>> 00D0: 01 20 00 00 08 06 00 00 00 00 00 00 01 80 00 00 // . ..............
> >>> 00E0: 20 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ...............
> >>> 00F0: 00 00 00 00 // ....
> > this isn't helpful either and will be wrong if cherry picked.
>
> It will be fixed in version 5 :)
>
> I'll have to not retain the Ack sign of Ani, right?
No you do but you should still address Michael's comments. Different
reviewers look for different things and they can ack/tag reviewed-by
if they were satisfied. Others can raise further concerns or comments
thereafter also. It does not mean previous tags needs removal. They
stand on their own merit.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-03-01 19:11 ` Liav Albani
@ 2022-03-02 5:12 ` Ani Sinha
2022-03-02 8:47 ` Michael S. Tsirkin
1 sibling, 0 replies; 32+ messages in thread
From: Ani Sinha @ 2022-03-02 5:12 UTC (permalink / raw)
To: Liav Albani; +Cc: Igor Mammedov, shentey, qemu-devel, Michael S. Tsirkin
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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-03-01 19:20 ` Liav Albani
@ 2022-03-02 5:14 ` Ani Sinha
2022-03-02 12:42 ` Michael S. Tsirkin
0 siblings, 1 reply; 32+ messages in thread
From: Ani Sinha @ 2022-03-02 5:14 UTC (permalink / raw)
To: Liav Albani; +Cc: Igor Mammedov, shentey, qemu-devel, mst
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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-03-01 19:11 ` Liav Albani
2022-03-02 5:12 ` Ani Sinha
@ 2022-03-02 8:47 ` Michael S. Tsirkin
1 sibling, 0 replies; 32+ messages in thread
From: Michael S. Tsirkin @ 2022-03-02 8:47 UTC (permalink / raw)
To: Liav Albani; +Cc: ani, Igor Mammedov, shentey, qemu-devel
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 */
> > > > /*
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 3/3] tests/acpi: i386: update FACP table differences
2022-03-01 19:13 ` Liav Albani
2022-03-02 5:05 ` Ani Sinha
@ 2022-03-02 8:48 ` Michael S. Tsirkin
1 sibling, 0 replies; 32+ messages in thread
From: Michael S. Tsirkin @ 2022-03-02 8:48 UTC (permalink / raw)
To: Liav Albani; +Cc: Ani Sinha, imammedo, shentey, qemu-devel
On Tue, Mar 01, 2022 at 09:13:04PM +0200, Liav Albani wrote:
>
> On 3/1/22 13:21, Michael S. Tsirkin wrote:
> > On Tue, Mar 01, 2022 at 08:29:57AM +0530, Ani Sinha wrote:
> > >
> > > On Mon, 28 Feb 2022, Liav Albani wrote:
> > >
> > > > After changing the IAPC boot flags register to indicate support of i8042
> > > > in the machine chipset to help the guest OS to determine its existence
> > > > "faster", we need to have the updated FACP ACPI binary images in tree.
> > > >
> > > > @@ -1,32 +1,32 @@
> > > > /*
> > > > * Intel ACPI Component Architecture
> > > > * AML/ASL+ Disassembler version 20211217 (64-bit version)
> > > > * Copyright (c) 2000 - 2021 Intel Corporation
> > > > *
> > > > - * Disassembly of tests/data/acpi/q35/FACP, Wed Feb 23 22:37:39 2022
> > > > + * Disassembly of /tmp/aml-BBFBI1, Wed Feb 23 22:37:39 2022
> > cut this out pls
> I see, this is indeed not very useful...
> > > > *
> > > > * ACPI Data Table [FACP]
> > > > *
> > > > * Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue (in hex)
> > > > */
> > > >
> > > > [000h 0000 4] Signature : "FACP" [Fixed ACPI Description Table (FADT)]
> > > > [004h 0004 4] Table Length : 000000F4
> > > > [008h 0008 1] Revision : 03
> > > > -[009h 0009 1] Checksum : B9
> > > > +[009h 0009 1] Checksum : B7
> > and this
> >
> > > > [00Ah 0010 6] Oem ID : "BOCHS "
> > > > [010h 0016 8] Oem Table ID : "BXPC "
> > > > [018h 0024 4] Oem Revision : 00000001
> > > > [01Ch 0028 4] Asl Compiler ID : "BXPC"
> > > > [020h 0032 4] Asl Compiler Revision : 00000001
> > > >
> > > > [024h 0036 4] FACS Address : 00000000
> > > > [028h 0040 4] DSDT Address : 00000000
> > > > [02Ch 0044 1] Model : 01
> > > > [02Dh 0045 1] PM Profile : 00 [Unspecified]
> > > > [02Eh 0046 2] SCI Interrupt : 0009
> > > > [030h 0048 4] SMI Command Port : 000000B2
> > > > [034h 0052 1] ACPI Enable Value : 02
> > > > [035h 0053 1] ACPI Disable Value : 03
> > > > [036h 0054 1] S4BIOS Command : 00
> > > > [037h 0055 1] P-State Control : 00
> > > > @@ -42,35 +42,35 @@
> > > > [059h 0089 1] PM1 Control Block Length : 02
> > > > [05Ah 0090 1] PM2 Control Block Length : 00
> > > > [05Bh 0091 1] PM Timer Block Length : 04
> > > > [05Ch 0092 1] GPE0 Block Length : 10
> > > > [05Dh 0093 1] GPE1 Block Length : 00
> > > > [05Eh 0094 1] GPE1 Base Offset : 00
> > > > [05Fh 0095 1] _CST Support : 00
> > > > [060h 0096 2] C2 Latency : 0FFF
> > > > [062h 0098 2] C3 Latency : 0FFF
> > > > [064h 0100 2] CPU Cache Size : 0000
> > > > [066h 0102 2] Cache Flush Stride : 0000
> > > > [068h 0104 1] Duty Cycle Offset : 00
> > > > [069h 0105 1] Duty Cycle Width : 00
> > > > [06Ah 0106 1] RTC Day Alarm Index : 00
> > > > [06Bh 0107 1] RTC Month Alarm Index : 00
> > > > [06Ch 0108 1] RTC Century Index : 32
> > > > -[06Dh 0109 2] Boot Flags (decoded below) : 0000
> > > > +[06Dh 0109 2] Boot Flags (decoded below) : 0002
> > > > Legacy Devices Supported (V2) : 0
> > > > - 8042 Present on ports 60/64 (V2) : 0
> > > > + 8042 Present on ports 60/64 (V2) : 1
> > > > VGA Not Present (V4) : 0
> > > > MSI Not Supported (V4) : 0
> > > > PCIe ASPM Not Supported (V4) : 0
> > > > CMOS RTC Not Present (V5) : 0
> >
> > leaving just this
> >
> It will be fixed in version 5.
> > > > [06Fh 0111 1] Reserved : 00
> > > > [070h 0112 4] Flags (decoded below) : 000084A5
> > > > WBINVD instruction is operational (V1) : 1
> > > > WBINVD flushes all caches (V1) : 0
> > > > All CPUs support C1 (V1) : 1
> > > > C2 works on MP system (V1) : 0
> > > > Control Method Power Button (V1) : 0
> > > > Control Method Sleep Button (V1) : 1
> > > > RTC wake not in fixed reg space (V1) : 0
> > > > RTC can wake system from S4 (V1) : 1
> > > > 32-bit PM Timer (V1) : 0
> > > > Docking Supported (V1) : 0
> > > > @@ -148,32 +148,32 @@
> > > > [0DCh 0220 1] Space ID : 01 [SystemIO]
> > > > [0DDh 0221 1] Bit Width : 80
> > > > [0DEh 0222 1] Bit Offset : 00
> > > > [0DFh 0223 1] Encoded Access Width : 00 [Undefined/Legacy]
> > > > [0E0h 0224 8] Address : 0000000000000620
> > > >
> > > > [0E8h 0232 12] GPE1 Block : [Generic Address Structure]
> > > > [0E8h 0232 1] Space ID : 00 [SystemMemory]
> > > > [0E9h 0233 1] Bit Width : 00
> > > > [0EAh 0234 1] Bit Offset : 00
> > > > [0EBh 0235 1] Encoded Access Width : 00 [Undefined/Legacy]
> > > > [0ECh 0236 8] Address : 0000000000000000
> > > >
> > > > Raw Table Data: Length 244 (0xF4)
> > > >
> > > > - 0000: 46 41 43 50 F4 00 00 00 03 B9 42 4F 43 48 53 20 // FACP......BOCHS
> > > > + 0000: 46 41 43 50 F4 00 00 00 03 B7 42 4F 43 48 53 20 // FACP......BOCHS
> > > > 0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43 // BXPC ....BXPC
> > > > 0020: 01 00 00 00 00 00 00 00 00 00 00 00 01 00 09 00 // ................
> > > > 0030: B2 00 00 00 02 03 00 00 00 06 00 00 00 00 00 00 // ................
> > > > 0040: 04 06 00 00 00 00 00 00 00 00 00 00 08 06 00 00 // ................
> > > > 0050: 20 06 00 00 00 00 00 00 04 02 00 04 10 00 00 00 // ...............
> > > > - 0060: FF 0F FF 0F 00 00 00 00 00 00 00 00 32 00 00 00 // ............2...
> > > > + 0060: FF 0F FF 0F 00 00 00 00 00 00 00 00 32 02 00 00 // ............2...
> > > > 0070: A5 84 00 00 01 08 00 00 F9 0C 00 00 00 00 00 00 // ................
> > > > 0080: 0F 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> > > > 0090: 00 00 00 00 01 20 00 00 00 06 00 00 00 00 00 00 // ..... ..........
> > > > 00A0: 00 00 00 00 00 00 00 00 00 00 00 00 01 10 00 00 // ................
> > > > 00B0: 04 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> > > > 00C0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> > > > 00D0: 01 20 00 00 08 06 00 00 00 00 00 00 01 80 00 00 // . ..............
> > > > 00E0: 20 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ...............
> > > > 00F0: 00 00 00 00 // ....
> > this isn't helpful either and will be wrong if cherry picked.
>
> It will be fixed in version 5 :)
>
> I'll have to not retain the Ack sign of Ani, right?
if all you change is commit log, acks normally stand.
> >
> > > > **
> > > Please retain tags from earlier revisions of the patch.
> > >
> > > > Signed-off-by: Liav Albani <liavalb@gmail.com>
> > > Acked-by: Ani Sinha <ani@anisinha.ca>
> > >
> > > > ---
> > > > tests/data/acpi/q35/FACP | Bin 244 -> 244 bytes
> > > > tests/data/acpi/q35/FACP.nosmm | Bin 244 -> 244 bytes
> > > > tests/data/acpi/q35/FACP.slic | Bin 244 -> 244 bytes
> > > > tests/data/acpi/q35/FACP.xapic | Bin 244 -> 244 bytes
> > > > tests/qtest/bios-tables-test-allowed-diff.h | 4 ----
> > > > 5 files changed, 4 deletions(-)
> > > >
> > > > diff --git a/tests/data/acpi/q35/FACP b/tests/data/acpi/q35/FACP
> > > > index f6a864cc863c7763f6c09d3814ad184a658fa0a0..a8f6a8961109d01059aceef9f1869cde09a2f10c 100644
> > > > GIT binary patch
> > > > delta 23
> > > > ecmeyu_=S<n&CxmF3j+fK^Y)2c$&5@B^V$GgGY3Ne
> > > >
> > > > delta 23
> > > > ecmeyu_=S<n&CxmF3j+fK^UjG}$&3sW^V$GgJqJSo
> > > >
> > > > diff --git a/tests/data/acpi/q35/FACP.nosmm b/tests/data/acpi/q35/FACP.nosmm
> > > > index 6a9aa5f370eb9af6a03dc739d8a159be58fdee01..c4e6d18ee5fc64159160d4589aa96b4d648c913a 100644
> > > > GIT binary patch
> > > > delta 23
> > > > ecmeyu_=S<n&CxmF3j+fKbKXR*WJacmd2Ik#q6Yc^
> > > >
> > > > delta 23
> > > > ecmeyu_=S<n&CxmF3j+fKbHPNeWJZRGd2Ik#tOoi3
> > > >
> > > > diff --git a/tests/data/acpi/q35/FACP.slic b/tests/data/acpi/q35/FACP.slic
> > > > index 15986e095cf2db7ee92f7ce113c1d46d54018c62..48bbb1cf5ad0ceda1d2f6d56edf5c1e207bd1a04 100644
> > > > GIT binary patch
> > > > delta 23
> > > > ecmeyu_=S<n&CxmF3j+fK^M#3A$&5@B^V$Gh6bD=Y
> > > >
> > > > delta 23
> > > > ecmeyu_=S<n&CxmF3j+fK^QDPg$&3sW^V$Gh9tT_i
> > > >
> > > > diff --git a/tests/data/acpi/q35/FACP.xapic b/tests/data/acpi/q35/FACP.xapic
> > > > index 2d3659c9c6753d07c3d48742343cb8e8cc034de7..31fa5dd19c213034eef4eeefa6a04e61dadd8a2a 100644
> > > > GIT binary patch
> > > > delta 23
> > > > ecmeyu_=S<n&CxmF3j+fK^X7?M$&5@B^V$Gg4+lR0
> > > >
> > > > delta 23
> > > > ecmeyu_=S<n&CxmF3j+fK^VW%6$&3sW^V$Gg83#WA
> > > >
> > > > diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> > > > index 7570e39369..dfb8523c8b 100644
> > > > --- a/tests/qtest/bios-tables-test-allowed-diff.h
> > > > +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> > > > @@ -1,5 +1 @@
> > > > /* List of comma-separated changed AML files to ignore */
> > > > -"tests/data/acpi/q35/FACP",
> > > > -"tests/data/acpi/q35/FACP.nosmm",
> > > > -"tests/data/acpi/q35/FACP.slic",
> > > > -"tests/data/acpi/q35/FACP.xapic",
> > > > --
> > > > 2.35.1
> > > >
> > > >
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-03-02 5:14 ` Ani Sinha
@ 2022-03-02 12:42 ` Michael S. Tsirkin
2022-03-02 15:43 ` Liav Albani
2022-03-02 15:58 ` Ani Sinha
0 siblings, 2 replies; 32+ messages in thread
From: Michael S. Tsirkin @ 2022-03-02 12:42 UTC (permalink / raw)
To: Ani Sinha; +Cc: Igor Mammedov, shentey, Liav Albani, qemu-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-03-02 12:42 ` Michael S. Tsirkin
@ 2022-03-02 15:43 ` Liav Albani
2022-03-02 15:51 ` Ani Sinha
2022-03-02 15:58 ` Ani Sinha
1 sibling, 1 reply; 32+ messages in thread
From: Liav Albani @ 2022-03-02 15:43 UTC (permalink / raw)
To: Michael S. Tsirkin, Ani Sinha; +Cc: Igor Mammedov, shentey, qemu-devel
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...
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-03-01 12:18 ` Michael S. Tsirkin
@ 2022-03-02 15:45 ` Liav Albani
2022-03-04 10:58 ` Igor Mammedov
0 siblings, 1 reply; 32+ messages in thread
From: Liav Albani @ 2022-03-02 15:45 UTC (permalink / raw)
To: Michael S. Tsirkin, Igor Mammedov; +Cc: ani, shentey, qemu-devel
>>> 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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-03-02 15:43 ` Liav Albani
@ 2022-03-02 15:51 ` Ani Sinha
0 siblings, 0 replies; 32+ messages in thread
From: Ani Sinha @ 2022-03-02 15:51 UTC (permalink / raw)
To: Liav Albani; +Cc: Igor Mammedov, shentey, qemu-devel, Michael S. Tsirkin
[-- Attachment #1: Type: text/plain, Size: 3100 bytes --]
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.
>
>
[-- Attachment #2: Type: text/html, Size: 5250 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-03-02 12:42 ` Michael S. Tsirkin
2022-03-02 15:43 ` Liav Albani
@ 2022-03-02 15:58 ` Ani Sinha
1 sibling, 0 replies; 32+ messages in thread
From: Ani Sinha @ 2022-03-02 15:58 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Igor Mammedov, shentey, Liav Albani, qemu-devel
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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 0/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-02-28 20:17 [PATCH v4 0/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table Liav Albani
` (2 preceding siblings ...)
2022-02-28 20:17 ` [PATCH v4 3/3] tests/acpi: i386: update FACP table differences Liav Albani
@ 2022-03-04 10:26 ` Michael S. Tsirkin
2022-03-04 10:34 ` Ani Sinha
3 siblings, 1 reply; 32+ messages in thread
From: Michael S. Tsirkin @ 2022-03-04 10:26 UTC (permalink / raw)
To: Liav Albani; +Cc: ani, imammedo, shentey, qemu-devel
On Mon, Feb 28, 2022 at 10:17:30PM +0200, Liav Albani wrote:
> This can allow the guest OS to determine more easily if i8042 controller
> is present in the system or not, so it doesn't need to do probing of the
> controller, but just initialize it immediately, before enumerating the
> ACPI AML namespace.
>
> To allow "flexible" indication, I don't hardcode the bit at location 1
> as on in the IA-PC boot flags, but try to search for i8042 on the ISA
> bus to verify it exists in the system.
>
> Why this is useful you might ask - this patch allows the guest OS to
> probe and use the i8042 controller without decoding the ACPI AML blob
> at all. For example, as a developer of the SerenityOS kernel, I might
> want to allow people to not try to decode the ACPI AML namespace (for
> now, we still don't support ACPI AML as it's a work in progress), but
> still to not probe for the i8042 but just use it after looking in the
> IA-PC boot flags in the ACPI FADT table.
OK still waiting for v5.
> Liav Albani (3):
> tests/acpi: i386: allow FACP acpi table changes
> hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT
> table
> tests/acpi: i386: update FACP table differences
>
> hw/acpi/aml-build.c | 11 ++++++++++-
> hw/i386/acpi-build.c | 9 +++++++++
> hw/i386/acpi-microvm.c | 9 +++++++++
> include/hw/acpi/acpi-defs.h | 1 +
> tests/data/acpi/q35/FACP | Bin 244 -> 244 bytes
> tests/data/acpi/q35/FACP.nosmm | Bin 244 -> 244 bytes
> tests/data/acpi/q35/FACP.slic | Bin 244 -> 244 bytes
> tests/data/acpi/q35/FACP.xapic | Bin 244 -> 244 bytes
> 8 files changed, 29 insertions(+), 1 deletion(-)
>
> --
> 2.35.1
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 0/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-03-04 10:26 ` [PATCH v4 0/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table Michael S. Tsirkin
@ 2022-03-04 10:34 ` Ani Sinha
2022-03-04 10:39 ` Michael S. Tsirkin
0 siblings, 1 reply; 32+ messages in thread
From: Ani Sinha @ 2022-03-04 10:34 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: imammedo, shentey, Liav Albani, qemu-devel
On Fri, Mar 4, 2022 at 3:56 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Feb 28, 2022 at 10:17:30PM +0200, Liav Albani wrote:
> > This can allow the guest OS to determine more easily if i8042 controller
> > is present in the system or not, so it doesn't need to do probing of the
> > controller, but just initialize it immediately, before enumerating the
> > ACPI AML namespace.
> >
> > To allow "flexible" indication, I don't hardcode the bit at location 1
> > as on in the IA-PC boot flags, but try to search for i8042 on the ISA
> > bus to verify it exists in the system.
> >
> > Why this is useful you might ask - this patch allows the guest OS to
> > probe and use the i8042 controller without decoding the ACPI AML blob
> > at all. For example, as a developer of the SerenityOS kernel, I might
> > want to allow people to not try to decode the ACPI AML namespace (for
> > now, we still don't support ACPI AML as it's a work in progress), but
> > still to not probe for the i8042 but just use it after looking in the
> > IA-PC boot flags in the ACPI FADT table.
>
> OK still waiting for v5.
Since the time is tight, I could quickly make the changes in patch 2
and send it over. I believe 8th is the last date for new changes.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 0/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-03-04 10:34 ` Ani Sinha
@ 2022-03-04 10:39 ` Michael S. Tsirkin
0 siblings, 0 replies; 32+ messages in thread
From: Michael S. Tsirkin @ 2022-03-04 10:39 UTC (permalink / raw)
To: Ani Sinha; +Cc: imammedo, shentey, Liav Albani, qemu-devel
On Fri, Mar 04, 2022 at 04:04:13PM +0530, Ani Sinha wrote:
> On Fri, Mar 4, 2022 at 3:56 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Feb 28, 2022 at 10:17:30PM +0200, Liav Albani wrote:
> > > This can allow the guest OS to determine more easily if i8042 controller
> > > is present in the system or not, so it doesn't need to do probing of the
> > > controller, but just initialize it immediately, before enumerating the
> > > ACPI AML namespace.
> > >
> > > To allow "flexible" indication, I don't hardcode the bit at location 1
> > > as on in the IA-PC boot flags, but try to search for i8042 on the ISA
> > > bus to verify it exists in the system.
> > >
> > > Why this is useful you might ask - this patch allows the guest OS to
> > > probe and use the i8042 controller without decoding the ACPI AML blob
> > > at all. For example, as a developer of the SerenityOS kernel, I might
> > > want to allow people to not try to decode the ACPI AML namespace (for
> > > now, we still don't support ACPI AML as it's a work in progress), but
> > > still to not probe for the i8042 but just use it after looking in the
> > > IA-PC boot flags in the ACPI FADT table.
> >
> > OK still waiting for v5.
>
> Since the time is tight, I could quickly make the changes in patch 2
> and send it over.
Sure.
> I believe 8th is the last date for new changes.
Yes.
--
MST
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
2022-03-02 15:45 ` Liav Albani
@ 2022-03-04 10:58 ` Igor Mammedov
0 siblings, 0 replies; 32+ messages in thread
From: Igor Mammedov @ 2022-03-04 10:58 UTC (permalink / raw)
To: Liav Albani; +Cc: ani, shentey, qemu-devel, Michael S. Tsirkin
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
^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2022-03-04 10:59 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-28 20:17 [PATCH v4 0/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table Liav Albani
2022-02-28 20:17 ` [PATCH v4 1/3] tests/acpi: i386: allow FACP acpi table changes Liav Albani
2022-03-01 2:55 ` Ani Sinha
2022-02-28 20:17 ` [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table Liav Albani
2022-03-01 2:59 ` Ani Sinha
2022-03-01 8:48 ` Igor Mammedov
2022-03-01 8:43 ` Igor Mammedov
2022-03-01 9:52 ` Ani Sinha
2022-03-01 11:51 ` Igor Mammedov
2022-03-01 19:20 ` Liav Albani
2022-03-02 5:14 ` Ani Sinha
2022-03-02 12:42 ` Michael S. Tsirkin
2022-03-02 15:43 ` Liav Albani
2022-03-02 15:51 ` Ani Sinha
2022-03-02 15:58 ` Ani Sinha
2022-03-01 11:19 ` Michael S. Tsirkin
2022-03-01 11:47 ` Igor Mammedov
2022-03-01 12:18 ` Michael S. Tsirkin
2022-03-02 15:45 ` Liav Albani
2022-03-04 10:58 ` Igor Mammedov
2022-03-01 19:11 ` Liav Albani
2022-03-02 5:12 ` Ani Sinha
2022-03-02 8:47 ` Michael S. Tsirkin
2022-02-28 20:17 ` [PATCH v4 3/3] tests/acpi: i386: update FACP table differences Liav Albani
2022-03-01 2:59 ` Ani Sinha
2022-03-01 11:21 ` Michael S. Tsirkin
2022-03-01 19:13 ` Liav Albani
2022-03-02 5:05 ` Ani Sinha
2022-03-02 8:48 ` Michael S. Tsirkin
2022-03-04 10:26 ` [PATCH v4 0/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table Michael S. Tsirkin
2022-03-04 10:34 ` Ani Sinha
2022-03-04 10:39 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.