Message ID | 1547566866-129386-9-git-send-email-imammedo@redhat.com |
---|---|
State | New |
Headers | show |
Series | tests: acpi: add UEFI (ARM) testing support | expand |
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
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
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);
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(-)