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 <address@hidden>
> ---
>  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