[for,4.1,v2,08/13] tests: acpi: add a way to start tests with UEFI firmware

For testcase to use UEFI firmware, one needs to provide and specify
firmwarei and varstore blobs names in test_data { uefi_fl1, uefi_fl2) }
fields respectively and RAM start address plus size where to look for
test structure signature. Additionally testcase should specify
bootable cdrom image uefi-boot-images EFI test utility.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
  * drop data_dir prefix and firmware will come from pc-bios directly
  * add cdrom option so test could use it for providing boot cdrom image
v2:
  * move RAM start address and size to test_data, as it could differ
    between boards (and even versions)
---
 tests/bios-tables-test.c | 40 ++++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

On 03/26/19 14:09, Igor Mammedov wrote:
> For testcase to use UEFI firmware, one needs to provide and specify
> firmwarei and varstore blobs names in test_data { uefi_fl1, uefi_fl2) }
> fields respectively and RAM start address plus size where to look for
> test structure signature. Additionally testcase should specify
> bootable cdrom image uefi-boot-images EFI test utility.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v3:
>   * drop data_dir prefix and firmware will come from pc-bios directly
>   * add cdrom option so test could use it for providing boot cdrom image
> v2:
>   * move RAM start address and size to test_data, as it could differ
>     between boards (and even versions)
> ---
>  tests/bios-tables-test.c | 40 ++++++++++++++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 53d4379..91846d6 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -26,6 +26,11 @@
>  typedef struct {
>      const char *machine;
>      const char *variant;
> +    const char *uefi_fl1;
> +    const char *uefi_fl2;
> +    const char *cd;
> +    const uint64_t ram_start;
> +    const uint64_t scan_len;
>      uint64_t rsdp_addr;
>      uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
>      GArray *tables;
> @@ -522,21 +527,36 @@ 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,readonly "
> +            "-drive if=pflash,format=raw,file=%s,snapshot=on -cdrom %s %s",
> +            data->machine, "kvm:tcg", data->uefi_fl1, data->uefi_fl2,
> +            data->cd, 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) {
> +        g_assert(data->scan_len);
> +        data->rsdp_addr = acpi_find_rsdp_address_uefi(data->qts,
> +            data->ram_start, data->scan_len);
> +    } 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);
> 

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

Later on, we should convert the "-drive if=pflash" options to the new
syntax (see commit e33763be7cd3). However, I think that syntax doesn't
work for the aarch64/virt board yet. (Should we file a Launchpad item
about this, as a reminder?)

Thanks,
Laszlo

On Tue, 26 Mar 2019 15:16:57 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> On 03/26/19 14:09, Igor Mammedov wrote:
> > For testcase to use UEFI firmware, one needs to provide and specify
> > firmwarei and varstore blobs names in test_data { uefi_fl1, uefi_fl2) }
> > fields respectively and RAM start address plus size where to look for
> > test structure signature. Additionally testcase should specify
> > bootable cdrom image uefi-boot-images EFI test utility.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v3:
> >   * drop data_dir prefix and firmware will come from pc-bios directly
> >   * add cdrom option so test could use it for providing boot cdrom image
> > v2:
> >   * move RAM start address and size to test_data, as it could differ
> >     between boards (and even versions)
> > ---
> >  tests/bios-tables-test.c | 40 ++++++++++++++++++++++++++++++----------
> >  1 file changed, 30 insertions(+), 10 deletions(-)
> > 
> > diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> > index 53d4379..91846d6 100644
> > --- a/tests/bios-tables-test.c
> > +++ b/tests/bios-tables-test.c
> > @@ -26,6 +26,11 @@
> >  typedef struct {
> >      const char *machine;
> >      const char *variant;
> > +    const char *uefi_fl1;
> > +    const char *uefi_fl2;
> > +    const char *cd;
> > +    const uint64_t ram_start;
> > +    const uint64_t scan_len;
> >      uint64_t rsdp_addr;
> >      uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
> >      GArray *tables;
> > @@ -522,21 +527,36 @@ 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,readonly "
> > +            "-drive if=pflash,format=raw,file=%s,snapshot=on -cdrom %s %s",
> > +            data->machine, "kvm:tcg", data->uefi_fl1, data->uefi_fl2,
> > +            data->cd, 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) {
> > +        g_assert(data->scan_len);
> > +        data->rsdp_addr = acpi_find_rsdp_address_uefi(data->qts,
> > +            data->ram_start, data->scan_len);
> > +    } 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);
> >   
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Later on, we should convert the "-drive if=pflash" options to the new
> syntax (see commit e33763be7cd3). However, I think that syntax doesn't
> work for the aarch64/virt board yet. (Should we file a Launchpad item
> about this, as a reminder?)
I tend not to use LP (last time was in 2015) so I'm not sure if it would
be of any help. Maybe adding a TODO comment in code would be better.
Something like:

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index dc52a65..b912684 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -533,6 +533,10 @@ static void test_acpi_one(const char *params, test_data *data)
     bool use_uefi = data->uefi_fl1 && data->uefi_fl2;
 
     if (use_uefi) {
+        /*
+         * TODO: convert '-drive if=pflash' to new syntax (see e33763be7cd3)
+         * when arm/virt boad starts to support it.
+         */
         args = g_strdup_printf("-machine %s,accel=%s -nodefaults -nographic "
             "-drive if=pflash,format=raw,file=%s,readonly "
             "-drive if=pflash,format=raw,file=%s,snapshot=on -cdrom %s %s",


> Thanks,
> Laszlo

On 03/27/19 11:09, Igor Mammedov wrote:
> On Tue, 26 Mar 2019 15:16:57 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 03/26/19 14:09, Igor Mammedov wrote:
>>> For testcase to use UEFI firmware, one needs to provide and specify
>>> firmwarei and varstore blobs names in test_data { uefi_fl1, uefi_fl2) }
>>> fields respectively and RAM start address plus size where to look for
>>> test structure signature. Additionally testcase should specify
>>> bootable cdrom image uefi-boot-images EFI test utility.
>>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>> v3:
>>>   * drop data_dir prefix and firmware will come from pc-bios directly
>>>   * add cdrom option so test could use it for providing boot cdrom image
>>> v2:
>>>   * move RAM start address and size to test_data, as it could differ
>>>     between boards (and even versions)
>>> ---
>>>  tests/bios-tables-test.c | 40 ++++++++++++++++++++++++++++++----------
>>>  1 file changed, 30 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
>>> index 53d4379..91846d6 100644
>>> --- a/tests/bios-tables-test.c
>>> +++ b/tests/bios-tables-test.c
>>> @@ -26,6 +26,11 @@
>>>  typedef struct {
>>>      const char *machine;
>>>      const char *variant;
>>> +    const char *uefi_fl1;
>>> +    const char *uefi_fl2;
>>> +    const char *cd;
>>> +    const uint64_t ram_start;
>>> +    const uint64_t scan_len;
>>>      uint64_t rsdp_addr;
>>>      uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
>>>      GArray *tables;
>>> @@ -522,21 +527,36 @@ 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,readonly "
>>> +            "-drive if=pflash,format=raw,file=%s,snapshot=on -cdrom %s %s",
>>> +            data->machine, "kvm:tcg", data->uefi_fl1, data->uefi_fl2,
>>> +            data->cd, 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) {
>>> +        g_assert(data->scan_len);
>>> +        data->rsdp_addr = acpi_find_rsdp_address_uefi(data->qts,
>>> +            data->ram_start, data->scan_len);
>>> +    } 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);
>>>   
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Later on, we should convert the "-drive if=pflash" options to the new
>> syntax (see commit e33763be7cd3). However, I think that syntax doesn't
>> work for the aarch64/virt board yet. (Should we file a Launchpad item
>> about this, as a reminder?)
> I tend not to use LP (last time was in 2015) so I'm not sure if it would
> be of any help. Maybe adding a TODO comment in code would be better.
> Something like:
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index dc52a65..b912684 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -533,6 +533,10 @@ static void test_acpi_one(const char *params, test_data *data)
>      bool use_uefi = data->uefi_fl1 && data->uefi_fl2;
>  
>      if (use_uefi) {
> +        /*
> +         * TODO: convert '-drive if=pflash' to new syntax (see e33763be7cd3)
> +         * when arm/virt boad starts to support it.
> +         */
>          args = g_strdup_printf("-machine %s,accel=%s -nodefaults -nographic "
>              "-drive if=pflash,format=raw,file=%s,readonly "
>              "-drive if=pflash,format=raw,file=%s,snapshot=on -cdrom %s %s",
> 
> 
>> Thanks,
>> Laszlo
> 

That looks good too, thanks!
Laszlo