[Qemu-devel] [PATCH 00/14] tests: acpi: add UEFI (ARM) testing support

[Qemu-devel] [PATCH 00/14] tests: acpi: add UEFI (ARM) testing support

[Igor Mammedov]

Series adds support for ACPI tables located above 4G. It only adds 64-bit
handling necessary for testing arm/virt board (i.e. might be not complete
wrt spec) and a test build of UEFI (AVMF) firmware that provides an entry
point[1] for fetching ACPI tables (RSDP pointer).

Series depends on:
   [PATCH v2 0/8] tests: apci: consolidate and cleanup  ACPI test code
   it's queued on PCI tree git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git pci
and requires following EDK2 patches to enable testing feature:
   1) https://github.com/lersek/edk2/commits/acpi_test_support

Git tree for testing:
   https://github.com/imammedo/qemu.git acpi_arm_tests_v1

Igor Mammedov (14):
  tests: acpi: add uefi_find_rsdp_addr() helper
  tests: acpi: make RSDT test routine handle XSDT
  tests: acpi: rename acpi_parse_rsdp_table() into
    acpi_fetch_rsdp_table()
  tests: acpi: make pointer to RSDP 64bit
  tests: acpi: fetch X_DSDT if pointer to DSDT is 0
  tests: acpi: add reference blobs arm/virt board testcase
  tests: acpi: skip FACS table if board uses hw reduced ACPI profile
  tests: acpi: introduce an abilty start tests with UEFI firmware
  tests: acpi: move boot_sector_init() into x86 tests branch
  tests: acpi: ignore SMBIOS tests when UEFI firmware is used
  tests: acpi: add AVMF firmware blobs
  tests: acpi: prepare AVMF firmware blobs to be used by
    bios-tables-test
  tests: acpi: add simple arm/virt testcase
  tests: acpi: refactor rebuild-expected-aml.sh to dump ACPI tables for
    a specified list of targets

 tests/acpi-utils.h                      |   4 +-
 Makefile                                |   3 +-
 pc-bios/avmf.img                        | Bin 0 -> 2097152 bytes
 pc-bios/avmf_vars.img                   | Bin 0 -> 786432 bytes
 tests/Makefile.include                  |  19 +++++-
 tests/acpi-utils.c                      |  57 ++++++++++++----
 tests/bios-tables-test.c                | 113 +++++++++++++++++++++++---------
 tests/data/acpi/rebuild-expected-aml.sh |  23 ++++---
 tests/data/acpi/virt/APIC               | Bin 0 -> 168 bytes
 tests/data/acpi/virt/DSDT               | Bin 0 -> 18476 bytes
 tests/data/acpi/virt/FACP               | Bin 0 -> 268 bytes
 tests/data/acpi/virt/GTDT               | Bin 0 -> 96 bytes
 tests/data/acpi/virt/MCFG               | Bin 0 -> 60 bytes
 tests/data/acpi/virt/SPCR               | Bin 0 -> 80 bytes
 tests/vmgenid-test.c                    |   2 +-
 15 files changed, 160 insertions(+), 61 deletions(-)
 create mode 100644 pc-bios/avmf.img
 create mode 100644 pc-bios/avmf_vars.img
 create mode 100644 tests/data/acpi/virt/APIC
 create mode 100644 tests/data/acpi/virt/DSDT
 create mode 100644 tests/data/acpi/virt/FACP
 create mode 100644 tests/data/acpi/virt/GTDT
 create mode 100644 tests/data/acpi/virt/MCFG
 create mode 100644 tests/data/acpi/virt/SPCR

-- 
2.7.4

[Qemu-devel] [PATCH 01/14] tests: acpi: add uefi_find_rsdp_addr() helper

[Igor Mammedov]

introduce UEFI specific counterpart to acpi_find_rsdp_address()
that will help to find RSDP address when [OA]VMF is used as
firmware. It requires a [OA]VMF built with PcdAcpiTestSupport=TRUE,
to locate RSDP address within 1Mb aligned ACPI test structure, tagged
with GUID AB87A6B1-2034-BDA0-71BD-375007757785

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/acpi-utils.h |  1 +
 tests/acpi-utils.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
index ef388bb..3b11f47 100644
--- a/tests/acpi-utils.h
+++ b/tests/acpi-utils.h
@@ -46,6 +46,7 @@ typedef struct {
 
 uint8_t acpi_calc_checksum(const uint8_t *data, int len);
 uint32_t acpi_find_rsdp_address(QTestState *qts);
+uint64_t uefi_find_rsdp_addr(QTestState *qts, uint64_t start, uint64_t size);
 uint64_t acpi_get_xsdt_address(uint8_t *rsdp_table);
 void acpi_parse_rsdp_table(QTestState *qts, uint32_t addr, uint8_t *rsdp_table);
 void acpi_fetch_table(QTestState *qts, uint8_t **aml, uint32_t *aml_len,
diff --git a/tests/acpi-utils.c b/tests/acpi-utils.c
index cc33b46..b9ff9df 100644
--- a/tests/acpi-utils.c
+++ b/tests/acpi-utils.c
@@ -111,3 +111,46 @@ void acpi_fetch_table(QTestState *qts, uint8_t **aml, uint32_t *aml_len,
         g_assert(!acpi_calc_checksum(*aml, *aml_len));
     }
 }
+
+#define GUID_SIZE 16
+static uint8_t AcpiTestSupportGuid[GUID_SIZE] =
+     { 0xb1, 0xa6, 0x87, 0xab,
+       0x34, 0x20,
+       0xa0, 0xbd,
+       0x71, 0xbd, 0x37, 0x50, 0x07, 0x75, 0x77, 0x85 };
+
+typedef struct {
+    uint8_t signature_guid[16];
+    uint64_t rsdp10;
+    uint64_t rsdp20;
+} __attribute__((packed)) UefiTestSupport;
+
+/* Wait at most 600 seconds (test is slow with TCI and --enable-debug) */
+#define TEST_DELAY (1 * G_USEC_PER_SEC / 10)
+#define TEST_CYCLES MAX((600 * G_USEC_PER_SEC / TEST_DELAY), 1)
+#define MB 0x100000ULL
+uint64_t uefi_find_rsdp_addr(QTestState *qts, uint64_t start, uint64_t size)
+{
+    int i, j;
+    uint8_t data[GUID_SIZE];
+
+    for (i = 0; i < TEST_CYCLES; ++i) {
+        for (j = 0; j < size / MB; j++) {
+            /* look for GUID at every 1Mb block */
+            uint64_t addr = start + j * MB;
+
+            qtest_memread(qts, addr, data, sizeof(data));
+            if (!memcmp(AcpiTestSupportGuid, data, sizeof(data))) {
+                UefiTestSupport ret;
+
+                qtest_memread(qts, addr, &ret, sizeof(ret));
+                ret.rsdp10 = le64_to_cpu(ret.rsdp10);
+                ret.rsdp20 = le64_to_cpu(ret.rsdp20);
+                return ret.rsdp20 ? ret.rsdp20 : ret.rsdp10;
+            }
+        }
+        g_usleep(TEST_DELAY);
+    }
+    g_assert_not_reached();
+    return 0;
+}
-- 
2.7.4

[Qemu-devel] [PATCH 02/14] tests: acpi: make RSDT test routine handle XSDT

[Igor Mammedov]

If RSDP revision is more than 0 fetch table pointed by XSDT
and fallback to legacy RSDT table otherwise.

While at it drop unused acpi_get_xsdt_address().

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
PS:
 it doesn't affect existing pc/q35 machines as they use RSDP.revision == 0
 but it will be used by followup patch to enable testing arm/virt
 board which uses provides XSDT table.
---
 tests/acpi-utils.h       |  3 +--
 tests/acpi-utils.c       | 14 +-------------
 tests/bios-tables-test.c | 18 +++++++++++++-----
 3 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
index 3b11f47..398900c 100644
--- a/tests/acpi-utils.h
+++ b/tests/acpi-utils.h
@@ -47,8 +47,7 @@ typedef struct {
 uint8_t acpi_calc_checksum(const uint8_t *data, int len);
 uint32_t acpi_find_rsdp_address(QTestState *qts);
 uint64_t uefi_find_rsdp_addr(QTestState *qts, uint64_t start, uint64_t size);
-uint64_t acpi_get_xsdt_address(uint8_t *rsdp_table);
-void acpi_parse_rsdp_table(QTestState *qts, uint32_t addr, uint8_t *rsdp_table);
+void acpi_parse_rsdp_table(QTestState *qts, uint64_t addr, uint8_t *rsdp_table);
 void acpi_fetch_table(QTestState *qts, uint8_t **aml, uint32_t *aml_len,
                       const uint8_t *addr_ptr, const char *sig,
                       bool verify_checksum);
diff --git a/tests/acpi-utils.c b/tests/acpi-utils.c
index b9ff9df..84068a8 100644
--- a/tests/acpi-utils.c
+++ b/tests/acpi-utils.c
@@ -51,19 +51,7 @@ uint32_t acpi_find_rsdp_address(QTestState *qts)
     return off;
 }
 
-uint64_t acpi_get_xsdt_address(uint8_t *rsdp_table)
-{
-    uint64_t xsdt_physical_address;
-    uint8_t revision = rsdp_table[15 /* Revision offset */];
-
-    /* We must have revision 2 if we're looking for an XSDT pointer */
-    g_assert(revision == 2);
-
-    memcpy(&xsdt_physical_address, &rsdp_table[24 /* XsdtAddress offset */], 8);
-    return le64_to_cpu(xsdt_physical_address);
-}
-
-void acpi_parse_rsdp_table(QTestState *qts, uint32_t addr, uint8_t *rsdp_table)
+void acpi_parse_rsdp_table(QTestState *qts, uint64_t addr, uint8_t *rsdp_table)
 {
     uint8_t revision;
 
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 0bf7164..529bfc4 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -107,17 +107,25 @@ static void test_acpi_rsdp_table(test_data *data)
     }
 }
 
-static void test_acpi_rsdt_table(test_data *data)
+static void test_acpi_rxsdt_table(test_data *data)
 {
+    const char *sig = "RSDT";
     AcpiSdtTable rsdt = {};
+    int entry_size = 4;
+    int addr_off = 16 /* RsdtAddress */;
     uint8_t *ent;
 
-    /* read RSDT table */
+    if (data->rsdp_table[15 /* Revision offset */] != 0) {
+        addr_off = 24 /* XsdtAddress */;
+        entry_size = 8;
+        sig = "XSDT";
+    }
+    /* read [RX]SDT table */
     acpi_fetch_table(data->qts, &rsdt.aml, &rsdt.aml_len,
-                     &data->rsdp_table[16 /* RsdtAddress */], "RSDT", true);
+                     &data->rsdp_table[addr_off], sig, true);
 
     /* Load all tables and add to test list directly RSDT referenced tables */
-    ACPI_FOREACH_RSDT_ENTRY(rsdt.aml, rsdt.aml_len, ent, 4 /* Entry size */) {
+    ACPI_FOREACH_RSDT_ENTRY(rsdt.aml, rsdt.aml_len, ent, entry_size) {
         AcpiSdtTable ssdt_table = {};
 
         acpi_fetch_table(data->qts, &ssdt_table.aml, &ssdt_table.aml_len, ent,
@@ -519,7 +527,7 @@ static void test_acpi_one(const char *params, test_data *data)
     data->tables = g_array_new(false, true, sizeof(AcpiSdtTable));
     test_acpi_rsdp_address(data);
     test_acpi_rsdp_table(data);
-    test_acpi_rsdt_table(data);
+    test_acpi_rxsdt_table(data);
     test_acpi_fadt_table(data);
 
     if (iasl) {
-- 
2.7.4

[Qemu-devel] [PATCH 03/14] tests: acpi: rename acpi_parse_rsdp_table() into acpi_fetch_rsdp_table()

[Igor Mammedov]

so name would reflect what the function does

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/acpi-utils.h       | 2 +-
 tests/acpi-utils.c       | 2 +-
 tests/bios-tables-test.c | 2 +-
 tests/vmgenid-test.c     | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
index 398900c..59fb7d5 100644
--- a/tests/acpi-utils.h
+++ b/tests/acpi-utils.h
@@ -47,7 +47,7 @@ typedef struct {
 uint8_t acpi_calc_checksum(const uint8_t *data, int len);
 uint32_t acpi_find_rsdp_address(QTestState *qts);
 uint64_t uefi_find_rsdp_addr(QTestState *qts, uint64_t start, uint64_t size);
-void acpi_parse_rsdp_table(QTestState *qts, uint64_t addr, uint8_t *rsdp_table);
+void acpi_fetch_rsdp_table(QTestState *qts, uint64_t addr, uint8_t *rsdp_table);
 void acpi_fetch_table(QTestState *qts, uint8_t **aml, uint32_t *aml_len,
                       const uint8_t *addr_ptr, const char *sig,
                       bool verify_checksum);
diff --git a/tests/acpi-utils.c b/tests/acpi-utils.c
index 84068a8..dce0fb7 100644
--- a/tests/acpi-utils.c
+++ b/tests/acpi-utils.c
@@ -51,7 +51,7 @@ uint32_t acpi_find_rsdp_address(QTestState *qts)
     return off;
 }
 
-void acpi_parse_rsdp_table(QTestState *qts, uint64_t addr, uint8_t *rsdp_table)
+void acpi_fetch_rsdp_table(QTestState *qts, uint64_t addr, uint8_t *rsdp_table)
 {
     uint8_t revision;
 
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 529bfc4..99d7bf8 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -89,7 +89,7 @@ static void test_acpi_rsdp_table(test_data *data)
     uint8_t *rsdp_table = data->rsdp_table, revision;
     uint32_t addr = data->rsdp_addr;
 
-    acpi_parse_rsdp_table(data->qts, addr, rsdp_table);
+    acpi_fetch_rsdp_table(data->qts, addr, rsdp_table);
     revision = rsdp_table[15 /* Revision offset */];
 
     switch (revision) {
diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
index 52cdd83..27153a0 100644
--- a/tests/vmgenid-test.c
+++ b/tests/vmgenid-test.c
@@ -40,7 +40,7 @@ static uint32_t acpi_find_vgia(QTestState *qts)
     g_assert_cmphex(rsdp_offset, <, RSDP_ADDR_INVALID);
 
 
-    acpi_parse_rsdp_table(qts, rsdp_offset, rsdp_table);
+    acpi_fetch_rsdp_table(qts, rsdp_offset, rsdp_table);
     acpi_fetch_table(qts, &rsdt, &rsdt_len, &rsdp_table[16 /* RsdtAddress */],
                      "RSDT", true);
 
-- 
2.7.4

[Qemu-devel] [PATCH 04/14] tests: acpi: make pointer to RSDP 64bit

[Igor Mammedov]

In case of UEFI RSDP doesn't have to be located in lowmem,
it could be placed at any address. Make sure that test won't
break if it is placed above the first 4Gb of address space.

PS:
While at it cleanup some local variables as we don't really
need them.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/bios-tables-test.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 99d7bf8..c28c5c7 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -26,7 +26,7 @@
 typedef struct {
     const char *machine;
     const char *variant;
-    uint32_t rsdp_addr;
+    uint64_t rsdp_addr;
     uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
     GArray *tables;
     uint32_t smbios_ep_addr;
@@ -86,13 +86,11 @@ static void test_acpi_rsdp_address(test_data *data)
 
 static void test_acpi_rsdp_table(test_data *data)
 {
-    uint8_t *rsdp_table = data->rsdp_table, revision;
-    uint32_t addr = data->rsdp_addr;
+    uint8_t *rsdp_table = data->rsdp_table;
 
-    acpi_fetch_rsdp_table(data->qts, addr, rsdp_table);
-    revision = rsdp_table[15 /* Revision offset */];
+    acpi_fetch_rsdp_table(data->qts, data->rsdp_addr, rsdp_table);
 
-    switch (revision) {
+    switch (rsdp_table[15 /* Revision offset */]) {
     case 0: /* ACPI 1.0 RSDP */
         /* With rev 1, checksum is only for the first 20 bytes */
         g_assert(!acpi_calc_checksum(rsdp_table,  20));
-- 
2.7.4

[Qemu-devel] [PATCH 05/14] tests: acpi: fetch X_DSDT if pointer to DSDT is 0

[Igor Mammedov]

that way it would be possible to test a DSDT pointed by
64bit X_DSDT field in FADT.

PS:
it will allow to enable testing arm/virt board, which sets
only newer X_DSDT field.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/bios-tables-test.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index c28c5c7..0f04a0a 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -140,6 +140,8 @@ static void test_acpi_fadt_table(test_data *data)
     AcpiSdtTable table = g_array_index(data->tables, typeof(table), 0);
     uint8_t *fadt_aml = table.aml;
     uint32_t fadt_len = table.aml_len;
+    uint32_t val;
+    int dsdt_offset = 40 /* DSDT */;
 
     g_assert(compare_signature(&table, "FACP"));
 
@@ -148,8 +150,12 @@ static void test_acpi_fadt_table(test_data *data)
                      fadt_aml + 36 /* FIRMWARE_CTRL */, "FACS", false);
     g_array_append_val(data->tables, table);
 
+    memcpy(&val, fadt_aml + dsdt_offset, 4);
+    if (!val) {
+        dsdt_offset = 140 /* X_DSDT */;
+    }
     acpi_fetch_table(data->qts, &table.aml, &table.aml_len,
-                     fadt_aml + 40 /* DSDT */, "DSDT", true);
+                     fadt_aml + dsdt_offset, "DSDT", true);
     g_array_append_val(data->tables, table);
 
     memset(fadt_aml + 36, 0, 4); /* sanitize FIRMWARE_CTRL ptr */
-- 
2.7.4

[Qemu-devel] [PATCH 06/14] tests: acpi: add reference blobs arm/virt board testcase

[Igor Mammedov]

adds test blobs for default arm/virt machine testcase

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/data/acpi/virt/APIC | Bin 0 -> 168 bytes
 tests/data/acpi/virt/DSDT | Bin 0 -> 18476 bytes
 tests/data/acpi/virt/FACP | Bin 0 -> 268 bytes
 tests/data/acpi/virt/GTDT | Bin 0 -> 96 bytes
 tests/data/acpi/virt/MCFG | Bin 0 -> 60 bytes
 tests/data/acpi/virt/SPCR | Bin 0 -> 80 bytes
 6 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 tests/data/acpi/virt/APIC
 create mode 100644 tests/data/acpi/virt/DSDT
 create mode 100644 tests/data/acpi/virt/FACP
 create mode 100644 tests/data/acpi/virt/GTDT
 create mode 100644 tests/data/acpi/virt/MCFG
 create mode 100644 tests/data/acpi/virt/SPCR

diff --git a/tests/data/acpi/virt/APIC b/tests/data/acpi/virt/APIC
new file mode 100644
index 0000000000000000000000000000000000000000..797dfde2841c51b7e72065602e99ce1714347f0d
GIT binary patch
literal 168
zcmZ<^@N{0mz`($~*~#D8BUr&HBEZ=ZD8>jB1F=Cg4Dd+6SPUF6788)c?E~X6Fu>G{
hBZPn~MyPrgD9sGlkD?67;f3451Xcqw&w(L;0RYV=2>}2A

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
new file mode 100644
index 0000000000000000000000000000000000000000..20e85c7f89f645c69935c615c07084e221419960
GIT binary patch
literal 18476
zcmc)ScU)EVAII^7km3at6myGa+R|dUS|(gjDG>rqi;BuJN5jfYEla7?tSq%Xt!(eT
zr!AYd_uhN&!S8+U(D&=t`StIw9$oH>d%mB0pK#D~&LeJRL*=*uql2K;?26j>=!V`E
z6YJuY`dmg31mXSgWB#J~S-UqiR5Ud<cZ(Mn7iTw(uCB~0kJnWzh6dS9<Etx!#^$Q5
zcx_Gk!TOrf#l<BhsRy&0;`I#$-C~^=whh9GZG-$EIH7frk<mvrM_ZLw*5`%~G&Yxv
z9Mh1RGG=Ujt)>jdl!92h)D&$WWX;hthf7M5uZl}Dl25#TNmhEvu#pquBa=&ZuBsU?
zNU5HsVO)7EM{DBc|GlzR+b&ufK3RFzF7@fJLGsy(?FFt|xgHw}TBWeXJ_0W|JtBPC
ze~f4qtRGR58c`9xic&YHN5oo1&B(GDr9Pu9az<v<jMg@z%x4UWoRJkZBim*S@)?68
zXKWBPBW5!O`-~xxGg?Q@$h8?me8$ko8Ev9ww6hsQea5iJ8QD=Y@@>X2pRsl1jJ8oT
zI@pY@ZAJ-xjMnWRv8Wk^He(x~5xJZ4ha|nLZ)h{N^%>FoX>Qc(=wdUr^BK|mX<pQf
zZZ>0kpAo&Ewu_q4!)6Tk8PWS``=}YcY{m{gBYHp0kDAfjW{mI|(fesZ)QpX6M!C<3
z-cLJ3&DhvxRQQbO{j_7$jQ%zwZZk@w_tV0t8JpUSN}mzEpLU9xvANBt@)^<l>4s4=
z2H1>|J|lWR?Hn~@OPevuXGHI(U7}_b+YJ3Rp7<Lo{JWtvdOz(NHKWvKRQrtR{j^)u
zj6pVIN1qYBpLUO$F~nx<<TIl8(;iVXhS`iUJ|lWR?HM&=8=FyMGs>d((_T?CwzC;y
zeMa<tS`;;7xXq|d%~<euW?j5G^+M{#))ki*57U85TnA*yDhm%|sz&LyqGGIWbzr4i
z9iiog>%s@e)fW`SdejB+pgPzu=p7X6ze?Sk6-*5#>0_Xck_RDm_2W7&y)ZK;$m)=j
zmDAD^jB3z`<oyiYF9|y2hM$kMQk146Q&ARl$ji!YX~_t}HQFv!;VNy|F8Nquoi<Hp
zxKi^I+v=DpxoxV#mFZ1&KomZsHchNlhAY$0l9^bUeg&C9xH7$IW^$!AO{`RgEA@&J
z&!VHl<hH3w855i804k+Sr#m-*bA!ZlrkxukSEgUQ(w(j0)FH86L3&diaJf_I&Ngti
zfwK*rN_S?%nVndf{*veb&7DejwuMs%^U1GX;!Dz&PNh3zaK_-&0i8RQ?#zKR2hJQi
zmG0CTg?&GB;nV@3JC*LtgEJ4#JUW%`YzJpMICa42PNh5B!`U9r_H-)Usgo&l=EJE2
zOm`~XSpa7NoCS0$-Pr-o4shy#)SXIqc7(GdoE_;@y0Z|@LO68*>rSORJHgor&Q5eH
z-MJy08^WmrUUw?p*%{8xaCW9s=}w($+V`^yoH{^ur_!BW;p_@$S2~sM>;`8yICVhn
zPNh4$!`U6q?sO{M*#piVaOwcuol1B1gtI4{J?T`svlpDb;M4)TJC*J%g0l$DB081s
z><wpcICX&UPNh5hz}W}RK6EPGxe=Tj!Ko7hcPic47tX$L_N7zl&W+*R7*3r)xKruQ
zesK1Kvmc#GclL*~Kb$(@aHrCpo4~mVoSV?8bmyjUZVIPPP~53>=Vowj2IppUD&4s`
zoSVa`6B>6a-MIyvTfn&mol18OfO7zxIstO0(wzh090=z?I+gC+63#8*)CrS2mG0aM
z&aL3wicY0FbzRQ>ye)=PCs^)Ox>J|qv@(6<SpsJXol1A=BAiyHk337^)Crk8mG0D~
zH?2$`-<H8yMyJx9y5MHcL2&8>&Yenk>N1--2g5m-PNh3_am}1V;M579JC*L#B{g#n
zg>xvKN_Xl)nmLETsS`wZD&48eXXe}*&aLTGx>Fa;%()GmI-zu@(w(|gX3lNl+?GzI
zJ9UA~oZG>v6Hs?5-KooB=G-36?depyQy0U`IUG)%u)0&}PF(^s=MHf0K&R53Bj6kX
z=LkBL?ktD19L{n&mG0C<E^}7ESwW}Low~GT&N!TLI+gC!1ud;ip8;3GSxKkTow|&r
zmFYi+s^F}mQ|V4!yfWuVI7iZ{bf+#^nR67Jqv%w+Qx~etIU3H<bSmAc%Twm8hO?SZ
zr8{*|%A7mGxg(uQcj{7<Id_6{Cpwkx)CDMWj)8Lwol1A=vXeP$;H;rj=}uj2GUr%0
z$I_{Er!Fy>vlh-;I+gCMgR>6KIy#l^+!@ZD;oO-{r9124tcSCnPNh2=;B0`iflj46
z$H6%c&T(`q-8ml4@o<i(Q|ZoK;M@hyUFcN0a{`<b;G96G(w!6GoCxPcI+gC+70zAZ
z+?7tHJ9mR~H#m2rQ|Zn|I2++?q*Lk6-QnCF&fV!$x^oXW_keQ`I+gC+6V5&1+>=hF
zJDcEag0qQEr91b6b1yjeqEqS4z2V#&&b{eWx^o{m_knXCI+gC61m`3;C()^N=e}_6
z3+KLcD&488-uCD1esJzbr_!C1;hYTTWIC1Z+#k;U;oP52r8}p<IR(xsbSm9B70#(}
zPNh@n&S`K?gL4|4N_S3&b2^;U=~TM&05}hT^8h-P?mQ6A1K~W7PNh2!g7Y9a5291)
z&V%7R7|w&~RJwBpoHO8@L8sE4hroFVoQKe<bmyUP9t!87bSm9>7@UW}c^I8acg}=!
zCY&?rRJ!wUI1h*Oa5|OloCW7BIA_tRbmtLp9s%bObSm9B8_wBq&Zbl8&N*<-fpZR>
zN_WnMb1s~7=~TM&NH~v#^GG_C?mP<4qu@M>PNh5N!8s4kd2}k>c{H3y!+A8FN_QRu
z=P_^|L#NW6^WmHi=X^Sq?py%p0yr1YsdQ&EoXv1H)2Vdlv2Y#>=dpAu-FY0G$H93V
zol18e59jf49#5yzoeSYy2<JjNmF`>w=OQ>4(W!LjVmKGWxtLC+J5PY~1UOHiQ|Znn
za4vy!37txJo(SiOaGpq~(w!&4c@mr_(W!Lj$#9+w=gD*`-MJLbrEo5#Q|ZoA;5-G+
zQ|MH>^Hexbh4WN8mF`>y=Q22#(W!LjayXa6xtva=J6FKD0?rk5D&2V+oTtHg8l6ga
zo(|{faGp-5(w%3(c?O(k(5ZCinQ)#7=b3aW-FX(AXTf<Eol19}4d>Z#o=vCHo#()L
z4xH!EsdVSLaGne2xpXSsc^;hS!Fe8?N_U<Q=lO7+Pp8tI7r=P|oEOlkbmxU|UI^!f
zbSmAs63&%yuB21x&Wqr@2+oV>RJ!wGI4_3tVmg)Xyadim;Jk!Rr8`%_xeCrzbSm9>
zDV&$Wc`2PrcU}hPWpG|br_!C5!+ANJm(!_q=M`{X0p}HTD&2V{oL9nmC7nulUIph>
za9%~H(w$esc{Q9@)2VdlHE>=7=QVUH-FYpX*TQ)%ol19J2j_KgUPq_Wo!7&8J)GCm
zsdVQJaNYpt4Rk8qc_W-R!g(W|N_XA_=S^_lM5of7H^X@|oHx^{bmuK_-U8<>bSm9>
zE1b8&c`Kbtcisl)ZE)U3r_!Cb!+ATLx6`R~=N)j~0p}fbD&2V}oOi-`C!I=n-Ua7f
zaNb3y(w%q1c{iMQ)2VdlJ#gLw=RI^P-FYvZ_riHEol1A!2j_io-bbg>o%h3eKb-f|
zsdVQ9a6SO%19U3g`5>GR!ucSbN_Rd4=R<HlM5of7tKnP?=W05Y?tB=|hv9sfPNh2^
zf%6eKAE8s}&PU;V6wXKKRJ!vqI3I)aF*=p*d>qcl;e4D<r90QaxdzTPbSmBX1e{O6
z`2?LxcRmT{lW;ysr_!BI!TA)NPtmD#=hJXL4d>HzD&6@EoX^1d44q1MJ`3lwa6U_?
z(w%GJTnpz~I+gBx4$kM`e2z|~JD-R1c{rb^Q|ZnZ;Cunj7wA;F^F=scg!4r@mF|2A
z&X?ePiB6?EUxxE#IA5ky>CRW+d<D)|=v2D%RXAUT^Hn;P?tBf-*Wi4OPNh3vhx2tf
zU#C;)&NtwE1I{<-RJ!v`INyZxO*)nCd<)LE;Czctr90n-^KCfarc>$8ci?;n&Uffk
zy7OH)--YvCI+gBx56<`Ce2-40JKu-%eK_BzQ|ZnR;QRp259n07^Fug4g!4l>mG1lq
z&X3^yh)$(DKZf&TI6tOS>CR8!`~=QV=v2D%Q#e0`^HVyN?)(hS&*1!wPNh3Phx2nd
zKc`da&M)Bn0?se!RJ!v^IKPDROFEVA{0h#m;QWeCr8~cd^J_T2rc>$8Z{Yj}&Tr^c
zy7OB&zlHN#I+gDH4$kl3{EkkgJHLnXdpN(RQ|Znh;QRs3ALvxN^G7&;g!4x_mG1lr
z&Y$4?iB6?Ee}?mCIDe*7>CRu^`~}Wm=v2D%S2%x#^H(~R?)(kT-{Aa>PNh44hx2zh
zf2ULF&OhM%1I|C_RJ!v|IRAw6Pdb(E{0q*%;QWhDr91zI^KUr+rc>$87C2krY@t(`
zbT&3uXX$^8vEMh17mrN-KB;c&^rjx|VmXO7^5`2R-^e3;qYr+ruys>IeM3fSRO<I%
z!(UeYU!yjT7?u1SN2PvU``<?Oix`#s97m;oYy00u^^X{p{9I~OVSZD*qC8mDP8;Tr
z&`n`&`|y2Fg6#T=@goaHw~5VMoENmp)gwWmZ$=PgEb1Htxf$VI{gdC)^7ruM-Igu&
zNJegEvb1$#^A<gt5iHrl)+9EVuiKXJpY-ObkKyO%1grjU&z#*bzOF9Fj*qJ!6BLeZ
z+f>^S&ss7)h*wT1Svj`NiYWyhWBH9WZ<PH~)MLaM6K0k_u>C8OmrRdkX@gRI%+|-U
z8DWHT!aHT*s2N9wx3Qi<_+e#-<twIU%$TPKOJdo{(Os6WShsERf&9b+Gr|SoBdexg
zl%HCXm3U;;^umEnl?^pnEBhp0)!LzJK57^|w`N)A&uhA_j@PVgyJDKYkeL}7;f>w|
zCa1oxxGDL|)s4+HS@l)vx2#F-LE(GJgg*#nvEqMxyAr#GzF9>hQs1W3hy3tk_y#kh
z+l;EKP5s1`C*DVANccWF>wb|tH9P&T8$ss!chlJ<F+$r`RTuP)^**V)_Lt<pdO-VH
q*Pxf~WCr0A=(!5>nyQM+f`xSx>MLUN8=H&5JIVJQNjl<q-v0n?ptJx0

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/virt/FACP b/tests/data/acpi/virt/FACP
new file mode 100644
index 0000000000000000000000000000000000000000..27de99f51bfe846b1f8796ace49d83f5b33a1aed
GIT binary patch
literal 268
ycmZ>BbPnKQWME+3?d0$55v<@85#a0w6axw|fY>0Kx<CNcIA#XwTY+i=(L4a*H3tCz

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/virt/GTDT b/tests/data/acpi/virt/GTDT
new file mode 100644
index 0000000000000000000000000000000000000000..10107a65e958ff6495bb8c17d63d0539690f59f6
GIT binary patch
literal 96
zcmZ<{aS2IaU|?Xn>E!S15v<@85#a0&6k`O6f!H7#8OTC8azL5|h^3)?DJYFj0RVOU
B2mt^9

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/virt/MCFG b/tests/data/acpi/virt/MCFG
new file mode 100644
index 0000000000000000000000000000000000000000..e8987e1af0ec3829770bf4fe11fab02b06160dd2
GIT binary patch
literal 60
scmeZuc5}C3U|?YMck*}k2v%^42ypfViZKGkKx`0=1Oyx)oc|yS05YNo0RR91

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/virt/SPCR b/tests/data/acpi/virt/SPCR
new file mode 100644
index 0000000000000000000000000000000000000000..377271a0e7817cc21a28c02123a89facad63604f
GIT binary patch
literal 80
zcmWFza1IJ!U|?VpcJg=j2v%^42yhMtiZKGkKx`1r48#l^3?L>agsBLmm>C$E7#RKo
I0Z0r60QF4^0RR91

literal 0
HcmV?d00001

-- 
2.7.4

[Qemu-devel] [PATCH 07/14] tests: acpi: skip FACS table if board uses hw reduced ACPI profile

[Igor Mammedov]

If FADT has HW_REDUCED_ACPI flag set, do not attempt to fetch
FACS as it's not provided by the board.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/bios-tables-test.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 0f04a0a..8887319 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -146,9 +146,13 @@ static void test_acpi_fadt_table(test_data *data)
     g_assert(compare_signature(&table, "FACP"));
 
     /* Since DSDT/FACS isn't in RSDT, add them to ASL test list manually */
-    acpi_fetch_table(data->qts, &table.aml, &table.aml_len,
-                     fadt_aml + 36 /* FIRMWARE_CTRL */, "FACS", false);
-    g_array_append_val(data->tables, table);
+    memcpy(&val, fadt_aml + 112 /* Flags */, 4);
+    val = le32_to_cpu(val);
+    if (!(val & 1UL << 20 /* HW_REDUCED_ACPI */)) {
+        acpi_fetch_table(data->qts, &table.aml, &table.aml_len,
+                         fadt_aml + 36 /* FIRMWARE_CTRL */, "FACS", false);
+        g_array_append_val(data->tables, table);
+    }
 
     memcpy(&val, fadt_aml + dsdt_offset, 4);
     if (!val) {
-- 
2.7.4

[Qemu-devel] [PATCH 08/14] tests: acpi: introduce an abilty start tests with UEFI firmware

[Igor Mammedov]

For testcase to use UEFI firmware, one needs to provide and specify
firmware and varstore blobs names in test_data { uefi_fl1, uefi_fl2) }
fields respectively.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/bios-tables-test.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 8887319..d290dd2 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -26,6 +26,8 @@
 typedef struct {
     const char *machine;
     const char *variant;
+    const char *uefi_fl1;
+    const char *uefi_fl2;
     uint64_t rsdp_addr;
     uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
     GArray *tables;
@@ -519,21 +521,35 @@ static void test_smbios_structs(test_data *data)
 static void test_acpi_one(const char *params, test_data *data)
 {
     char *args;
-
-    /* Disable kernel irqchip to be able to override apic irq0. */
-    args = g_strdup_printf("-machine %s,accel=%s,kernel-irqchip=off "
-                           "-net none -display none %s "
-                           "-drive id=hd0,if=none,file=%s,format=raw "
-                           "-device ide-hd,drive=hd0 ",
-                           data->machine, "kvm:tcg",
-                           params ? params : "", disk);
+    bool use_uefi = data->uefi_fl1 && data->uefi_fl2;
+
+    if (use_uefi) {
+        args = g_strdup_printf("-machine %s,accel=%s -nodefaults -nographic "
+            "-drive if=pflash,format=raw,file=%s/%s,readonly "
+            "-drive if=pflash,format=raw,file=%s/%s,snapshot=on %s",
+            data->machine, "kvm:tcg", data_dir, data->uefi_fl1, data_dir,
+            data->uefi_fl2, params ? params : "");
+
+    } else {
+        /* Disable kernel irqchip to be able to override apic irq0. */
+        args = g_strdup_printf("-machine %s,accel=%s,kernel-irqchip=off "
+            "-net none -display none %s "
+            "-drive id=hd0,if=none,file=%s,format=raw "
+            "-device ide-hd,drive=hd0 ",
+             data->machine, "kvm:tcg", params ? params : "", disk);
+    }
 
     data->qts = qtest_init(args);
 
-    boot_sector_test(data->qts);
+    if (use_uefi) {
+        data->rsdp_addr = uefi_find_rsdp_addr(data->qts,
+            0x40000000ULL, 128ULL * 1024 * 1024);
+    } else {
+        boot_sector_test(data->qts);
+        test_acpi_rsdp_address(data);
+    }
 
     data->tables = g_array_new(false, true, sizeof(AcpiSdtTable));
-    test_acpi_rsdp_address(data);
     test_acpi_rsdp_table(data);
     test_acpi_rxsdt_table(data);
     test_acpi_fadt_table(data);
-- 
2.7.4

[Qemu-devel] [PATCH 09/14] tests: acpi: move boot_sector_init() into x86 tests branch

[Igor Mammedov]

boot_sector_init() won't be used by arm/virt board, so move it from
global scope to x86 branch that uses it.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/bios-tables-test.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index d290dd2..d9efe59 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -799,13 +799,13 @@ int main(int argc, char *argv[])
     const char *arch = qtest_get_arch();
     int ret;
 
-    ret = boot_sector_init(disk);
-    if(ret)
-        return ret;
-
     g_test_init(&argc, &argv, NULL);
 
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
+        ret = boot_sector_init(disk);
+        if(ret)
+           return ret;
+
         qtest_add_func("acpi/piix4", test_acpi_piix4_tcg);
         qtest_add_func("acpi/piix4/bridge", test_acpi_piix4_tcg_bridge);
         qtest_add_func("acpi/q35", test_acpi_q35_tcg);
-- 
2.7.4

[Qemu-devel] [PATCH 10/14] tests: acpi: ignore SMBIOS tests when UEFI firmware is used

[Igor Mammedov]

once FW provides a pointer to SMBIOS entry point like it does for
RSDP it should be possible to enable this one the same way.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/bios-tables-test.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index d9efe59..a64d0c2 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -562,8 +562,11 @@ static void test_acpi_one(const char *params, test_data *data)
         }
     }
 
-    test_smbios_entry_point(data);
-    test_smbios_structs(data);
+    /* TODO: make SMBIOS tests work with UEFI firmware */
+    if (!use_uefi) {
+        test_smbios_entry_point(data);
+        test_smbios_structs(data);
+    }
 
     assert(!global_qtest);
     qtest_quit(data->qts);
-- 
2.7.4

[Qemu-devel] [PATCH 12/14] tests: acpi: prepare AVMF firmware blobs to be used by bios-tables-test

[Igor Mammedov]

Copy blobs in ACPI test data directory and pad them up to 64Mb
so that QEMU run by test could use them.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/Makefile.include | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index aa68eb5..e1201d9 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -295,6 +295,7 @@ check-qtest-aarch64-y = tests/numa-test$(EXESUF)
 check-qtest-aarch64-$(CONFIG_SDHCI) += tests/sdhci-test$(EXESUF)
 check-qtest-aarch64-y += tests/boot-serial-test$(EXESUF)
 check-qtest-aarch64-y += tests/migration-test$(EXESUF)
+qtest-uefi-images-aarch64 = avmf.img avmf_vars.img
 
 check-qtest-microblazeel-y += $(check-qtest-microblaze-y)
 
@@ -710,7 +711,8 @@ tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o
 tests/boot-order-test$(EXESUF): tests/boot-order-test.o $(libqos-obj-y)
 tests/boot-serial-test$(EXESUF): tests/boot-serial-test.o $(libqos-obj-y)
 tests/bios-tables-test$(EXESUF): tests/bios-tables-test.o \
-	tests/boot-sector.o tests/acpi-utils.o $(libqos-obj-y)
+	tests/boot-sector.o tests/acpi-utils.o $(libqos-obj-y) \
+	| prep-uefi-images
 tests/pxe-test$(EXESUF): tests/pxe-test.o tests/boot-sector.o $(libqos-obj-y)
 tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
 tests/pca9552-test$(EXESUF): tests/pca9552-test.o $(libqos-omap-obj-y)
@@ -936,6 +938,18 @@ $(patsubst %, check-%, $(check-qapi-schema-y)): check-%.json: $(SRC_PATH)/%.json
 	@diff -u $(SRC_PATH)/$*.out $*.test.out
 	@diff -u $(SRC_PATH)/$*.exit $*.test.exit
 
+qtest-uefi-images = $(foreach target,$(QTEST_TARGETS), $(strip $(qtest-uefi-images-$(target))))
+# Suppress implicit pc-bios/foo rules
+$(patsubst %,pc-bios/%,$(qtest-uefi-images)): ;
+
+# create rules for expanding UEFI images for configured targets
+$(foreach uefi-img, $(qtest-uefi-images), \
+    $(eval tests/data/acpi/$(uefi-img): pc-bios/$(uefi-img) ; \
+              $(call quiet-command, cat $$< /dev/zero | head -c 67108864 > $$@)))
+
+.PHONY: prep-uefi-images
+prep-uefi-images: $(patsubst %, tests/data/acpi/%, $(qtest-uefi-images))
+
 .PHONY: check-tests/qapi-schema/doc-good.texi
 check-tests/qapi-schema/doc-good.texi: tests/qapi-schema/doc-good.test.texi
 	@diff -u $(SRC_PATH)/tests/qapi-schema/doc-good.texi $<
@@ -999,6 +1013,7 @@ check-clean:
 	rm -rf $(sort $(foreach target,$(SYSEMU_TARGET_LIST), $(check-qtest-$(target)-y)) $(check-qtest-generic-y))
 	rm -f tests/test-qapi-gen-timestamp
 	rm -rf $(TESTS_VENV_DIR) $(TESTS_RESULTS_DIR)
+	rm -f $(patsubst %,tests/data/acpi/%, $(qtest-uefi-images))
 
 clean: check-clean
 
@@ -1009,4 +1024,5 @@ all: $(QEMU_IOTESTS_HELPERS-y)
 -include $(wildcard tests/*.d)
 -include $(wildcard tests/libqos/*.d)
 
+
 endif
-- 
2.7.4

[Qemu-devel] [PATCH 13/14] tests: acpi: add simple arm/virt testcase

[Igor Mammedov]

adds simple arm/virt test case that starts guest with default values
(modulo cortex-a57)

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/Makefile.include   |  1 +
 tests/bios-tables-test.c | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index e1201d9..a62de4c 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -296,6 +296,7 @@ check-qtest-aarch64-$(CONFIG_SDHCI) += tests/sdhci-test$(EXESUF)
 check-qtest-aarch64-y += tests/boot-serial-test$(EXESUF)
 check-qtest-aarch64-y += tests/migration-test$(EXESUF)
 qtest-uefi-images-aarch64 = avmf.img avmf_vars.img
+check-qtest-aarch64-y += tests/bios-tables-test$(EXESUF)
 
 check-qtest-microblazeel-y += $(check-qtest-microblaze-y)
 
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index a64d0c2..197ae26 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -797,6 +797,20 @@ static void test_acpi_piix4_tcg_dimm_pxm(void)
     test_acpi_tcg_dimm_pxm(MACHINE_PC);
 }
 
+static void test_acpi_virt_tcg(void)
+{
+    test_data data;
+
+    memset(&data, 0, sizeof(data));
+    data.machine = "virt";
+    data.required_struct_types = base_required_struct_types;
+    data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types);
+    data.uefi_fl1 = "avmf.img";
+    data.uefi_fl2 = "avmf_vars.img";
+    test_acpi_one("-cpu cortex-a57 ", &data);
+    free_test_data(&data);
+}
+
 int main(int argc, char *argv[])
 {
     const char *arch = qtest_get_arch();
@@ -824,6 +838,8 @@ int main(int argc, char *argv[])
         qtest_add_func("acpi/q35/numamem", test_acpi_q35_tcg_numamem);
         qtest_add_func("acpi/piix4/dimmpxm", test_acpi_piix4_tcg_dimm_pxm);
         qtest_add_func("acpi/q35/dimmpxm", test_acpi_q35_tcg_dimm_pxm);
+    } else if (strcmp(arch, "aarch64") == 0) {
+        qtest_add_func("acpi/virt", test_acpi_virt_tcg);
     }
     ret = g_test_run();
     boot_sector_cleanup(disk);
-- 
2.7.4

[Qemu-devel] [PATCH 14/14] tests: acpi: refactor rebuild-expected-aml.sh to dump ACPI tables for a specified list of targets

[Igor Mammedov]

Make initial list contain aarch64 and x86_64 targets.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/data/acpi/rebuild-expected-aml.sh | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/tests/data/acpi/rebuild-expected-aml.sh b/tests/data/acpi/rebuild-expected-aml.sh
index bf9ba24..6287aa6 100755
--- a/tests/data/acpi/rebuild-expected-aml.sh
+++ b/tests/data/acpi/rebuild-expected-aml.sh
@@ -7,21 +7,12 @@
 #
 # Authors:
 #  Marcel Apfelbaum <marcel.a@redhat.com>
+#  Igor Mammedov <imammedo@redhat.com>
 #
 # This work is licensed under the terms of the GNU GPLv2.
 # See the COPYING.LIB file in the top-level directory.
 
-qemu=
-
-if [ -e x86_64-softmmu/qemu-system-x86_64 ]; then
-    qemu="x86_64-softmmu/qemu-system-x86_64"
-elif [ -e i386-softmmu/qemu-system-i386 ]; then
-    qemu="i386-softmmu/qemu-system-i386"
-else
-    echo "Run 'make' to build the qemu exectutable!"
-    echo "Run this script from the build directory."
-    exit 1;
-fi
+qemu_bins="aarch64-softmmu/qemu-system-aarch64 x86_64-softmmu/qemu-system-x86_64"
 
 if [ ! -e "tests/bios-tables-test" ]; then
     echo "Test: bios-tables-test is required! Run make check before this script."
@@ -29,6 +20,14 @@ if [ ! -e "tests/bios-tables-test" ]; then
     exit 1;
 fi
 
-TEST_ACPI_REBUILD_AML=y QTEST_QEMU_BINARY=$qemu tests/bios-tables-test
+for qemu in $qemu_bins; do
+    if [ ! -e $qemu ]; then
+        echo "Run 'make' to build following the qemu exectutables: $qemu_bins"
+        echo "Run this script from the build directory."
+        exit 1;
+    fi
+    TEST_ACPI_REBUILD_AML=y QTEST_QEMU_BINARY=$qemu tests/bios-tables-test
+done
+
 
 echo "The files were rebuilt and can be added to git."
-- 
2.7.4

Re: [Qemu-devel] [PATCH 01/14] tests: acpi: add uefi_find_rsdp_addr() helper

[Laszlo Ersek]

Hi Igor,

On 01/15/19 16:40, Igor Mammedov wrote:
> introduce UEFI specific counterpart to acpi_find_rsdp_address()
> that will help to find RSDP address when [OA]VMF is used as
> firmware. It requires a [OA]VMF built with PcdAcpiTestSupport=TRUE,
> to locate RSDP address within 1Mb aligned ACPI test structure, tagged
> with GUID AB87A6B1-2034-BDA0-71BD-375007757785
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  tests/acpi-utils.h |  1 +
>  tests/acpi-utils.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)

I'm not promising to review all of this patch set (Phil, feel free to
chime in); I'll just make some quick comments below:

> diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
> index ef388bb..3b11f47 100644
> --- a/tests/acpi-utils.h
> +++ b/tests/acpi-utils.h
> @@ -46,6 +46,7 @@ typedef struct {
>  
>  uint8_t acpi_calc_checksum(const uint8_t *data, int len);
>  uint32_t acpi_find_rsdp_address(QTestState *qts);
> +uint64_t uefi_find_rsdp_addr(QTestState *qts, uint64_t start, uint64_t size);

I think it would make sense to keep the "acpi_find_rsdp_address" prefix
for the new function name; maybe append "_uefi"? Because now "acpi" is
replaced with "uefi , plus "address" is truncated to "addr"; those don't
seem overly logical.

Anyway, up to you.

>  uint64_t acpi_get_xsdt_address(uint8_t *rsdp_table);
>  void acpi_parse_rsdp_table(QTestState *qts, uint32_t addr, uint8_t *rsdp_table);
>  void acpi_fetch_table(QTestState *qts, uint8_t **aml, uint32_t *aml_len,
> diff --git a/tests/acpi-utils.c b/tests/acpi-utils.c
> index cc33b46..b9ff9df 100644
> --- a/tests/acpi-utils.c
> +++ b/tests/acpi-utils.c
> @@ -111,3 +111,46 @@ void acpi_fetch_table(QTestState *qts, uint8_t **aml, uint32_t *aml_len,
>          g_assert(!acpi_calc_checksum(*aml, *aml_len));
>      }
>  }
> +
> +#define GUID_SIZE 16
> +static uint8_t AcpiTestSupportGuid[GUID_SIZE] =
> +     { 0xb1, 0xa6, 0x87, 0xab,
> +       0x34, 0x20,
> +       0xa0, 0xbd,
> +       0x71, 0xbd, 0x37, 0x50, 0x07, 0x75, 0x77, 0x85 };

I think this is generally good. QEMU has some utilities/helpers for
working with UUIDs; however, for the test infrastructure, I think this
should be good enough.

Suggestion: make the GUID "const" as well.

> +
> +typedef struct {
> +    uint8_t signature_guid[16];

s/16/GUID_SIZE/?

> +    uint64_t rsdp10;
> +    uint64_t rsdp20;
> +} __attribute__((packed)) UefiTestSupport;
> +
> +/* Wait at most 600 seconds (test is slow with TCI and --enable-debug) */

Do you specifically mean "Tiny Code Interpreter" here?

> +#define TEST_DELAY (1 * G_USEC_PER_SEC / 10)
> +#define TEST_CYCLES MAX((600 * G_USEC_PER_SEC / TEST_DELAY), 1)
> +#define MB 0x100000ULL
> +uint64_t uefi_find_rsdp_addr(QTestState *qts, uint64_t start, uint64_t size)
> +{
> +    int i, j;
> +    uint8_t data[GUID_SIZE];
> +
> +    for (i = 0; i < TEST_CYCLES; ++i) {
> +        for (j = 0; j < size / MB; j++) {
> +            /* look for GUID at every 1Mb block */
> +            uint64_t addr = start + j * MB;
> +
> +            qtest_memread(qts, addr, data, sizeof(data));
> +            if (!memcmp(AcpiTestSupportGuid, data, sizeof(data))) {
> +                UefiTestSupport ret;
> +
> +                qtest_memread(qts, addr, &ret, sizeof(ret));
> +                ret.rsdp10 = le64_to_cpu(ret.rsdp10);
> +                ret.rsdp20 = le64_to_cpu(ret.rsdp20);
> +                return ret.rsdp20 ? ret.rsdp20 : ret.rsdp10;
> +            }
> +        }
> +        g_usleep(TEST_DELAY);
> +    }
> +    g_assert_not_reached();
> +    return 0;
> +}
> 

Apart from my hair-splitting, it looks good. If you update
16-->GUID_SIZE, then you can add

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH 04/14] tests: acpi: make pointer to RSDP 64bit

[Laszlo Ersek]

On 01/15/19 16:40, Igor Mammedov wrote:
> In case of UEFI RSDP doesn't have to be located in lowmem,

I suggest inserting a comma (,) after "UEFI". Currently the message
reads like

  in case of UEFI RSDP, doesn't have to... ENOPARSE

:) It should be

  in case of UEFI, RSDP doesn't have to...

Thanks
Laszlo

> it could be placed at any address. Make sure that test won't
> break if it is placed above the first 4Gb of address space.
> 
> PS:
> While at it cleanup some local variables as we don't really
> need them.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  tests/bios-tables-test.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 99d7bf8..c28c5c7 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -26,7 +26,7 @@
>  typedef struct {
>      const char *machine;
>      const char *variant;
> -    uint32_t rsdp_addr;
> +    uint64_t rsdp_addr;
>      uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
>      GArray *tables;
>      uint32_t smbios_ep_addr;
> @@ -86,13 +86,11 @@ static void test_acpi_rsdp_address(test_data *data)
>  
>  static void test_acpi_rsdp_table(test_data *data)
>  {
> -    uint8_t *rsdp_table = data->rsdp_table, revision;
> -    uint32_t addr = data->rsdp_addr;
> +    uint8_t *rsdp_table = data->rsdp_table;
>  
> -    acpi_fetch_rsdp_table(data->qts, addr, rsdp_table);
> -    revision = rsdp_table[15 /* Revision offset */];
> +    acpi_fetch_rsdp_table(data->qts, data->rsdp_addr, rsdp_table);
>  
> -    switch (revision) {
> +    switch (rsdp_table[15 /* Revision offset */]) {
>      case 0: /* ACPI 1.0 RSDP */
>          /* With rev 1, checksum is only for the first 20 bytes */
>          g_assert(!acpi_calc_checksum(rsdp_table,  20));
> 

Re: [Qemu-devel] [PATCH 08/14] tests: acpi: introduce an abilty start tests with UEFI firmware

[Laszlo Ersek]

On 01/15/19 16:41, Igor Mammedov wrote:
> For testcase to use UEFI firmware, one needs to provide and specify
> firmware and varstore blobs names in test_data { uefi_fl1, uefi_fl2) }
> fields respectively.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  tests/bios-tables-test.c | 36 ++++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 8887319..d290dd2 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -26,6 +26,8 @@
>  typedef struct {
>      const char *machine;
>      const char *variant;
> +    const char *uefi_fl1;
> +    const char *uefi_fl2;
>      uint64_t rsdp_addr;
>      uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
>      GArray *tables;
> @@ -519,21 +521,35 @@ static void test_smbios_structs(test_data *data)
>  static void test_acpi_one(const char *params, test_data *data)
>  {
>      char *args;
> -
> -    /* Disable kernel irqchip to be able to override apic irq0. */
> -    args = g_strdup_printf("-machine %s,accel=%s,kernel-irqchip=off "
> -                           "-net none -display none %s "
> -                           "-drive id=hd0,if=none,file=%s,format=raw "
> -                           "-device ide-hd,drive=hd0 ",
> -                           data->machine, "kvm:tcg",
> -                           params ? params : "", disk);
> +    bool use_uefi = data->uefi_fl1 && data->uefi_fl2;
> +
> +    if (use_uefi) {
> +        args = g_strdup_printf("-machine %s,accel=%s -nodefaults -nographic "
> +            "-drive if=pflash,format=raw,file=%s/%s,readonly "
> +            "-drive if=pflash,format=raw,file=%s/%s,snapshot=on %s",

Today I Learned: about "snapshot=on". Thanks :) The command line looks good.

> +            data->machine, "kvm:tcg", data_dir, data->uefi_fl1, data_dir,

You could open-code "kvm:tcg" in the format string at once (unless you
turn that into a parameter in a later patch in the series). But, I see
the pre-patch code passes "kvm:tcg" as an argument too.

> +            data->uefi_fl2, params ? params : "");
> +
> +    } else {
> +        /* Disable kernel irqchip to be able to override apic irq0. */
> +        args = g_strdup_printf("-machine %s,accel=%s,kernel-irqchip=off "
> +            "-net none -display none %s "
> +            "-drive id=hd0,if=none,file=%s,format=raw "
> +            "-device ide-hd,drive=hd0 ",
> +             data->machine, "kvm:tcg", params ? params : "", disk);
> +    }
>  
>      data->qts = qtest_init(args);
>  
> -    boot_sector_test(data->qts);
> +    if (use_uefi) {
> +        data->rsdp_addr = uefi_find_rsdp_addr(data->qts,
> +            0x40000000ULL, 128ULL * 1024 * 1024);

I think open-coding the DRAM size is valid; after all, it depends on the
QEMU command line, and you control the QEMU command line above. However,
do we really want to open-code the DRAM base here? That's
board-specific. Should we pass that too through the "test_data" structure?

> +    } else {
> +        boot_sector_test(data->qts);
> +        test_acpi_rsdp_address(data);
> +    }
>  
>      data->tables = g_array_new(false, true, sizeof(AcpiSdtTable));
> -    test_acpi_rsdp_address(data);
>      test_acpi_rsdp_table(data);
>      test_acpi_rxsdt_table(data);
>      test_acpi_fadt_table(data);
> 

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH 10/14] tests: acpi: ignore SMBIOS tests when UEFI firmware is used

[Laszlo Ersek]

On 01/15/19 16:41, Igor Mammedov wrote:
> once FW provides a pointer to SMBIOS entry point like it does for
> RSDP it should be possible to enable this one the same way.

Good point, I didn't think of SMBIOS.

We have the following options:

(1) Use just one "test support" structure, and add more fields (such as
the SMBIOS entry point) to it, beyond the RSDP1.0/RSDP2.0. For this, we
should also introduce a "size" field to the table, so we don't have to
extend the table between firmware and QEMU in lock-step.

(2) Use a different table (with a different GUID) for exposing the
SMBIOS entry point.

On the firmware side, (1) would be more work now, but it would keep
things simpler (and better separated) in the future. (2) would be more
lazy ^W convenient now, but it would introduce more churn / possibly
some code duplication in the future.

In QEMU, which one would you prefer?

Thanks,
Laszlo

> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  tests/bios-tables-test.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index d9efe59..a64d0c2 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -562,8 +562,11 @@ static void test_acpi_one(const char *params, test_data *data)
>          }
>      }
>  
> -    test_smbios_entry_point(data);
> -    test_smbios_structs(data);
> +    /* TODO: make SMBIOS tests work with UEFI firmware */
> +    if (!use_uefi) {
> +        test_smbios_entry_point(data);
> +        test_smbios_structs(data);
> +    }
>  
>      assert(!global_qtest);
>      qtest_quit(data->qts);
> 

Re: [Qemu-devel] [PATCH 11/14] tests: acpi: add AVMF firmware blobs

[Laszlo Ersek]

On 01/15/19 16:41, Igor Mammedov wrote:
> Add firmware blobs built with PcdAcpiTestSupport=TRUE,
> that puts RSDP address in RAM after 1Mb aligned GUID
>   AB87A6B1-2034-BDA0-71BD-375007757785
> so that tests could scan and find it in RAM once firmware's
> initialized ACPI tables.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  Makefile              |   3 ++-
>  pc-bios/avmf.img      | Bin 0 -> 2097152 bytes
>  pc-bios/avmf_vars.img | Bin 0 -> 786432 bytes
>  3 files changed, 2 insertions(+), 1 deletion(-)
>  create mode 100644 pc-bios/avmf.img
>  create mode 100644 pc-bios/avmf_vars.img

"AVMF" is not a great name. "AAVMF" is a downstream name alright, but
many dislike it in upstream use. "edk2-aarch64" or "edk2-ArmVirtQemu"
would be more precise, but those are verbose. Sigh, why are names so
hard. What does everyone think?

Also, do you have to care about the license of firmware images built
from edk2 when bundling such images? Since edk2 commit 9a67ba261fe9
("ArmVirtPkg: Replace obsoleted network drivers from platform DSC/FDF.",
2018-12-14), you cannot build the ArmVirtQemu (aka AAVMF) firmware
without OpenSSL. Thus, the resultant license is 2-BSDL + OpenSSL -- for
now anyway.

Because, in the near future, that might change again. edk2 is in the
process of moving to Apache-2.0, and OpenSSL will also move (AFAICT) to
Apache-2.0 starting with release 3.0.0. (See commit 151333164ece,
"Change license to the Apache License v2.0", 2018-12-06.)

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 01/14] tests: acpi: add uefi_find_rsdp_addr() helper

[Igor Mammedov]

On Tue, 15 Jan 2019 21:06:16 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> Hi Igor,
> 
> On 01/15/19 16:40, Igor Mammedov wrote:
> > introduce UEFI specific counterpart to acpi_find_rsdp_address()
> > that will help to find RSDP address when [OA]VMF is used as
> > firmware. It requires a [OA]VMF built with PcdAcpiTestSupport=TRUE,
> > to locate RSDP address within 1Mb aligned ACPI test structure, tagged
> > with GUID AB87A6B1-2034-BDA0-71BD-375007757785
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  tests/acpi-utils.h |  1 +
> >  tests/acpi-utils.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 44 insertions(+)  
> 
> I'm not promising to review all of this patch set (Phil, feel free to
> chime in); I'll just make some quick comments below:
> 
> > diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
> > index ef388bb..3b11f47 100644
> > --- a/tests/acpi-utils.h
> > +++ b/tests/acpi-utils.h
> > @@ -46,6 +46,7 @@ typedef struct {
> >  
> >  uint8_t acpi_calc_checksum(const uint8_t *data, int len);
> >  uint32_t acpi_find_rsdp_address(QTestState *qts);
> > +uint64_t uefi_find_rsdp_addr(QTestState *qts, uint64_t start, uint64_t size);  
> 
> I think it would make sense to keep the "acpi_find_rsdp_address" prefix
> for the new function name; maybe append "_uefi"? Because now "acpi" is
> replaced with "uefi , plus "address" is truncated to "addr"; those don't
> seem overly logical.
> 
> Anyway, up to you.
> 
> >  uint64_t acpi_get_xsdt_address(uint8_t *rsdp_table);
> >  void acpi_parse_rsdp_table(QTestState *qts, uint32_t addr, uint8_t *rsdp_table);
> >  void acpi_fetch_table(QTestState *qts, uint8_t **aml, uint32_t *aml_len,
> > diff --git a/tests/acpi-utils.c b/tests/acpi-utils.c
> > index cc33b46..b9ff9df 100644
> > --- a/tests/acpi-utils.c
> > +++ b/tests/acpi-utils.c
> > @@ -111,3 +111,46 @@ void acpi_fetch_table(QTestState *qts, uint8_t **aml, uint32_t *aml_len,
> >          g_assert(!acpi_calc_checksum(*aml, *aml_len));
> >      }
> >  }
> > +
> > +#define GUID_SIZE 16
> > +static uint8_t AcpiTestSupportGuid[GUID_SIZE] =
> > +     { 0xb1, 0xa6, 0x87, 0xab,
> > +       0x34, 0x20,
> > +       0xa0, 0xbd,
> > +       0x71, 0xbd, 0x37, 0x50, 0x07, 0x75, 0x77, 0x85 };  
> 
> I think this is generally good. QEMU has some utilities/helpers for
> working with UUIDs; however, for the test infrastructure, I think this
> should be good enough.
> 
> Suggestion: make the GUID "const" as well.
> 
> > +
> > +typedef struct {
> > +    uint8_t signature_guid[16];  
> 
> s/16/GUID_SIZE/?
> 
> > +    uint64_t rsdp10;
> > +    uint64_t rsdp20;
> > +} __attribute__((packed)) UefiTestSupport;
> > +
> > +/* Wait at most 600 seconds (test is slow with TCI and --enable-debug) */  
> 
> Do you specifically mean "Tiny Code Interpreter" here?
it's typo, I've meant TCG. I'll fix it.

> > +#define TEST_DELAY (1 * G_USEC_PER_SEC / 10)
> > +#define TEST_CYCLES MAX((600 * G_USEC_PER_SEC / TEST_DELAY), 1)
> > +#define MB 0x100000ULL
> > +uint64_t uefi_find_rsdp_addr(QTestState *qts, uint64_t start, uint64_t size)
> > +{
> > +    int i, j;
> > +    uint8_t data[GUID_SIZE];
> > +
> > +    for (i = 0; i < TEST_CYCLES; ++i) {
> > +        for (j = 0; j < size / MB; j++) {
> > +            /* look for GUID at every 1Mb block */
> > +            uint64_t addr = start + j * MB;
> > +
> > +            qtest_memread(qts, addr, data, sizeof(data));
> > +            if (!memcmp(AcpiTestSupportGuid, data, sizeof(data))) {
> > +                UefiTestSupport ret;
> > +
> > +                qtest_memread(qts, addr, &ret, sizeof(ret));
> > +                ret.rsdp10 = le64_to_cpu(ret.rsdp10);
> > +                ret.rsdp20 = le64_to_cpu(ret.rsdp20);
> > +                return ret.rsdp20 ? ret.rsdp20 : ret.rsdp10;
> > +            }
> > +        }
> > +        g_usleep(TEST_DELAY);
> > +    }
> > +    g_assert_not_reached();
> > +    return 0;
> > +}
> >   
> 
> Apart from my hair-splitting, it looks good. If you update
> 16-->GUID_SIZE, then you can add
Thanks,
I'll apply all of your suggestions for v2 respin

> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks,
> Laszlo
> 

Re: [Qemu-devel] [PATCH 08/14] tests: acpi: introduce an abilty start tests with UEFI firmware

[Igor Mammedov]

On Tue, 15 Jan 2019 21:18:18 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> On 01/15/19 16:41, Igor Mammedov wrote:
> > For testcase to use UEFI firmware, one needs to provide and specify
> > firmware and varstore blobs names in test_data { uefi_fl1, uefi_fl2) }
> > fields respectively.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  tests/bios-tables-test.c | 36 ++++++++++++++++++++++++++----------
> >  1 file changed, 26 insertions(+), 10 deletions(-)
> > 
> > diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> > index 8887319..d290dd2 100644
> > --- a/tests/bios-tables-test.c
> > +++ b/tests/bios-tables-test.c
> > @@ -26,6 +26,8 @@
> >  typedef struct {
> >      const char *machine;
> >      const char *variant;
> > +    const char *uefi_fl1;
> > +    const char *uefi_fl2;
> >      uint64_t rsdp_addr;
> >      uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
> >      GArray *tables;
> > @@ -519,21 +521,35 @@ static void test_smbios_structs(test_data *data)
> >  static void test_acpi_one(const char *params, test_data *data)
> >  {
> >      char *args;
> > -
> > -    /* Disable kernel irqchip to be able to override apic irq0. */
> > -    args = g_strdup_printf("-machine %s,accel=%s,kernel-irqchip=off "
> > -                           "-net none -display none %s "
> > -                           "-drive id=hd0,if=none,file=%s,format=raw "
> > -                           "-device ide-hd,drive=hd0 ",
> > -                           data->machine, "kvm:tcg",
> > -                           params ? params : "", disk);
> > +    bool use_uefi = data->uefi_fl1 && data->uefi_fl2;
> > +
> > +    if (use_uefi) {
> > +        args = g_strdup_printf("-machine %s,accel=%s -nodefaults -nographic "
> > +            "-drive if=pflash,format=raw,file=%s/%s,readonly "
> > +            "-drive if=pflash,format=raw,file=%s/%s,snapshot=on %s",  
> 
> Today I Learned: about "snapshot=on". Thanks :) The command line looks good.
This way one doesn't have to make images in C (which is messy), keep track of
temporary images and clean up (which isn't reliable when test crashes).
Makefile magic can to all of that and in a much cleaner way.

> > +            data->machine, "kvm:tcg", data_dir, data->uefi_fl1, data_dir,  
> 
> You could open-code "kvm:tcg" in the format string at once (unless you
> turn that into a parameter in a later patch in the series). But, I see
> the pre-patch code passes "kvm:tcg" as an argument too.
That was my line of reasoning as well, so I've kept it for consistence
with original code. If someone would insist I can throw in a separate
patch to open-code it before this one.

> > +            data->uefi_fl2, params ? params : "");
> > +
> > +    } else {
> > +        /* Disable kernel irqchip to be able to override apic irq0. */
> > +        args = g_strdup_printf("-machine %s,accel=%s,kernel-irqchip=off "
> > +            "-net none -display none %s "
> > +            "-drive id=hd0,if=none,file=%s,format=raw "
> > +            "-device ide-hd,drive=hd0 ",
> > +             data->machine, "kvm:tcg", params ? params : "", disk);
> > +    }
> >  
> >      data->qts = qtest_init(args);
> >  
> > -    boot_sector_test(data->qts);
> > +    if (use_uefi) {
> > +        data->rsdp_addr = uefi_find_rsdp_addr(data->qts,
> > +            0x40000000ULL, 128ULL * 1024 * 1024);  
> 
> I think open-coding the DRAM size is valid; after all, it depends on the
> QEMU command line, and you control the QEMU command line above. However,
> do we really want to open-code the DRAM base here? That's
> board-specific. Should we pass that too through the "test_data" structure?
I've missed this one. In addition to your suggestion, RAM size is also
a good candidate for "test_data" structure.

> 
> > +    } else {
> > +        boot_sector_test(data->qts);
> > +        test_acpi_rsdp_address(data);
> > +    }
> >  
> >      data->tables = g_array_new(false, true, sizeof(AcpiSdtTable));
> > -    test_acpi_rsdp_address(data);
> >      test_acpi_rsdp_table(data);
> >      test_acpi_rxsdt_table(data);
> >      test_acpi_fadt_table(data);
> >   
> 
> Thanks,
> Laszlo

Re: [Qemu-devel] [PATCH 10/14] tests: acpi: ignore SMBIOS tests when UEFI firmware is used

[Igor Mammedov]

On Tue, 15 Jan 2019 21:31:54 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> On 01/15/19 16:41, Igor Mammedov wrote:
> > once FW provides a pointer to SMBIOS entry point like it does for
> > RSDP it should be possible to enable this one the same way.  
> 
> Good point, I didn't think of SMBIOS.
> 
> We have the following options:
> 
> (1) Use just one "test support" structure, and add more fields (such as
> the SMBIOS entry point) to it, beyond the RSDP1.0/RSDP2.0. For this, we
> should also introduce a "size" field to the table, so we don't have to
> extend the table between firmware and QEMU in lock-step.
> 
> (2) Use a different table (with a different GUID) for exposing the
> SMBIOS entry point.
> 
> On the firmware side, (1) would be more work now, but it would keep
> things simpler (and better separated) in the future. (2) would be more
> lazy ^W convenient now, but it would introduce more churn / possibly
> some code duplication in the future.
> 
> In QEMU, which one would you prefer?
I'd prefer #1 to minimize # of memory scans.
However with size (i.e. implicit versioning) and who know what else in
the future complexity grows up and dependency this approach causes
between firmware and QEMU (I dislike special build instead of reusing
shipped images).

So I've dug a little bit into the history why we've chosen including
structure into the firmware itself instead of writing EFI application
as part of QEMU that would provide the same test structure but won't
require special firmware build.
If I sum it up, it was issue with distros are shipping (if they do it at all)
only a version that matches distro's architecture and a need for cross
compiling EFI test app.

Could we revisit EFI app approach (I'd prefer it over firwmare hack if it's
possible)? We can try to avoid dependency on gnu-efi and cross compiling on
regular builds and ship along with efi app source code several prebuild app
binaries (boot disk images), that one would rebuild only when efi app
is changed, and it could be done manually (the same like special fw build
but contained withing QEMU). As for gnu-efi, is it possible to use striped
down gnu-efi stubs to drop external library dependency, something along of
lines https://github.com/tqh/efi-example ?


> Thanks,
> Laszlo
> 
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  tests/bios-tables-test.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> > index d9efe59..a64d0c2 100644
> > --- a/tests/bios-tables-test.c
> > +++ b/tests/bios-tables-test.c
> > @@ -562,8 +562,11 @@ static void test_acpi_one(const char *params, test_data *data)
> >          }
> >      }
> >  
> > -    test_smbios_entry_point(data);
> > -    test_smbios_structs(data);
> > +    /* TODO: make SMBIOS tests work with UEFI firmware */
> > +    if (!use_uefi) {
> > +        test_smbios_entry_point(data);
> > +        test_smbios_structs(data);
> > +    }
> >  
> >      assert(!global_qtest);
> >      qtest_quit(data->qts);
> >   
> 
> 

Re: [Qemu-devel] [PATCH 10/14] tests: acpi: ignore SMBIOS tests when UEFI firmware is used

[Laszlo Ersek]

+Gerd

On 01/16/19 11:32, Igor Mammedov wrote:
> On Tue, 15 Jan 2019 21:31:54 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 01/15/19 16:41, Igor Mammedov wrote:
>>> once FW provides a pointer to SMBIOS entry point like it does for
>>> RSDP it should be possible to enable this one the same way.  
>>
>> Good point, I didn't think of SMBIOS.
>>
>> We have the following options:
>>
>> (1) Use just one "test support" structure, and add more fields (such as
>> the SMBIOS entry point) to it, beyond the RSDP1.0/RSDP2.0. For this, we
>> should also introduce a "size" field to the table, so we don't have to
>> extend the table between firmware and QEMU in lock-step.
>>
>> (2) Use a different table (with a different GUID) for exposing the
>> SMBIOS entry point.
>>
>> On the firmware side, (1) would be more work now, but it would keep
>> things simpler (and better separated) in the future. (2) would be more
>> lazy ^W convenient now, but it would introduce more churn / possibly
>> some code duplication in the future.
>>
>> In QEMU, which one would you prefer?
> I'd prefer #1 to minimize # of memory scans.
> However with size (i.e. implicit versioning) and who know what else in
> the future complexity grows up and dependency this approach causes
> between firmware and QEMU (I dislike special build instead of reusing
> shipped images).
> 
> So I've dug a little bit into the history why we've chosen including
> structure into the firmware itself instead of writing EFI application
> as part of QEMU that would provide the same test structure but won't
> require special firmware build.
> If I sum it up, it was issue with distros are shipping (if they do it at all)
> only a version that matches distro's architecture and a need for cross
> compiling EFI test app.
> 
> Could we revisit EFI app approach (I'd prefer it over firwmare hack if it's
> possible)? We can try to avoid dependency on gnu-efi and cross compiling on
> regular builds and ship along with efi app source code several prebuild app
> binaries (boot disk images), that one would rebuild only when efi app
> is changed, and it could be done manually (the same like special fw build
> but contained withing QEMU). As for gnu-efi, is it possible to use striped
> down gnu-efi stubs to drop external library dependency, something along of
> lines https://github.com/tqh/efi-example ?

If it is permissible to require the affected QEMU maintainers to
*manually* rebuild the UEFI binary on their workstations, whenever the
source code for the UEFI app changes, then the solution is a lot easier
indeed.

In particular, for this approach, we don't even need gnu-efi. (Because,
personally, I would strongly prefer to write the UEFI application with
the edk2 framework.) For a while now, edk2 has supported "multiple
workspaces":

https://github.com/tianocore/tianocore.github.io/wiki/Multiple_Workspace

and as a result, it is possible to build the necessary UEFI app from:
- an edk2 checkout that the maintainer has "somewhere" on their disk,
- and the UEFI app source code that is contained in a QEMU checkout that
the maintainer has "somewhere else" on their disk.

This approach allows the UEFI app source to live in the QEMU tree, and
the affected maintainer(s) would be personally responsible for setting
up their edk2 clones, and compilers. (The edk2 clone could even be a
submodule of QEMU, for example at roms/edk2.) For example,
"roms/Makefile" already calls an external EFIROM utility (also from
edk2) in order to build the combined iPXE option ROMs.

And yes, we could turn the UEFI binaries into bootable ISO images at once.

I'll try to post some patches soon (or not so soon). I think the app's
source code, and the edk2 submodule, should live under roms/, and the
bootable images should live under pc-bios/.

(In fact we could use this opportunity to build & bundle OVMF itself...
not sure if that's in scope for now. Gerd, what's your take?)

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH 10/14] tests: acpi: ignore SMBIOS tests when UEFI firmware is used

[Laszlo Ersek]

On 01/16/19 12:07, Laszlo Ersek wrote:
> +Gerd
> 
> On 01/16/19 11:32, Igor Mammedov wrote:
>> On Tue, 15 Jan 2019 21:31:54 +0100
>> Laszlo Ersek <lersek@redhat.com> wrote:
>>
>>> On 01/15/19 16:41, Igor Mammedov wrote:
>>>> once FW provides a pointer to SMBIOS entry point like it does for
>>>> RSDP it should be possible to enable this one the same way.  
>>>
>>> Good point, I didn't think of SMBIOS.
>>>
>>> We have the following options:
>>>
>>> (1) Use just one "test support" structure, and add more fields (such as
>>> the SMBIOS entry point) to it, beyond the RSDP1.0/RSDP2.0. For this, we
>>> should also introduce a "size" field to the table, so we don't have to
>>> extend the table between firmware and QEMU in lock-step.
>>>
>>> (2) Use a different table (with a different GUID) for exposing the
>>> SMBIOS entry point.
>>>
>>> On the firmware side, (1) would be more work now, but it would keep
>>> things simpler (and better separated) in the future. (2) would be more
>>> lazy ^W convenient now, but it would introduce more churn / possibly
>>> some code duplication in the future.
>>>
>>> In QEMU, which one would you prefer?
>> I'd prefer #1 to minimize # of memory scans.
>> However with size (i.e. implicit versioning) and who know what else in
>> the future complexity grows up and dependency this approach causes
>> between firmware and QEMU (I dislike special build instead of reusing
>> shipped images).
>>
>> So I've dug a little bit into the history why we've chosen including
>> structure into the firmware itself instead of writing EFI application
>> as part of QEMU that would provide the same test structure but won't
>> require special firmware build.
>> If I sum it up, it was issue with distros are shipping (if they do it at all)
>> only a version that matches distro's architecture and a need for cross
>> compiling EFI test app.
>>
>> Could we revisit EFI app approach (I'd prefer it over firwmare hack if it's
>> possible)? We can try to avoid dependency on gnu-efi and cross compiling on
>> regular builds and ship along with efi app source code several prebuild app
>> binaries (boot disk images), that one would rebuild only when efi app
>> is changed, and it could be done manually (the same like special fw build
>> but contained withing QEMU). As for gnu-efi, is it possible to use striped
>> down gnu-efi stubs to drop external library dependency, something along of
>> lines https://github.com/tqh/efi-example ?
> 
> If it is permissible to require the affected QEMU maintainers to
> *manually* rebuild the UEFI binary on their workstations, whenever the
> source code for the UEFI app changes, then the solution is a lot easier
> indeed.
> 
> In particular, for this approach, we don't even need gnu-efi. (Because,
> personally, I would strongly prefer to write the UEFI application with
> the edk2 framework.) For a while now, edk2 has supported "multiple
> workspaces":
> 
> https://github.com/tianocore/tianocore.github.io/wiki/Multiple_Workspace
> 
> and as a result, it is possible to build the necessary UEFI app from:
> - an edk2 checkout that the maintainer has "somewhere" on their disk,
> - and the UEFI app source code that is contained in a QEMU checkout that
> the maintainer has "somewhere else" on their disk.
> 
> This approach allows the UEFI app source to live in the QEMU tree, and
> the affected maintainer(s) would be personally responsible for setting
> up their edk2 clones, and compilers. (The edk2 clone could even be a
> submodule of QEMU, for example at roms/edk2.) For example,
> "roms/Makefile" already calls an external EFIROM utility (also from
> edk2) in order to build the combined iPXE option ROMs.
> 
> And yes, we could turn the UEFI binaries into bootable ISO images at once.
> 
> I'll try to post some patches soon (or not so soon). I think the app's
> source code, and the edk2 submodule, should live under roms/, and the
> bootable images should live under pc-bios/.
> 
> (In fact we could use this opportunity to build & bundle OVMF itself...
> not sure if that's in scope for now. Gerd, what's your take?)

Oh, I should add: the UEFI app in question could be built without
pulling in any OpenSSL bits; OVMF itself can't be built like that (it
now has a hard dependency on OpenSSL). This might matter from a
licensing/bundling perspective.

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH 10/14] tests: acpi: ignore SMBIOS tests when UEFI firmware is used

[Gerd Hoffmann]

  Hi,

> This approach allows the UEFI app source to live in the QEMU tree, and
> the affected maintainer(s) would be personally responsible for setting
> up their edk2 clones, and compilers. (The edk2 clone could even be a
> submodule of QEMU, for example at roms/edk2.) For example,
> "roms/Makefile" already calls an external EFIROM utility (also from
> edk2) in order to build the combined iPXE option ROMs.
> 
> And yes, we could turn the UEFI binaries into bootable ISO images at once.
> 
> I'll try to post some patches soon (or not so soon). I think the app's
> source code, and the edk2 submodule, should live under roms/, and the
> bootable images should live under pc-bios/.
> 
> (In fact we could use this opportunity to build & bundle OVMF itself...
> not sure if that's in scope for now. Gerd, what's your take?)

Well, there is still the idea to move over firmware submodules and
prebuilt firmware blobs to a separate repo.  Expermimental repo:
https://git.kraxel.org/cgit/qemu-firmware/.  Not touched for more than a
year due to being busy with other stuff.  Oh well ...

(if someone feels like picking this up feel free to do so).

I think adding edk2 as submodule below roms/ makes sense.  Adding rules
to roms/Makefile to build the blobs makes sense too.  Not sure we want
the binaries actually copied over to pc-bios/ and commited as the uefi
firmware is pretty big ...

Not sure what a good place for the uefi app would be.  I'd tend to not
use roms/, that is the place for firmware submodules.  contrib/ or test/
maybe?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 10/14] tests: acpi: ignore SMBIOS tests when UEFI firmware is used

[Igor Mammedov]

On Wed, 16 Jan 2019 12:09:03 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> On 01/16/19 12:07, Laszlo Ersek wrote:
> > +Gerd
> > 
> > On 01/16/19 11:32, Igor Mammedov wrote:  
> >> On Tue, 15 Jan 2019 21:31:54 +0100
> >> Laszlo Ersek <lersek@redhat.com> wrote:
> >>  
> >>> On 01/15/19 16:41, Igor Mammedov wrote:  
> >>>> once FW provides a pointer to SMBIOS entry point like it does for
> >>>> RSDP it should be possible to enable this one the same way.    
> >>>
> >>> Good point, I didn't think of SMBIOS.
> >>>
> >>> We have the following options:
> >>>
> >>> (1) Use just one "test support" structure, and add more fields (such as
> >>> the SMBIOS entry point) to it, beyond the RSDP1.0/RSDP2.0. For this, we
> >>> should also introduce a "size" field to the table, so we don't have to
> >>> extend the table between firmware and QEMU in lock-step.
> >>>
> >>> (2) Use a different table (with a different GUID) for exposing the
> >>> SMBIOS entry point.
> >>>
> >>> On the firmware side, (1) would be more work now, but it would keep
> >>> things simpler (and better separated) in the future. (2) would be more
> >>> lazy ^W convenient now, but it would introduce more churn / possibly
> >>> some code duplication in the future.
> >>>
> >>> In QEMU, which one would you prefer?  
> >> I'd prefer #1 to minimize # of memory scans.
> >> However with size (i.e. implicit versioning) and who know what else in
> >> the future complexity grows up and dependency this approach causes
> >> between firmware and QEMU (I dislike special build instead of reusing
> >> shipped images).
> >>
> >> So I've dug a little bit into the history why we've chosen including
> >> structure into the firmware itself instead of writing EFI application
> >> as part of QEMU that would provide the same test structure but won't
> >> require special firmware build.
> >> If I sum it up, it was issue with distros are shipping (if they do it at all)
> >> only a version that matches distro's architecture and a need for cross
> >> compiling EFI test app.
> >>
> >> Could we revisit EFI app approach (I'd prefer it over firwmare hack if it's
> >> possible)? We can try to avoid dependency on gnu-efi and cross compiling on
> >> regular builds and ship along with efi app source code several prebuild app
> >> binaries (boot disk images), that one would rebuild only when efi app
> >> is changed, and it could be done manually (the same like special fw build
> >> but contained withing QEMU). As for gnu-efi, is it possible to use striped
> >> down gnu-efi stubs to drop external library dependency, something along of
> >> lines https://github.com/tqh/efi-example ?  
> > 
> > If it is permissible to require the affected QEMU maintainers to
> > *manually* rebuild the UEFI binary on their workstations, whenever the
> > source code for the UEFI app changes, then the solution is a lot easier
> > indeed.
I expect EFI app won't change often based on how much we've changed similar
counterpart for BIOS version. So it probably ok, due to low load and we
won't have to waste resources on rebuilding static thing over and over again
on every qemu build (I'm horrified how much longer QEMU build would become
if we would have to pull in complex dependencies to build it).

> > In particular, for this approach, we don't even need gnu-efi. (Because,
> > personally, I would strongly prefer to write the UEFI application with
> > the edk2 framework.) For a while now, edk2 has supported "multiple
> > workspaces":
> > 
> > https://github.com/tianocore/tianocore.github.io/wiki/Multiple_Workspace
> > 
> > and as a result, it is possible to build the necessary UEFI app from:
> > - an edk2 checkout that the maintainer has "somewhere" on their disk,
> > - and the UEFI app source code that is contained in a QEMU checkout that
> > the maintainer has "somewhere else" on their disk.
> > 
> > This approach allows the UEFI app source to live in the QEMU tree, and
> > the affected maintainer(s) would be personally responsible for setting
> > up their edk2 clones, and compilers. (The edk2 clone could even be a
> > submodule of QEMU, for example at roms/edk2.) For example,
> > "roms/Makefile" already calls an external EFIROM utility (also from
> > edk2) in order to build the combined iPXE option ROMs.
> > 
> > And yes, we could turn the UEFI binaries into bootable ISO images at once.
> > 
> > I'll try to post some patches soon (or not so soon). I think the app's
> > source code, and the edk2 submodule, should live under roms/, and the
> > bootable images should live under pc-bios/.
> > 
> > (In fact we could use this opportunity to build & bundle OVMF itself...
> > not sure if that's in scope for now. Gerd, what's your take?)  
> 
> Oh, I should add: the UEFI app in question could be built without
> pulling in any OpenSSL bits; OVMF itself can't be built like that (it
> now has a hard dependency on OpenSSL). This might matter from a
> licensing/bundling perspective.
That's why I've suggested non EDK2 variant, as we don't need very much
from it and its license looks BDS like and it's very simple to build
pretty much for everyone.
Dealing with EDK2 to rebuild EFI is rather daunting prospect
(at least from my point of view so I'd try to avoid it if possible)


> 
> Thanks,
> Laszlo

Re: [Qemu-devel] [PATCH 11/14] tests: acpi: add AVMF firmware blobs

[Igor Mammedov]

On Tue, 15 Jan 2019 21:47:49 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> On 01/15/19 16:41, Igor Mammedov wrote:
> > Add firmware blobs built with PcdAcpiTestSupport=TRUE,
> > that puts RSDP address in RAM after 1Mb aligned GUID
> >   AB87A6B1-2034-BDA0-71BD-375007757785
> > so that tests could scan and find it in RAM once firmware's
> > initialized ACPI tables.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  Makefile              |   3 ++-
> >  pc-bios/avmf.img      | Bin 0 -> 2097152 bytes
> >  pc-bios/avmf_vars.img | Bin 0 -> 786432 bytes
> >  3 files changed, 2 insertions(+), 1 deletion(-)
> >  create mode 100644 pc-bios/avmf.img
> >  create mode 100644 pc-bios/avmf_vars.img  
> 
> "AVMF" is not a great name. "AAVMF" is a downstream name alright, but
> many dislike it in upstream use. "edk2-aarch64" or "edk2-ArmVirtQemu"
> would be more precise, but those are verbose. Sigh, why are names so
> hard. What does everyone think?
I'm fine with either version.
 
> Also, do you have to care about the license of firmware images built
> from edk2 when bundling such images? Since edk2 commit 9a67ba261fe9
> ("ArmVirtPkg: Replace obsoleted network drivers from platform DSC/FDF.",
> 2018-12-14), you cannot build the ArmVirtQemu (aka AAVMF) firmware
> without OpenSSL. Thus, the resultant license is 2-BSDL + OpenSSL -- for
> now anyway.
> 
> Because, in the near future, that might change again. edk2 is in the
> process of moving to Apache-2.0, and OpenSSL will also move (AFAICT) to
> Apache-2.0 starting with release 3.0.0. (See commit 151333164ece,
> "Change license to the Apache License v2.0", 2018-12-06.)
That's another reason to look into EFI app approach (a simple one without
dependencies) ans let distro to provide firmware image.

> Thanks
> Laszlo

Re: [Qemu-devel] [PATCH 10/14] tests: acpi: ignore SMBIOS tests when UEFI firmware is used

[Igor Mammedov]

On Wed, 16 Jan 2019 12:52:17 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > This approach allows the UEFI app source to live in the QEMU tree, and
> > the affected maintainer(s) would be personally responsible for setting
> > up their edk2 clones, and compilers. (The edk2 clone could even be a
> > submodule of QEMU, for example at roms/edk2.) For example,
> > "roms/Makefile" already calls an external EFIROM utility (also from
> > edk2) in order to build the combined iPXE option ROMs.
> > 
> > And yes, we could turn the UEFI binaries into bootable ISO images at once.
> > 
> > I'll try to post some patches soon (or not so soon). I think the app's
> > source code, and the edk2 submodule, should live under roms/, and the
> > bootable images should live under pc-bios/.
> > 
> > (In fact we could use this opportunity to build & bundle OVMF itself...
> > not sure if that's in scope for now. Gerd, what's your take?)  
> 
> Well, there is still the idea to move over firmware submodules and
> prebuilt firmware blobs to a separate repo.  Expermimental repo:
> https://git.kraxel.org/cgit/qemu-firmware/.  Not touched for more than a
> year due to being busy with other stuff.  Oh well ...
> 
> (if someone feels like picking this up feel free to do so).
> 
> I think adding edk2 as submodule below roms/ makes sense.  Adding rules
> to roms/Makefile to build the blobs makes sense too.  Not sure we want
> the binaries actually copied over to pc-bios/ and commited as the uefi
> firmware is pretty big ...
> 
> Not sure what a good place for the uefi app would be.  I'd tend to not
> use roms/, that is the place for firmware submodules.  contrib/ or test/
> maybe?
Could be tests/data/acpi in this case

> 
> cheers,
>   Gerd
> 

Re: [Qemu-devel] [PATCH 10/14] tests: acpi: ignore SMBIOS tests when UEFI firmware is used

[Michael S. Tsirkin]

On Wed, Jan 16, 2019 at 12:52:17PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > This approach allows the UEFI app source to live in the QEMU tree, and
> > the affected maintainer(s) would be personally responsible for setting
> > up their edk2 clones, and compilers. (The edk2 clone could even be a
> > submodule of QEMU, for example at roms/edk2.) For example,
> > "roms/Makefile" already calls an external EFIROM utility (also from
> > edk2) in order to build the combined iPXE option ROMs.
> > 
> > And yes, we could turn the UEFI binaries into bootable ISO images at once.
> > 
> > I'll try to post some patches soon (or not so soon). I think the app's
> > source code, and the edk2 submodule, should live under roms/, and the
> > bootable images should live under pc-bios/.
> > 
> > (In fact we could use this opportunity to build & bundle OVMF itself...
> > not sure if that's in scope for now. Gerd, what's your take?)
> 
> Well, there is still the idea to move over firmware submodules and
> prebuilt firmware blobs to a separate repo.

What's the advantage of this? 99% of people want the specific firmware
shipped with QEMU. Ony firmware developers might save a bit of disk
space. Seems like a loss overall.


>  Expermimental repo:
> https://git.kraxel.org/cgit/qemu-firmware/.  Not touched for more than a
> year due to being busy with other stuff.  Oh well ...
> 
> (if someone feels like picking this up feel free to do so).
> 
> I think adding edk2 as submodule below roms/ makes sense.  Adding rules
> to roms/Makefile to build the blobs makes sense too.  Not sure we want
> the binaries actually copied over to pc-bios/ and commited as the uefi
> firmware is pretty big ...
> 
> Not sure what a good place for the uefi app would be.  I'd tend to not
> use roms/, that is the place for firmware submodules.  contrib/ or test/
> maybe?
> 
> cheers,
>   Gerd

Re: [Qemu-devel] [PATCH 11/14] tests: acpi: add AVMF firmware blobs

[Michael S. Tsirkin]

On Wed, Jan 16, 2019 at 01:29:53PM +0100, Igor Mammedov wrote:
> On Tue, 15 Jan 2019 21:47:49 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
> > On 01/15/19 16:41, Igor Mammedov wrote:
> > > Add firmware blobs built with PcdAcpiTestSupport=TRUE,
> > > that puts RSDP address in RAM after 1Mb aligned GUID
> > >   AB87A6B1-2034-BDA0-71BD-375007757785
> > > so that tests could scan and find it in RAM once firmware's
> > > initialized ACPI tables.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  Makefile              |   3 ++-
> > >  pc-bios/avmf.img      | Bin 0 -> 2097152 bytes
> > >  pc-bios/avmf_vars.img | Bin 0 -> 786432 bytes
> > >  3 files changed, 2 insertions(+), 1 deletion(-)
> > >  create mode 100644 pc-bios/avmf.img
> > >  create mode 100644 pc-bios/avmf_vars.img  
> > 
> > "AVMF" is not a great name. "AAVMF" is a downstream name alright, but
> > many dislike it in upstream use. "edk2-aarch64" or "edk2-ArmVirtQemu"
> > would be more precise, but those are verbose. Sigh, why are names so
> > hard. What does everyone think?
> I'm fine with either version.
>  
> > Also, do you have to care about the license of firmware images built
> > from edk2 when bundling such images? Since edk2 commit 9a67ba261fe9
> > ("ArmVirtPkg: Replace obsoleted network drivers from platform DSC/FDF.",
> > 2018-12-14), you cannot build the ArmVirtQemu (aka AAVMF) firmware
> > without OpenSSL. Thus, the resultant license is 2-BSDL + OpenSSL -- for
> > now anyway.
> > 
> > Because, in the near future, that might change again. edk2 is in the
> > process of moving to Apache-2.0, and OpenSSL will also move (AFAICT) to
> > Apache-2.0 starting with release 3.0.0. (See commit 151333164ece,
> > "Change license to the Apache License v2.0", 2018-12-06.)
> That's another reason to look into EFI app approach (a simple one without
> dependencies) ans let distro to provide firmware image.

That will mean supporting very old firmware with the app.
I'm all for the EFI app approach for modularity
but I think we should include the batteries with this toy.


> > Thanks
> > Laszlo

Re: [Qemu-devel] [PATCH 10/14] tests: acpi: ignore SMBIOS tests when UEFI firmware is used

[Laszlo Ersek]

On 01/16/19 13:20, Igor Mammedov wrote:
> On Wed, 16 Jan 2019 12:09:03 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
>> On 01/16/19 12:07, Laszlo Ersek wrote:

>>> In particular, for this approach, we don't even need gnu-efi. (Because,
>>> personally, I would strongly prefer to write the UEFI application with
>>> the edk2 framework.) For a while now, edk2 has supported "multiple
>>> workspaces":
>>>
>>> https://github.com/tianocore/tianocore.github.io/wiki/Multiple_Workspace
>>>
>>> and as a result, it is possible to build the necessary UEFI app from:
>>> - an edk2 checkout that the maintainer has "somewhere" on their disk,
>>> - and the UEFI app source code that is contained in a QEMU checkout that
>>> the maintainer has "somewhere else" on their disk.
>>>
>>> This approach allows the UEFI app source to live in the QEMU tree, and
>>> the affected maintainer(s) would be personally responsible for setting
>>> up their edk2 clones, and compilers. (The edk2 clone could even be a
>>> submodule of QEMU, for example at roms/edk2.) For example,
>>> "roms/Makefile" already calls an external EFIROM utility (also from
>>> edk2) in order to build the combined iPXE option ROMs.
>>>
>>> And yes, we could turn the UEFI binaries into bootable ISO images at once.
>>>
>>> I'll try to post some patches soon (or not so soon). I think the app's
>>> source code, and the edk2 submodule, should live under roms/, and the
>>> bootable images should live under pc-bios/.
>>>
>>> (In fact we could use this opportunity to build & bundle OVMF itself...
>>> not sure if that's in scope for now. Gerd, what's your take?)  

>> Oh, I should add: the UEFI app in question could be built without
>> pulling in any OpenSSL bits; OVMF itself can't be built like that (it
>> now has a hard dependency on OpenSSL). This might matter from a
>> licensing/bundling perspective.

> That's why I've suggested non EDK2 variant, as we don't need very much
> from it and its license looks BDS like and it's very simple to build
> pretty much for everyone.
> Dealing with EDK2 to rebuild EFI is rather daunting prospect
> (at least from my point of view so I'd try to avoid it if possible)

Writing the app against edk2 is a lot simpler for me, and I have working
experience with cross-compiling edk2 stuff from x86_64 to aarch64 as well.

OTOH I have zero experience with gnu-efi, especially when it comes to
cross-compilation. System-level gnu-efi packages don't offer cross-built
binaries anyway, so we can't get around a git submodule -- whether it's
gnu-efi or edk2, we have to build from source.

And once we have a git submodule, I can put all the build commands in a
simple shell script.

Regarding OpenSSL, the edk2 metafiles for the EFI app will make it
evident that OpenSSL is not used. OpenSSL is anyway a git submodule of
edk2, so if *that* submodule is not inited -- which you can verify --,
then any OpenSSL references would fail to build. The resultant UEFI
binary will be covered by the 2-clause BSDL (from core edk2) and
whatever license we choose for the UEFI app itself (could be BSDL, could
be GPL, as you prefer).

Obviously I'm willing to take on maintenance for the EFI app and the
build script, if that works for you.

I understand the point of gnu-efi, but it's just unbearably limiting,
relative to the goodies in edk2.

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH 10/14] tests: acpi: ignore SMBIOS tests when UEFI firmware is used

[Laszlo Ersek]

On 01/16/19 13:31, Igor Mammedov wrote:
> On Wed, 16 Jan 2019 12:52:17 +0100
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
>>   Hi,
>>
>>> This approach allows the UEFI app source to live in the QEMU tree, and
>>> the affected maintainer(s) would be personally responsible for setting
>>> up their edk2 clones, and compilers. (The edk2 clone could even be a
>>> submodule of QEMU, for example at roms/edk2.) For example,
>>> "roms/Makefile" already calls an external EFIROM utility (also from
>>> edk2) in order to build the combined iPXE option ROMs.
>>>
>>> And yes, we could turn the UEFI binaries into bootable ISO images at once.
>>>
>>> I'll try to post some patches soon (or not so soon). I think the app's
>>> source code, and the edk2 submodule, should live under roms/, and the
>>> bootable images should live under pc-bios/.
>>>
>>> (In fact we could use this opportunity to build & bundle OVMF itself...
>>> not sure if that's in scope for now. Gerd, what's your take?)  
>>
>> Well, there is still the idea to move over firmware submodules and
>> prebuilt firmware blobs to a separate repo.  Expermimental repo:
>> https://git.kraxel.org/cgit/qemu-firmware/.  Not touched for more than a
>> year due to being busy with other stuff.  Oh well ...
>>
>> (if someone feels like picking this up feel free to do so).
>>
>> I think adding edk2 as submodule below roms/ makes sense.  Adding rules
>> to roms/Makefile to build the blobs makes sense too.  Not sure we want
>> the binaries actually copied over to pc-bios/ and commited as the uefi
>> firmware is pretty big ...
>>
>> Not sure what a good place for the uefi app would be.  I'd tend to not
>> use roms/, that is the place for firmware submodules.

I figured the source code for the UEFI app would fit due to the script
"configure-seabios.sh" and some actual config files being there already.
But, I'm happy to follow directions. :)

>>  contrib/ or test/ maybe?

> Could be tests/data/acpi in this case

If "tests/data/acpi" is appropriate for source code, that works for me.
We already have "rebuild-expected-aml.sh" there, so I guess another
build script and the UEFI app source code would fit there too.

It would be nice if I could get around submitting some patches this
week. Sigh. :/

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH 02/14] tests: acpi: make RSDT test routine handle XSDT

[Philippe Mathieu-Daudé]

On 1/15/19 4:40 PM, Igor Mammedov wrote:
> If RSDP revision is more than 0 fetch table pointed by XSDT
> and fallback to legacy RSDT table otherwise.
> 
> While at it drop unused acpi_get_xsdt_address().
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
> PS:
>  it doesn't affect existing pc/q35 machines as they use RSDP.revision == 0
>  but it will be used by followup patch to enable testing arm/virt
>  board which uses provides XSDT table.
> ---
>  tests/acpi-utils.h       |  3 +--
>  tests/acpi-utils.c       | 14 +-------------
>  tests/bios-tables-test.c | 18 +++++++++++++-----
>  3 files changed, 15 insertions(+), 20 deletions(-)
> 
> diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
> index 3b11f47..398900c 100644
> --- a/tests/acpi-utils.h
> +++ b/tests/acpi-utils.h
> @@ -47,8 +47,7 @@ typedef struct {
>  uint8_t acpi_calc_checksum(const uint8_t *data, int len);
>  uint32_t acpi_find_rsdp_address(QTestState *qts);
>  uint64_t uefi_find_rsdp_addr(QTestState *qts, uint64_t start, uint64_t size);
> -uint64_t acpi_get_xsdt_address(uint8_t *rsdp_table);
> -void acpi_parse_rsdp_table(QTestState *qts, uint32_t addr, uint8_t *rsdp_table);
> +void acpi_parse_rsdp_table(QTestState *qts, uint64_t addr, uint8_t *rsdp_table);
>  void acpi_fetch_table(QTestState *qts, uint8_t **aml, uint32_t *aml_len,
>                        const uint8_t *addr_ptr, const char *sig,
>                        bool verify_checksum);
> diff --git a/tests/acpi-utils.c b/tests/acpi-utils.c
> index b9ff9df..84068a8 100644
> --- a/tests/acpi-utils.c
> +++ b/tests/acpi-utils.c
> @@ -51,19 +51,7 @@ uint32_t acpi_find_rsdp_address(QTestState *qts)
>      return off;
>  }
>  
> -uint64_t acpi_get_xsdt_address(uint8_t *rsdp_table)
> -{
> -    uint64_t xsdt_physical_address;
> -    uint8_t revision = rsdp_table[15 /* Revision offset */];
> -
> -    /* We must have revision 2 if we're looking for an XSDT pointer */
> -    g_assert(revision == 2);
> -
> -    memcpy(&xsdt_physical_address, &rsdp_table[24 /* XsdtAddress offset */], 8);
> -    return le64_to_cpu(xsdt_physical_address);
> -}
> -
> -void acpi_parse_rsdp_table(QTestState *qts, uint32_t addr, uint8_t *rsdp_table)
> +void acpi_parse_rsdp_table(QTestState *qts, uint64_t addr, uint8_t *rsdp_table)
>  {
>      uint8_t revision;
>  
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 0bf7164..529bfc4 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -107,17 +107,25 @@ static void test_acpi_rsdp_table(test_data *data)
>      }
>  }
>  
> -static void test_acpi_rsdt_table(test_data *data)
> +static void test_acpi_rxsdt_table(test_data *data)
>  {
> +    const char *sig = "RSDT";
>      AcpiSdtTable rsdt = {};
> +    int entry_size = 4;
> +    int addr_off = 16 /* RsdtAddress */;
>      uint8_t *ent;
>  
> -    /* read RSDT table */
> +    if (data->rsdp_table[15 /* Revision offset */] != 0) {
> +        addr_off = 24 /* XsdtAddress */;
> +        entry_size = 8;
> +        sig = "XSDT";
> +    }
> +    /* read [RX]SDT table */
>      acpi_fetch_table(data->qts, &rsdt.aml, &rsdt.aml_len,
> -                     &data->rsdp_table[16 /* RsdtAddress */], "RSDT", true);
> +                     &data->rsdp_table[addr_off], sig, true);
>  
>      /* Load all tables and add to test list directly RSDT referenced tables */
> -    ACPI_FOREACH_RSDT_ENTRY(rsdt.aml, rsdt.aml_len, ent, 4 /* Entry size */) {
> +    ACPI_FOREACH_RSDT_ENTRY(rsdt.aml, rsdt.aml_len, ent, entry_size) {
>          AcpiSdtTable ssdt_table = {};
>  
>          acpi_fetch_table(data->qts, &ssdt_table.aml, &ssdt_table.aml_len, ent,
> @@ -519,7 +527,7 @@ static void test_acpi_one(const char *params, test_data *data)
>      data->tables = g_array_new(false, true, sizeof(AcpiSdtTable));
>      test_acpi_rsdp_address(data);
>      test_acpi_rsdp_table(data);
> -    test_acpi_rsdt_table(data);
> +    test_acpi_rxsdt_table(data);
>      test_acpi_fadt_table(data);
>  
>      if (iasl) {
> 

Re: [Qemu-devel] [PATCH 03/14] tests: acpi: rename acpi_parse_rsdp_table() into acpi_fetch_rsdp_table()

[Philippe Mathieu-Daudé]

On 1/15/19 4:40 PM, Igor Mammedov wrote:
> so name would reflect what the function does
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  tests/acpi-utils.h       | 2 +-
>  tests/acpi-utils.c       | 2 +-
>  tests/bios-tables-test.c | 2 +-
>  tests/vmgenid-test.c     | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
> index 398900c..59fb7d5 100644
> --- a/tests/acpi-utils.h
> +++ b/tests/acpi-utils.h
> @@ -47,7 +47,7 @@ typedef struct {
>  uint8_t acpi_calc_checksum(const uint8_t *data, int len);
>  uint32_t acpi_find_rsdp_address(QTestState *qts);
>  uint64_t uefi_find_rsdp_addr(QTestState *qts, uint64_t start, uint64_t size);
> -void acpi_parse_rsdp_table(QTestState *qts, uint64_t addr, uint8_t *rsdp_table);
> +void acpi_fetch_rsdp_table(QTestState *qts, uint64_t addr, uint8_t *rsdp_table);
>  void acpi_fetch_table(QTestState *qts, uint8_t **aml, uint32_t *aml_len,
>                        const uint8_t *addr_ptr, const char *sig,
>                        bool verify_checksum);
> diff --git a/tests/acpi-utils.c b/tests/acpi-utils.c
> index 84068a8..dce0fb7 100644
> --- a/tests/acpi-utils.c
> +++ b/tests/acpi-utils.c
> @@ -51,7 +51,7 @@ uint32_t acpi_find_rsdp_address(QTestState *qts)
>      return off;
>  }
>  
> -void acpi_parse_rsdp_table(QTestState *qts, uint64_t addr, uint8_t *rsdp_table)
> +void acpi_fetch_rsdp_table(QTestState *qts, uint64_t addr, uint8_t *rsdp_table)
>  {
>      uint8_t revision;
>  
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 529bfc4..99d7bf8 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -89,7 +89,7 @@ static void test_acpi_rsdp_table(test_data *data)
>      uint8_t *rsdp_table = data->rsdp_table, revision;
>      uint32_t addr = data->rsdp_addr;
>  
> -    acpi_parse_rsdp_table(data->qts, addr, rsdp_table);
> +    acpi_fetch_rsdp_table(data->qts, addr, rsdp_table);
>      revision = rsdp_table[15 /* Revision offset */];
>  
>      switch (revision) {
> diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
> index 52cdd83..27153a0 100644
> --- a/tests/vmgenid-test.c
> +++ b/tests/vmgenid-test.c
> @@ -40,7 +40,7 @@ static uint32_t acpi_find_vgia(QTestState *qts)
>      g_assert_cmphex(rsdp_offset, <, RSDP_ADDR_INVALID);
>  
>  
> -    acpi_parse_rsdp_table(qts, rsdp_offset, rsdp_table);
> +    acpi_fetch_rsdp_table(qts, rsdp_offset, rsdp_table);
>      acpi_fetch_table(qts, &rsdt, &rsdt_len, &rsdp_table[16 /* RsdtAddress */],
>                       "RSDT", true);
>  
> 

Re: [Qemu-devel] [PATCH 04/14] tests: acpi: make pointer to RSDP 64bit

[Philippe Mathieu-Daudé]

On 1/15/19 4:40 PM, Igor Mammedov wrote:
> In case of UEFI RSDP doesn't have to be located in lowmem,
> it could be placed at any address. Make sure that test won't
> break if it is placed above the first 4Gb of address space.
> 
> PS:
> While at it cleanup some local variables as we don't really
> need them.

Splitting as obvious patches make review quicker, but it's a matter of
taste.

> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  tests/bios-tables-test.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 99d7bf8..c28c5c7 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -26,7 +26,7 @@
>  typedef struct {
>      const char *machine;
>      const char *variant;
> -    uint32_t rsdp_addr;
> +    uint64_t rsdp_addr;
>      uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
>      GArray *tables;
>      uint32_t smbios_ep_addr;
> @@ -86,13 +86,11 @@ static void test_acpi_rsdp_address(test_data *data)
>  
>  static void test_acpi_rsdp_table(test_data *data)
>  {
> -    uint8_t *rsdp_table = data->rsdp_table, revision;
> -    uint32_t addr = data->rsdp_addr;
> +    uint8_t *rsdp_table = data->rsdp_table;
>  
> -    acpi_fetch_rsdp_table(data->qts, addr, rsdp_table);
> -    revision = rsdp_table[15 /* Revision offset */];
> +    acpi_fetch_rsdp_table(data->qts, data->rsdp_addr, rsdp_table);
>  
> -    switch (revision) {
> +    switch (rsdp_table[15 /* Revision offset */]) {
>      case 0: /* ACPI 1.0 RSDP */
>          /* With rev 1, checksum is only for the first 20 bytes */
>          g_assert(!acpi_calc_checksum(rsdp_table,  20));
> 

Re: [Qemu-devel] [PATCH 07/14] tests: acpi: skip FACS table if board uses hw reduced ACPI profile

[Philippe Mathieu-Daudé]

On 1/15/19 4:40 PM, Igor Mammedov wrote:
> If FADT has HW_REDUCED_ACPI flag set, do not attempt to fetch
> FACS as it's not provided by the board.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  tests/bios-tables-test.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 0f04a0a..8887319 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -146,9 +146,13 @@ static void test_acpi_fadt_table(test_data *data)
>      g_assert(compare_signature(&table, "FACP"));
>  
>      /* Since DSDT/FACS isn't in RSDT, add them to ASL test list manually */
> -    acpi_fetch_table(data->qts, &table.aml, &table.aml_len,
> -                     fadt_aml + 36 /* FIRMWARE_CTRL */, "FACS", false);
> -    g_array_append_val(data->tables, table);
> +    memcpy(&val, fadt_aml + 112 /* Flags */, 4);
> +    val = le32_to_cpu(val);
> +    if (!(val & 1UL << 20 /* HW_REDUCED_ACPI */)) {
> +        acpi_fetch_table(data->qts, &table.aml, &table.aml_len,
> +                         fadt_aml + 36 /* FIRMWARE_CTRL */, "FACS", false);
> +        g_array_append_val(data->tables, table);
> +    }
>  
>      memcpy(&val, fadt_aml + dsdt_offset, 4);
>      if (!val) {
> 

Re: [Qemu-devel] [PATCH 09/14] tests: acpi: move boot_sector_init() into x86 tests branch

[Philippe Mathieu-Daudé]

On 1/15/19 4:41 PM, Igor Mammedov wrote:
> boot_sector_init() won't be used by arm/virt board, so move it from
> global scope to x86 branch that uses it.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  tests/bios-tables-test.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index d290dd2..d9efe59 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -799,13 +799,13 @@ int main(int argc, char *argv[])
>      const char *arch = qtest_get_arch();
>      int ret;
>  
> -    ret = boot_sector_init(disk);
> -    if(ret)
> -        return ret;
> -
>      g_test_init(&argc, &argv, NULL);
>  
>      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> +        ret = boot_sector_init(disk);
> +        if(ret)
> +           return ret;
> +
>          qtest_add_func("acpi/piix4", test_acpi_piix4_tcg);
>          qtest_add_func("acpi/piix4/bridge", test_acpi_piix4_tcg_bridge);
>          qtest_add_func("acpi/q35", test_acpi_q35_tcg);
> 

Re: [Qemu-devel] [PATCH 14/14] tests: acpi: refactor rebuild-expected-aml.sh to dump ACPI tables for a specified list of targets

[Philippe Mathieu-Daudé]

On 1/15/19 4:41 PM, Igor Mammedov wrote:
> Make initial list contain aarch64 and x86_64 targets.

Maybe worth adding a line "remove i386"?

> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  tests/data/acpi/rebuild-expected-aml.sh | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/tests/data/acpi/rebuild-expected-aml.sh b/tests/data/acpi/rebuild-expected-aml.sh
> index bf9ba24..6287aa6 100755
> --- a/tests/data/acpi/rebuild-expected-aml.sh
> +++ b/tests/data/acpi/rebuild-expected-aml.sh
> @@ -7,21 +7,12 @@
>  #
>  # Authors:
>  #  Marcel Apfelbaum <marcel.a@redhat.com>
> +#  Igor Mammedov <imammedo@redhat.com>
>  #
>  # This work is licensed under the terms of the GNU GPLv2.
>  # See the COPYING.LIB file in the top-level directory.
>  
> -qemu=
> -
> -if [ -e x86_64-softmmu/qemu-system-x86_64 ]; then
> -    qemu="x86_64-softmmu/qemu-system-x86_64"
> -elif [ -e i386-softmmu/qemu-system-i386 ]; then
> -    qemu="i386-softmmu/qemu-system-i386"
> -else
> -    echo "Run 'make' to build the qemu exectutable!"
> -    echo "Run this script from the build directory."
> -    exit 1;
> -fi
> +qemu_bins="aarch64-softmmu/qemu-system-aarch64 x86_64-softmmu/qemu-system-x86_64"
>  
>  if [ ! -e "tests/bios-tables-test" ]; then
>      echo "Test: bios-tables-test is required! Run make check before this script."
> @@ -29,6 +20,14 @@ if [ ! -e "tests/bios-tables-test" ]; then
>      exit 1;
>  fi
>  
> -TEST_ACPI_REBUILD_AML=y QTEST_QEMU_BINARY=$qemu tests/bios-tables-test
> +for qemu in $qemu_bins; do
> +    if [ ! -e $qemu ]; then
> +        echo "Run 'make' to build following the qemu exectutables: $qemu_bins"

"to build the following QEMU executables: ..."

> +        echo "Run this script from the build directory."

Maybe "Also, run ..."

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +        exit 1;
> +    fi
> +    TEST_ACPI_REBUILD_AML=y QTEST_QEMU_BINARY=$qemu tests/bios-tables-test
> +done
> +
>  
>  echo "The files were rebuilt and can be added to git."
> 

Re: [Qemu-devel] [PATCH 11/14] tests: acpi: add AVMF firmware blobs

[Laszlo Ersek]

Hello Michael,

On 01/16/19 17:01, Michael S. Tsirkin wrote:
> On Wed, Jan 16, 2019 at 01:29:53PM +0100, Igor Mammedov wrote:
>> On Tue, 15 Jan 2019 21:47:49 +0100
>> Laszlo Ersek <lersek@redhat.com> wrote:
>>
>>> On 01/15/19 16:41, Igor Mammedov wrote:
>>>> Add firmware blobs built with PcdAcpiTestSupport=TRUE,
>>>> that puts RSDP address in RAM after 1Mb aligned GUID
>>>>   AB87A6B1-2034-BDA0-71BD-375007757785
>>>> so that tests could scan and find it in RAM once firmware's
>>>> initialized ACPI tables.
>>>>
>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>>> ---
>>>>  Makefile              |   3 ++-
>>>>  pc-bios/avmf.img      | Bin 0 -> 2097152 bytes
>>>>  pc-bios/avmf_vars.img | Bin 0 -> 786432 bytes
>>>>  3 files changed, 2 insertions(+), 1 deletion(-)
>>>>  create mode 100644 pc-bios/avmf.img
>>>>  create mode 100644 pc-bios/avmf_vars.img  
>>>
>>> "AVMF" is not a great name. "AAVMF" is a downstream name alright, but
>>> many dislike it in upstream use. "edk2-aarch64" or "edk2-ArmVirtQemu"
>>> would be more precise, but those are verbose. Sigh, why are names so
>>> hard. What does everyone think?
>> I'm fine with either version.
>>  
>>> Also, do you have to care about the license of firmware images built
>>> from edk2 when bundling such images? Since edk2 commit 9a67ba261fe9
>>> ("ArmVirtPkg: Replace obsoleted network drivers from platform DSC/FDF.",
>>> 2018-12-14), you cannot build the ArmVirtQemu (aka AAVMF) firmware
>>> without OpenSSL. Thus, the resultant license is 2-BSDL + OpenSSL -- for
>>> now anyway.
>>>
>>> Because, in the near future, that might change again. edk2 is in the
>>> process of moving to Apache-2.0, and OpenSSL will also move (AFAICT) to
>>> Apache-2.0 starting with release 3.0.0. (See commit 151333164ece,
>>> "Change license to the Apache License v2.0", 2018-12-06.)
>> That's another reason to look into EFI app approach (a simple one without
>> dependencies) ans let distro to provide firmware image.
> 
> That will mean supporting very old firmware with the app.
> I'm all for the EFI app approach for modularity
> but I think we should include the batteries with this toy.

There are two approaches here. (Both require that edk2 be present in the
QEMU source tree as a submodule, and both require QEMU to receive a
script for building edk2 in *some* way.)


(1) Carry the bios-tables-test helper UEFI app as an "out of tree"
module, from the perspective of edk2; carry it natively in the QEMU tree.

* Edk2 supports this use case as a first class citizen.

* In this case, the UEFI application is permitted to link only such
libraries from edk2 that the resultant binary inherit no platform
dependencies. The app can only be made dependent on a minimum UEFI spec
release, if necessary. The resultant binary can be run on any conformant
UEFI implementation, including physical hardware.

An example for "platform dependency prohibition" is that the X64 build
of the app could not print debug messages to the QEMU debug port (like
the rest of OVMF does), only to the UEFI console. On OVMF, that console
would direct the debug messages to the serial port and the graphics card.

Regarding a minimum UEFI spec release, the oldest UEFI spec I have on my
disk is "2.3.1, Errata C", dated "June 27, 2012". I'm not aware of
anything in the helper app that requires more recent spec features.

* Should we have to extend the UEFI app with a feature, we could easily
do that in sync with the consumer QEMU test code.

* The build output to commit to the QEMU repo would be an ISO image
containing the UEFI binary as a "boot loader". It would not contain
OpenSSL bits. The derived license would be a combination of core edk2's
license and our out-of-edk2-module's license.


(2) Carry the bios-tables-test helper *functionality* (not necessarily a
standalone UEFI application) in edk2; add a custom build flag to the
OVMF and ArmVirtQemu firmware platforms to enable the helper functionality.

* In this case, the test helper logic is permitted to rely on platform
internals. For example, on X64, it could print debug messages to the
QEMU debug port, like the rest of OVMF does.

* Whenever a new feature became necessary, edk2 would get new patches,
then QEMU would bump the submodule commit reference accordingly.

* The build output to commit to the QEMU repo would be a custom firmware
image (built with the special build flag mentioned above), and no other
bootable media would be formatted / saved. The custom firmware image
would contain OpenSSL bits. The image's license would be derived from
edk2 and from the OpenSSL submodule used by edk2.

* Also, the custom firmware image would not be suitable for "production"
use, so it couldn't be at once bundled under pc-bios as well -- that
would remain a separate RFE.


Since last night, I have some rough patches for (1), including the
QEMU-side Makefile + build script.

Regarding (2), I also have the edk2-native code ready (I had posted it a
few weeks back -- that's what Igor used now). For the QEMU side of
approach (2), I reckon I could reuse most of the build script I already
have for (1).

Could we please decide for (1) vs (2), before I put more work into (1)?

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 11/14] tests: acpi: add AVMF firmware blobs

[Laszlo Ersek]

On 01/17/19 09:53, Laszlo Ersek wrote:
> Hello Michael,
> 
> On 01/16/19 17:01, Michael S. Tsirkin wrote:
>> On Wed, Jan 16, 2019 at 01:29:53PM +0100, Igor Mammedov wrote:
>>> On Tue, 15 Jan 2019 21:47:49 +0100
>>> Laszlo Ersek <lersek@redhat.com> wrote:
>>>
>>>> On 01/15/19 16:41, Igor Mammedov wrote:
>>>>> Add firmware blobs built with PcdAcpiTestSupport=TRUE,
>>>>> that puts RSDP address in RAM after 1Mb aligned GUID
>>>>>   AB87A6B1-2034-BDA0-71BD-375007757785
>>>>> so that tests could scan and find it in RAM once firmware's
>>>>> initialized ACPI tables.
>>>>>
>>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>>>> ---
>>>>>  Makefile              |   3 ++-
>>>>>  pc-bios/avmf.img      | Bin 0 -> 2097152 bytes
>>>>>  pc-bios/avmf_vars.img | Bin 0 -> 786432 bytes
>>>>>  3 files changed, 2 insertions(+), 1 deletion(-)
>>>>>  create mode 100644 pc-bios/avmf.img
>>>>>  create mode 100644 pc-bios/avmf_vars.img  
>>>>
>>>> "AVMF" is not a great name. "AAVMF" is a downstream name alright, but
>>>> many dislike it in upstream use. "edk2-aarch64" or "edk2-ArmVirtQemu"
>>>> would be more precise, but those are verbose. Sigh, why are names so
>>>> hard. What does everyone think?
>>> I'm fine with either version.
>>>  
>>>> Also, do you have to care about the license of firmware images built
>>>> from edk2 when bundling such images? Since edk2 commit 9a67ba261fe9
>>>> ("ArmVirtPkg: Replace obsoleted network drivers from platform DSC/FDF.",
>>>> 2018-12-14), you cannot build the ArmVirtQemu (aka AAVMF) firmware
>>>> without OpenSSL. Thus, the resultant license is 2-BSDL + OpenSSL -- for
>>>> now anyway.
>>>>
>>>> Because, in the near future, that might change again. edk2 is in the
>>>> process of moving to Apache-2.0, and OpenSSL will also move (AFAICT) to
>>>> Apache-2.0 starting with release 3.0.0. (See commit 151333164ece,
>>>> "Change license to the Apache License v2.0", 2018-12-06.)
>>> That's another reason to look into EFI app approach (a simple one without
>>> dependencies) ans let distro to provide firmware image.
>>
>> That will mean supporting very old firmware with the app.
>> I'm all for the EFI app approach for modularity
>> but I think we should include the batteries with this toy.
> 
> There are two approaches here. (Both require that edk2 be present in the
> QEMU source tree as a submodule, and both require QEMU to receive a
> script for building edk2 in *some* way.)
> 
> 
> (1) Carry the bios-tables-test helper UEFI app as an "out of tree"
> module, from the perspective of edk2; carry it natively in the QEMU tree.
> 
> * Edk2 supports this use case as a first class citizen.
> 
> * In this case, the UEFI application is permitted to link only such
> libraries from edk2 that the resultant binary inherit no platform
> dependencies. The app can only be made dependent on a minimum UEFI spec
> release, if necessary. The resultant binary can be run on any conformant
> UEFI implementation, including physical hardware.
> 
> An example for "platform dependency prohibition" is that the X64 build
> of the app could not print debug messages to the QEMU debug port (like
> the rest of OVMF does), only to the UEFI console. On OVMF, that console
> would direct the debug messages to the serial port and the graphics card.
> 
> Regarding a minimum UEFI spec release, the oldest UEFI spec I have on my
> disk is "2.3.1, Errata C", dated "June 27, 2012". I'm not aware of
> anything in the helper app that requires more recent spec features.
> 
> * Should we have to extend the UEFI app with a feature, we could easily
> do that in sync with the consumer QEMU test code.
> 
> * The build output to commit to the QEMU repo would be an ISO image
> containing the UEFI binary as a "boot loader". It would not contain
> OpenSSL bits. The derived license would be a combination of core edk2's
> license and our out-of-edk2-module's license.
> 
> 
> (2) Carry the bios-tables-test helper *functionality* (not necessarily a
> standalone UEFI application) in edk2; add a custom build flag to the
> OVMF and ArmVirtQemu firmware platforms to enable the helper functionality.
> 
> * In this case, the test helper logic is permitted to rely on platform
> internals. For example, on X64, it could print debug messages to the
> QEMU debug port, like the rest of OVMF does.
> 
> * Whenever a new feature became necessary, edk2 would get new patches,
> then QEMU would bump the submodule commit reference accordingly.
> 
> * The build output to commit to the QEMU repo would be a custom firmware
> image (built with the special build flag mentioned above), and no other
> bootable media would be formatted / saved. The custom firmware image
> would contain OpenSSL bits. The image's license would be derived from
> edk2 and from the OpenSSL submodule used by edk2.
> 
> * Also, the custom firmware image would not be suitable for "production"
> use, so it couldn't be at once bundled under pc-bios as well -- that
> would remain a separate RFE.
> 
> 
> Since last night, I have some rough patches for (1), including the
> QEMU-side Makefile + build script.
> 
> Regarding (2), I also have the edk2-native code ready (I had posted it a
> few weeks back -- that's what Igor used now). For the QEMU side of
> approach (2), I reckon I could reuse most of the build script I already
> have for (1).
> 
> Could we please decide for (1) vs (2), before I put more work into (1)?

Sorry, borked English, I meant "decide between (1) and (2)". I didn't
intend to express a preference (I don't have one).

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH 11/14] tests: acpi: add AVMF firmware blobs

[Gerd Hoffmann]

  Hi,

> >>>>  create mode 100644 pc-bios/avmf.img
> >>>>  create mode 100644 pc-bios/avmf_vars.img  
> >>>
> >>> "AVMF" is not a great name. "AAVMF" is a downstream name alright, but
> >>> many dislike it in upstream use. "edk2-aarch64" or "edk2-ArmVirtQemu"
> >>> would be more precise, but those are verbose. Sigh, why are names so
> >>> hard. What does everyone think?
> >> I'm fine with either version.

How about placing them in pc-bios/efi-$arch subdirs and not renaming the
files, i.e. that would be ...

	pc-bios/efi-aarch64/QEMU_EFI.fd
	pc-bios/efi-aarch64/QEMU_VARS.fd

... for arm, and ...

	pc-bios/efi-x86_64/OVMF_CODE.fd
	pc-bios/efi-x86_64/OVMF_VARS.fd

... for x86.

> Could we please decide for (1) vs (2), before I put more work into (1)?

I'd tend to prefer (1).

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 11/14] tests: acpi: add AVMF firmware blobs

[Laszlo Ersek]

On 01/17/19 11:22, Gerd Hoffmann wrote:
>   Hi,
> 
>>>>>>  create mode 100644 pc-bios/avmf.img
>>>>>>  create mode 100644 pc-bios/avmf_vars.img  
>>>>>
>>>>> "AVMF" is not a great name. "AAVMF" is a downstream name alright, but
>>>>> many dislike it in upstream use. "edk2-aarch64" or "edk2-ArmVirtQemu"
>>>>> would be more precise, but those are verbose. Sigh, why are names so
>>>>> hard. What does everyone think?
>>>> I'm fine with either version.
> 
> How about placing them in pc-bios/efi-$arch subdirs and not renaming the
> files, i.e. that would be ...
> 
> 	pc-bios/efi-aarch64/QEMU_EFI.fd
> 	pc-bios/efi-aarch64/QEMU_VARS.fd
> 
> ... for arm, and ...
> 
> 	pc-bios/efi-x86_64/OVMF_CODE.fd
> 	pc-bios/efi-x86_64/OVMF_VARS.fd
> 
> ... for x86.

That sounds good to me. One thing to note is that the arm/aarch64 images
have to be padded to 64MB, so I generally append ".padded" to those file
names. Would that be OK? Any better ideas?

>> Could we please decide for (1) vs (2), before I put more work into (1)?
> 
> I'd tend to prefer (1).

Thanks. I have a patch set that's almost suitable for posting as an RFC.
I should split the last patch and write some sensible commit messages.

BTW, the bundling under pc-bios is a bit larger task than it immediately
appears:

- there are many build options to consider (as you know perfectly well
  :) ),

- plus now we have the "docs/interop/firmware.json" schema too, hence
  whatever images we build for end-user consumption, should likely be
  accompanied by metafiles that conform to this schema.

I think once we introduce the "roms/edk2" submodule for the current
purpose, we could address the pc-bios binaries (+metafiles) in a
separate series, on top.

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH 05/14] tests: acpi: fetch X_DSDT if pointer to DSDT is 0

[Philippe Mathieu-Daudé]

On 1/15/19 4:40 PM, Igor Mammedov wrote:
> that way it would be possible to test a DSDT pointed by
> 64bit X_DSDT field in FADT.
> 
> PS:
> it will allow to enable testing arm/virt board, which sets
> only newer X_DSDT field.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  tests/bios-tables-test.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index c28c5c7..0f04a0a 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -140,6 +140,8 @@ static void test_acpi_fadt_table(test_data *data)
>      AcpiSdtTable table = g_array_index(data->tables, typeof(table), 0);
>      uint8_t *fadt_aml = table.aml;
>      uint32_t fadt_len = table.aml_len;
> +    uint32_t val;
> +    int dsdt_offset = 40 /* DSDT */;
>  
>      g_assert(compare_signature(&table, "FACP"));
>  
> @@ -148,8 +150,12 @@ static void test_acpi_fadt_table(test_data *data)
>                       fadt_aml + 36 /* FIRMWARE_CTRL */, "FACS", false);
>      g_array_append_val(data->tables, table);
>  
> +    memcpy(&val, fadt_aml + dsdt_offset, 4);

Maybe in case someone extend this test, add:

       val = le32_to_cpu(val);

Regardless:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +    if (!val) {
> +        dsdt_offset = 140 /* X_DSDT */;
> +    }
>      acpi_fetch_table(data->qts, &table.aml, &table.aml_len,
> -                     fadt_aml + 40 /* DSDT */, "DSDT", true);
> +                     fadt_aml + dsdt_offset, "DSDT", true);
>      g_array_append_val(data->tables, table);
>  
>      memset(fadt_aml + 36, 0, 4); /* sanitize FIRMWARE_CTRL ptr */
> 

Re: [Qemu-devel] [PATCH 11/14] tests: acpi: add AVMF firmware blobs

[Gerd Hoffmann]

On Thu, Jan 17, 2019 at 01:54:51PM +0100, Laszlo Ersek wrote:
> On 01/17/19 11:22, Gerd Hoffmann wrote:
> >   Hi,
> > 
> >>>>>>  create mode 100644 pc-bios/avmf.img
> >>>>>>  create mode 100644 pc-bios/avmf_vars.img  
> >>>>>
> >>>>> "AVMF" is not a great name. "AAVMF" is a downstream name alright, but
> >>>>> many dislike it in upstream use. "edk2-aarch64" or "edk2-ArmVirtQemu"
> >>>>> would be more precise, but those are verbose. Sigh, why are names so
> >>>>> hard. What does everyone think?
> >>>> I'm fine with either version.
> > 
> > How about placing them in pc-bios/efi-$arch subdirs and not renaming the
> > files, i.e. that would be ...
> > 
> > 	pc-bios/efi-aarch64/QEMU_EFI.fd
> > 	pc-bios/efi-aarch64/QEMU_VARS.fd
> > 
> > ... for arm, and ...
> > 
> > 	pc-bios/efi-x86_64/OVMF_CODE.fd
> > 	pc-bios/efi-x86_64/OVMF_VARS.fd
> > 
> > ... for x86.
> 
> That sounds good to me. One thing to note is that the arm/aarch64 images
> have to be padded to 64MB, so I generally append ".padded" to those file
> names. Would that be OK? Any better ideas?

Ah, right, the arm versions can't be used as-is with pflash.  In my rpm
builds I've named the padded version "QEMU_EFI-pflash.raw".  Using
.padded looks fine to me too.

Other idea:  Does it make sense to use qcow2 for the pflash images?
i.e. "qemu-img create -f qcow2 -b QEMU_EFI.fd -F raw QEMU_EFI.qcow2 64M"?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 05/14] tests: acpi: fetch X_DSDT if pointer to DSDT is 0

[Igor Mammedov]

On Thu, 17 Jan 2019 15:02:57 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 1/15/19 4:40 PM, Igor Mammedov wrote:
> > that way it would be possible to test a DSDT pointed by
> > 64bit X_DSDT field in FADT.
> > 
> > PS:
> > it will allow to enable testing arm/virt board, which sets
> > only newer X_DSDT field.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  tests/bios-tables-test.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> > index c28c5c7..0f04a0a 100644
> > --- a/tests/bios-tables-test.c
> > +++ b/tests/bios-tables-test.c
> > @@ -140,6 +140,8 @@ static void test_acpi_fadt_table(test_data *data)
> >      AcpiSdtTable table = g_array_index(data->tables, typeof(table), 0);
> >      uint8_t *fadt_aml = table.aml;
> >      uint32_t fadt_len = table.aml_len;
> > +    uint32_t val;
> > +    int dsdt_offset = 40 /* DSDT */;
> >  
> >      g_assert(compare_signature(&table, "FACP"));
> >  
> > @@ -148,8 +150,12 @@ static void test_acpi_fadt_table(test_data *data)
> >                       fadt_aml + 36 /* FIRMWARE_CTRL */, "FACS", false);
> >      g_array_append_val(data->tables, table);
> >  
> > +    memcpy(&val, fadt_aml + dsdt_offset, 4);  
> 
> Maybe in case someone extend this test, add:
> 
>        val = le32_to_cpu(val);
I've skipped this conversion intentionally as we only care about
0 vs non 0. If we would care about concrete value then le32_to_cpu()
would be must have. I can put le32_to_cpu() there if you prefer
as it doesn't hurt and documents that value is in LE.

> Regardless:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> > +    if (!val) {
> > +        dsdt_offset = 140 /* X_DSDT */;
> > +    }
> >      acpi_fetch_table(data->qts, &table.aml, &table.aml_len,
> > -                     fadt_aml + 40 /* DSDT */, "DSDT", true);
> > +                     fadt_aml + dsdt_offset, "DSDT", true);
> >      g_array_append_val(data->tables, table);
> >  
> >      memset(fadt_aml + 36, 0, 4); /* sanitize FIRMWARE_CTRL ptr */
> >   
> 

Re: [Qemu-devel] [PATCH 10/14] tests: acpi: ignore SMBIOS tests when UEFI firmware is used

[Igor Mammedov]

On Wed, 16 Jan 2019 17:22:31 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> On 01/16/19 13:31, Igor Mammedov wrote:
> > On Wed, 16 Jan 2019 12:52:17 +0100
> > Gerd Hoffmann <kraxel@redhat.com> wrote:
> >   
> >>   Hi,
> >>  
> >>> This approach allows the UEFI app source to live in the QEMU tree, and
> >>> the affected maintainer(s) would be personally responsible for setting
> >>> up their edk2 clones, and compilers. (The edk2 clone could even be a
> >>> submodule of QEMU, for example at roms/edk2.) For example,
> >>> "roms/Makefile" already calls an external EFIROM utility (also from
> >>> edk2) in order to build the combined iPXE option ROMs.
> >>>
> >>> And yes, we could turn the UEFI binaries into bootable ISO images at once.
> >>>
> >>> I'll try to post some patches soon (or not so soon). I think the app's
> >>> source code, and the edk2 submodule, should live under roms/, and the
> >>> bootable images should live under pc-bios/.
> >>>
> >>> (In fact we could use this opportunity to build & bundle OVMF itself...
> >>> not sure if that's in scope for now. Gerd, what's your take?)    
> >>
> >> Well, there is still the idea to move over firmware submodules and
> >> prebuilt firmware blobs to a separate repo.  Expermimental repo:
> >> https://git.kraxel.org/cgit/qemu-firmware/.  Not touched for more than a
> >> year due to being busy with other stuff.  Oh well ...
> >>
> >> (if someone feels like picking this up feel free to do so).
> >>
> >> I think adding edk2 as submodule below roms/ makes sense.  Adding rules
> >> to roms/Makefile to build the blobs makes sense too.  Not sure we want
> >> the binaries actually copied over to pc-bios/ and commited as the uefi
> >> firmware is pretty big ...
> >>
> >> Not sure what a good place for the uefi app would be.  I'd tend to not
> >> use roms/, that is the place for firmware submodules.  
> 
> I figured the source code for the UEFI app would fit due to the script
> "configure-seabios.sh" and some actual config files being there already.
> But, I'm happy to follow directions. :)
> 
> >>  contrib/ or test/ maybe?  
> 
> > Could be tests/data/acpi in this case  
> 
> If "tests/data/acpi" is appropriate for source code, that works for me.
> We already have "rebuild-expected-aml.sh" there, so I guess another
> build script and the UEFI app source code would fit there too.
I suggested tests/data/acpi for binary EFI app blobs
For EFI app source maybe just 'tests' or sub-directory there and make
'make check-efi-app' build EFI app (doing all needed magic and asking for things that missing)
if source is newer than blobs.

> 
> It would be nice if I could get around submitting some patches this
> week. Sigh. :/
> 
> Thanks,
> Laszlo
> 

Re: [Qemu-devel] [PATCH 10/14] tests: acpi: ignore SMBIOS tests when UEFI firmware is used

[Igor Mammedov]

On Wed, 16 Jan 2019 17:17:23 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> On 01/16/19 13:20, Igor Mammedov wrote:
> > On Wed, 16 Jan 2019 12:09:03 +0100
> > Laszlo Ersek <lersek@redhat.com> wrote:  
> >> On 01/16/19 12:07, Laszlo Ersek wrote:  
> 
> >>> In particular, for this approach, we don't even need gnu-efi. (Because,
> >>> personally, I would strongly prefer to write the UEFI application with
> >>> the edk2 framework.) For a while now, edk2 has supported "multiple
> >>> workspaces":
> >>>
> >>> https://github.com/tianocore/tianocore.github.io/wiki/Multiple_Workspace
> >>>
> >>> and as a result, it is possible to build the necessary UEFI app from:
> >>> - an edk2 checkout that the maintainer has "somewhere" on their disk,
> >>> - and the UEFI app source code that is contained in a QEMU checkout that
> >>> the maintainer has "somewhere else" on their disk.
> >>>
> >>> This approach allows the UEFI app source to live in the QEMU tree, and
> >>> the affected maintainer(s) would be personally responsible for setting
> >>> up their edk2 clones, and compilers. (The edk2 clone could even be a
> >>> submodule of QEMU, for example at roms/edk2.) For example,
> >>> "roms/Makefile" already calls an external EFIROM utility (also from
> >>> edk2) in order to build the combined iPXE option ROMs.
> >>>
> >>> And yes, we could turn the UEFI binaries into bootable ISO images at once.
> >>>
> >>> I'll try to post some patches soon (or not so soon). I think the app's
> >>> source code, and the edk2 submodule, should live under roms/, and the
> >>> bootable images should live under pc-bios/.
> >>>
> >>> (In fact we could use this opportunity to build & bundle OVMF itself...
> >>> not sure if that's in scope for now. Gerd, what's your take?)    
> 
> >> Oh, I should add: the UEFI app in question could be built without
> >> pulling in any OpenSSL bits; OVMF itself can't be built like that (it
> >> now has a hard dependency on OpenSSL). This might matter from a
> >> licensing/bundling perspective.  
> 
> > That's why I've suggested non EDK2 variant, as we don't need very much
> > from it and its license looks BDS like and it's very simple to build
> > pretty much for everyone.
> > Dealing with EDK2 to rebuild EFI is rather daunting prospect
> > (at least from my point of view so I'd try to avoid it if possible)  
> 
> Writing the app against edk2 is a lot simpler for me, and I have working
> experience with cross-compiling edk2 stuff from x86_64 to aarch64 as well.
> 
> OTOH I have zero experience with gnu-efi, especially when it comes to
> cross-compilation. System-level gnu-efi packages don't offer cross-built
> binaries anyway, so we can't get around a git submodule -- whether it's
> gnu-efi or edk2, we have to build from source.
> 
> And once we have a git submodule, I can put all the build commands in a
> simple shell script.
> 
> Regarding OpenSSL, the edk2 metafiles for the EFI app will make it
> evident that OpenSSL is not used. OpenSSL is anyway a git submodule of
> edk2, so if *that* submodule is not inited -- which you can verify --,
> then any OpenSSL references would fail to build. The resultant UEFI
> binary will be covered by the 2-clause BSDL (from core edk2) and
> whatever license we choose for the UEFI app itself (could be BSDL, could
> be GPL, as you prefer).
> 
> Obviously I'm willing to take on maintenance for the EFI app and the
> build script, if that works for you.
That works for me too, it would be better if anyone would be able to do
rebuild as well (i.e. some in tree magic script/make target that would
do rebuild when it's executed and ask for necessary deps if something
is missing)

> I understand the point of gnu-efi, but it's just unbearably limiting,
> relative to the goodies in edk2.
The project I've pointed out is gnu-efi less (it borrowed some linking magic
and some protocol definitions from gnu-efi), so it's even more limited
but we do not need even most of what it provides. So I was thinking that
would benefit you and a random contributor as well.


> Thanks,
> Laszlo

Re: [Qemu-devel] [PATCH 14/14] tests: acpi: refactor rebuild-expected-aml.sh to dump ACPI tables for a specified list of targets

[Igor Mammedov]

On Wed, 16 Jan 2019 18:08:17 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 1/15/19 4:41 PM, Igor Mammedov wrote:
> > Make initial list contain aarch64 and x86_64 targets.  
> 
> Maybe worth adding a line "remove i386"?
I've dropped i386 as it's redundant, since x86_64 target re-builds the same tables.
And maintainer is likely to build all targets when preparing update/pull req.

> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  tests/data/acpi/rebuild-expected-aml.sh | 23 +++++++++++------------
> >  1 file changed, 11 insertions(+), 12 deletions(-)
> > 
> > diff --git a/tests/data/acpi/rebuild-expected-aml.sh b/tests/data/acpi/rebuild-expected-aml.sh
> > index bf9ba24..6287aa6 100755
> > --- a/tests/data/acpi/rebuild-expected-aml.sh
> > +++ b/tests/data/acpi/rebuild-expected-aml.sh
> > @@ -7,21 +7,12 @@
> >  #
> >  # Authors:
> >  #  Marcel Apfelbaum <marcel.a@redhat.com>
> > +#  Igor Mammedov <imammedo@redhat.com>
> >  #
> >  # This work is licensed under the terms of the GNU GPLv2.
> >  # See the COPYING.LIB file in the top-level directory.
> >  
> > -qemu=
> > -
> > -if [ -e x86_64-softmmu/qemu-system-x86_64 ]; then
> > -    qemu="x86_64-softmmu/qemu-system-x86_64"
> > -elif [ -e i386-softmmu/qemu-system-i386 ]; then
> > -    qemu="i386-softmmu/qemu-system-i386"
> > -else
> > -    echo "Run 'make' to build the qemu exectutable!"
> > -    echo "Run this script from the build directory."
> > -    exit 1;
> > -fi
> > +qemu_bins="aarch64-softmmu/qemu-system-aarch64 x86_64-softmmu/qemu-system-x86_64"
> >  
> >  if [ ! -e "tests/bios-tables-test" ]; then
> >      echo "Test: bios-tables-test is required! Run make check before this script."
> > @@ -29,6 +20,14 @@ if [ ! -e "tests/bios-tables-test" ]; then
> >      exit 1;
> >  fi
> >  
> > -TEST_ACPI_REBUILD_AML=y QTEST_QEMU_BINARY=$qemu tests/bios-tables-test
> > +for qemu in $qemu_bins; do
> > +    if [ ! -e $qemu ]; then
> > +        echo "Run 'make' to build following the qemu exectutables: $qemu_bins"  
> 
> "to build the following QEMU executables: ..."
> 
> > +        echo "Run this script from the build directory."  
> 
> Maybe "Also, run ..."
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Thanks,
I'll add the corrections on respin.

> 
> > +        exit 1;
> > +    fi
> > +    TEST_ACPI_REBUILD_AML=y QTEST_QEMU_BINARY=$qemu tests/bios-tables-test
> > +done
> > +
> >  
> >  echo "The files were rebuilt and can be added to git."
> >   

Re: [Qemu-devel] [PATCH 11/14] tests: acpi: add AVMF firmware blobs

[Igor Mammedov]

On Thu, 17 Jan 2019 13:54:51 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> On 01/17/19 11:22, Gerd Hoffmann wrote:
> >   Hi,
> >   
> >>>>>>  create mode 100644 pc-bios/avmf.img
> >>>>>>  create mode 100644 pc-bios/avmf_vars.img    
> >>>>>
> >>>>> "AVMF" is not a great name. "AAVMF" is a downstream name alright, but
> >>>>> many dislike it in upstream use. "edk2-aarch64" or "edk2-ArmVirtQemu"
> >>>>> would be more precise, but those are verbose. Sigh, why are names so
> >>>>> hard. What does everyone think?  
> >>>> I'm fine with either version.  
> > 
> > How about placing them in pc-bios/efi-$arch subdirs and not renaming the
> > files, i.e. that would be ...
> > 
> > 	pc-bios/efi-aarch64/QEMU_EFI.fd
> > 	pc-bios/efi-aarch64/QEMU_VARS.fd
> > 
> > ... for arm, and ...
> > 
> > 	pc-bios/efi-x86_64/OVMF_CODE.fd
> > 	pc-bios/efi-x86_64/OVMF_VARS.fd
> > 
> > ... for x86.  
if it's non production images (i.e. openssl-less) than maybe use
tests/data/acpi instead of pc-bios for now, once we have pc-bios ones
ready we drop test specific and use production ones.

> That sounds good to me. One thing to note is that the arm/aarch64 images
> have to be padded to 64MB, so I generally append ".padded" to those file
> names. Would that be OK? Any better ideas?
Images could be pre-padded and ready for commit, wrt large size we can commit
them compressed and decompress for using in tests instead of padding padding
I'm doing now before I use them.

> >> Could we please decide for (1) vs (2), before I put more work into (1)?  
> > 
> > I'd tend to prefer (1).  

+1

> 
> Thanks. I have a patch set that's almost suitable for posting as an RFC.
> I should split the last patch and write some sensible commit messages.
> 
> BTW, the bundling under pc-bios is a bit larger task than it immediately
> appears:
> 
> - there are many build options to consider (as you know perfectly well
>   :) ),
> 
> - plus now we have the "docs/interop/firmware.json" schema too, hence
>   whatever images we build for end-user consumption, should likely be
>   accompanied by metafiles that conform to this schema.
> 
> I think once we introduce the "roms/edk2" submodule for the current
> purpose, we could address the pc-bios binaries (+metafiles) in a
> separate series, on top.
> 
> Thanks,
> Laszlo
> 

Re: [Qemu-devel] [PATCH 11/14] tests: acpi: add AVMF firmware blobs

[Laszlo Ersek]

On 01/17/19 15:09, Gerd Hoffmann wrote:
> On Thu, Jan 17, 2019 at 01:54:51PM +0100, Laszlo Ersek wrote:
>> On 01/17/19 11:22, Gerd Hoffmann wrote:
>>>   Hi,
>>>
>>>>>>>>  create mode 100644 pc-bios/avmf.img
>>>>>>>>  create mode 100644 pc-bios/avmf_vars.img  
>>>>>>>
>>>>>>> "AVMF" is not a great name. "AAVMF" is a downstream name alright, but
>>>>>>> many dislike it in upstream use. "edk2-aarch64" or "edk2-ArmVirtQemu"
>>>>>>> would be more precise, but those are verbose. Sigh, why are names so
>>>>>>> hard. What does everyone think?
>>>>>> I'm fine with either version.
>>>
>>> How about placing them in pc-bios/efi-$arch subdirs and not renaming the
>>> files, i.e. that would be ...
>>>
>>> 	pc-bios/efi-aarch64/QEMU_EFI.fd
>>> 	pc-bios/efi-aarch64/QEMU_VARS.fd
>>>
>>> ... for arm, and ...
>>>
>>> 	pc-bios/efi-x86_64/OVMF_CODE.fd
>>> 	pc-bios/efi-x86_64/OVMF_VARS.fd
>>>
>>> ... for x86.
>>
>> That sounds good to me. One thing to note is that the arm/aarch64 images
>> have to be padded to 64MB, so I generally append ".padded" to those file
>> names. Would that be OK? Any better ideas?
> 
> Ah, right, the arm versions can't be used as-is with pflash.  In my rpm
> builds I've named the padded version "QEMU_EFI-pflash.raw".  Using
> .padded looks fine to me too.
> 
> Other idea:  Does it make sense to use qcow2 for the pflash images?
> i.e. "qemu-img create -f qcow2 -b QEMU_EFI.fd -F raw QEMU_EFI.qcow2 64M"?

That's a long story.

In a perfect world, the answer would be, "qcow2 (and all its features)
make perfect sense for pflash images". In the world we have however,
"savevm" exists, which might dump live guest RAM into whichever qcow2
disk it finds first. See
<https://bugzilla.redhat.com/show_bug.cgi?id=1214187>.

(I apologize to any non-RedHatters reading this; that's a private RHBZ
for some obscure reason, and I dare not open it up myself.)

See also libvirt commit 9e2465834f4b ("qemu: snapshot: Forbid internal
snapshots with pflash firmware", 2017-03-24).

So, for now, it remains the case that we're better off with raw, at
least for the writeable (=varstore) pflash chip.

In addition, when libvirt composes the cmdline, it specifies format=raw
for unit=0 (i.e., firmware image pflash) as well. (That's actually a
good thing, because we shouldn't auto-detect the format, and qcow2 is
out (for now), so we should specify format=raw. In the future, the
domain XML schema may have to be extended with "format" too.)

... For now, it's probably best to check the unpadded FDs into git, and
pad them only in "make install".

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH 10/14] tests: acpi: ignore SMBIOS tests when UEFI firmware is used

[Laszlo Ersek]

On 01/17/19 16:11, Igor Mammedov wrote:
> On Wed, 16 Jan 2019 17:22:31 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 01/16/19 13:31, Igor Mammedov wrote:
>>> On Wed, 16 Jan 2019 12:52:17 +0100
>>> Gerd Hoffmann <kraxel@redhat.com> wrote:
>>>   
>>>>   Hi,
>>>>  
>>>>> This approach allows the UEFI app source to live in the QEMU tree, and
>>>>> the affected maintainer(s) would be personally responsible for setting
>>>>> up their edk2 clones, and compilers. (The edk2 clone could even be a
>>>>> submodule of QEMU, for example at roms/edk2.) For example,
>>>>> "roms/Makefile" already calls an external EFIROM utility (also from
>>>>> edk2) in order to build the combined iPXE option ROMs.
>>>>>
>>>>> And yes, we could turn the UEFI binaries into bootable ISO images at once.
>>>>>
>>>>> I'll try to post some patches soon (or not so soon). I think the app's
>>>>> source code, and the edk2 submodule, should live under roms/, and the
>>>>> bootable images should live under pc-bios/.
>>>>>
>>>>> (In fact we could use this opportunity to build & bundle OVMF itself...
>>>>> not sure if that's in scope for now. Gerd, what's your take?)    
>>>>
>>>> Well, there is still the idea to move over firmware submodules and
>>>> prebuilt firmware blobs to a separate repo.  Expermimental repo:
>>>> https://git.kraxel.org/cgit/qemu-firmware/.  Not touched for more than a
>>>> year due to being busy with other stuff.  Oh well ...
>>>>
>>>> (if someone feels like picking this up feel free to do so).
>>>>
>>>> I think adding edk2 as submodule below roms/ makes sense.  Adding rules
>>>> to roms/Makefile to build the blobs makes sense too.  Not sure we want
>>>> the binaries actually copied over to pc-bios/ and commited as the uefi
>>>> firmware is pretty big ...
>>>>
>>>> Not sure what a good place for the uefi app would be.  I'd tend to not
>>>> use roms/, that is the place for firmware submodules.  
>>
>> I figured the source code for the UEFI app would fit due to the script
>> "configure-seabios.sh" and some actual config files being there already.
>> But, I'm happy to follow directions. :)
>>
>>>>  contrib/ or test/ maybe?  
>>
>>> Could be tests/data/acpi in this case  
>>
>> If "tests/data/acpi" is appropriate for source code, that works for me.
>> We already have "rebuild-expected-aml.sh" there, so I guess another
>> build script and the UEFI app source code would fit there too.
> I suggested tests/data/acpi for binary EFI app blobs
> For EFI app source maybe just 'tests' or sub-directory there and make
> 'make check-efi-app' build EFI app (doing all needed magic and asking for things that missing)
> if source is newer than blobs.

I've been busy all day hacking and staying away from email, so when you see that my patches didn't follow this closely in the end, please know that I didn't deliberately ignore your point -- I'm just getting to it now. Let's continue the discussion under

  [qemu-devel] [PATCH 0/5] add the BiosTablesTest UEFI app, build it with the new roms/edk2 submodule

Thanks!
Laszlo

> 
>>
>> It would be nice if I could get around submitting some patches this
>> week. Sigh. :/
>>
>> Thanks,
>> Laszlo
>>
>