[V5,2/7] tests/acpi: add pxb/pxb-pcie tests

Commit Message

Marcel Apfelbaum July 17, 2016, 4:53 p.m. UTC

Add an ivshmem device with 1G shared memory to
pxb in order to check the ACPI code of 64bit MMIO allocation.

Suggested-by: Igor Mammedov <[e-mail]>
Signed-off-by: Marcel Apfelbaum <[e-mail]>
Tested-by: Laszlo Ersek <[e-mail]>
---
 tests/bios-tables-test.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

Comments

Igor Mammedov July 18, 2016, 1:34 p.m. UTC | #1

On Sun, 17 Jul 2016 19:53:09 +0300
Marcel Apfelbaum <[e-mail]> wrote:

> Add an ivshmem device with 1G shared memory to
> pxb in order to check the ACPI code of 64bit MMIO allocation.
what is forcing ivshmem to be mapped above 4G?

> 
> Suggested-by: Igor Mammedov <[e-mail]>
> Signed-off-by: Marcel Apfelbaum <[e-mail]>
> Tested-by: Laszlo Ersek <[e-mail]>
> ---
>  tests/bios-tables-test.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index de4019e..b23c6b0 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -864,6 +864,41 @@ static void test_acpi_piix4_tcg_ipmi(void)
>      free_test_data(&data);
>  }
>  
> +static void test_acpi_piix4_tcg_pxb(void)
> +{
> +    test_data data;
> +
> +    memset(&data, 0, sizeof(data));
> +    data.machine = MACHINE_PC;
> +    data.variant = ".pxb";
> +    data.required_struct_types = base_required_struct_types;
> +    data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types);
> +    test_acpi_one("-machine accel=tcg"
> +                  " -device pxb,id=pxb,bus_nr=0x80,bus=pci.0"
> +                  " -object memory-backend-file,size=1G,mem-path=/tmp/shmem,share,id=mb"
> +                  " -device ivshmem-plain,memdev=mb,bus=pxb",
> +                  &data);
> +    free_test_data(&data);
> +}
> +
> +static void test_acpi_q35_tcg_pxb_pcie(void)
> +{
> +    test_data data;
> +
> +    memset(&data, 0, sizeof(data));
> +    data.machine = MACHINE_Q35;
> +    data.variant = ".pxb_pcie";
> +    data.required_struct_types = base_required_struct_types;
> +    data.required_struct_types_len = ARRAY_SIZE(ipmi_required_struct_types);
> +    test_acpi_one("-machine q35,accel=tcg"
> +                  " -device pxb-pcie,id=pxb,bus_nr=0x80,bus=pcie.0"
> +                  " -device ioh3420,id=rp,bus=pxb,slot=1"
> +                  " -object memory-backend-file,size=1G,mem-path=/tmp/shmem,share,id=mb"
> +                  " -device ivshmem-plain,memdev=mb,bus=rp",
> +                  &data);
> +    free_test_data(&data);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>      const char *arch = qtest_get_arch();
> @@ -884,6 +919,8 @@ int main(int argc, char *argv[])
>          qtest_add_func("acpi/q35/tcg/ipmi", test_acpi_q35_tcg_ipmi);
>          qtest_add_func("acpi/piix4/tcg/cpuhp", test_acpi_piix4_tcg_cphp);
>          qtest_add_func("acpi/q35/tcg/cpuhp", test_acpi_q35_tcg_cphp);
> +        qtest_add_func("acpi/piix4/tcg/pxb", test_acpi_piix4_tcg_pxb);
> +        qtest_add_func("acpi/q35/tcg/pxb-pcie", test_acpi_q35_tcg_pxb_pcie);
>      }
>      ret = g_test_run();
>      boot_sector_cleanup(disk);

Marcel Apfelbaum July 18, 2016, 2:17 p.m. UTC | #2

On 07/18/2016 04:34 PM, Igor Mammedov wrote:
> On Sun, 17 Jul 2016 19:53:09 +0300
> Marcel Apfelbaum <[e-mail]> wrote:
>
>> Add an ivshmem device with 1G shared memory to
>> pxb in order to check the ACPI code of 64bit MMIO allocation.
> what is forcing ivshmem to be mapped above 4G?

For PC machine: nothing -> ivshmem gets mapped in 32-bit area
For Q35 machine: there is not enough space for mapping 1G
in 32-bit area, and maybe there are also alignment constrains.
You can use this test in verbose mode and see exactly how
the ranges are distributed. If you want me to run it and add the output,
please let me know.

Thanks,
Marcel

>
>>
>> Suggested-by: Igor Mammedov <[e-mail]>
>> Signed-off-by: Marcel Apfelbaum <[e-mail]>
>> Tested-by: Laszlo Ersek <[e-mail]>
>> ---
>>   tests/bios-tables-test.c | 37 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
>> index de4019e..b23c6b0 100644
>> --- a/tests/bios-tables-test.c
>> +++ b/tests/bios-tables-test.c
>> @@ -864,6 +864,41 @@ static void test_acpi_piix4_tcg_ipmi(void)
>>       free_test_data(&data);
>>   }
>>
>> +static void test_acpi_piix4_tcg_pxb(void)
>> +{
>> +    test_data data;
>> +
>> +    memset(&data, 0, sizeof(data));
>> +    data.machine = MACHINE_PC;
>> +    data.variant = ".pxb";
>> +    data.required_struct_types = base_required_struct_types;
>> +    data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types);
>> +    test_acpi_one("-machine accel=tcg"
>> +                  " -device pxb,id=pxb,bus_nr=0x80,bus=pci.0"
>> +                  " -object memory-backend-file,size=1G,mem-path=/tmp/shmem,share,id=mb"
>> +                  " -device ivshmem-plain,memdev=mb,bus=pxb",
>> +                  &data);
>> +    free_test_data(&data);
>> +}
>> +
>> +static void test_acpi_q35_tcg_pxb_pcie(void)
>> +{
>> +    test_data data;
>> +
>> +    memset(&data, 0, sizeof(data));
>> +    data.machine = MACHINE_Q35;
>> +    data.variant = ".pxb_pcie";
>> +    data.required_struct_types = base_required_struct_types;
>> +    data.required_struct_types_len = ARRAY_SIZE(ipmi_required_struct_types);
>> +    test_acpi_one("-machine q35,accel=tcg"
>> +                  " -device pxb-pcie,id=pxb,bus_nr=0x80,bus=pcie.0"
>> +                  " -device ioh3420,id=rp,bus=pxb,slot=1"
>> +                  " -object memory-backend-file,size=1G,mem-path=/tmp/shmem,share,id=mb"
>> +                  " -device ivshmem-plain,memdev=mb,bus=rp",
>> +                  &data);
>> +    free_test_data(&data);
>> +}
>> +
>>   int main(int argc, char *argv[])
>>   {
>>       const char *arch = qtest_get_arch();
>> @@ -884,6 +919,8 @@ int main(int argc, char *argv[])
>>           qtest_add_func("acpi/q35/tcg/ipmi", test_acpi_q35_tcg_ipmi);
>>           qtest_add_func("acpi/piix4/tcg/cpuhp", test_acpi_piix4_tcg_cphp);
>>           qtest_add_func("acpi/q35/tcg/cpuhp", test_acpi_q35_tcg_cphp);
>> +        qtest_add_func("acpi/piix4/tcg/pxb", test_acpi_piix4_tcg_pxb);
>> +        qtest_add_func("acpi/q35/tcg/pxb-pcie", test_acpi_q35_tcg_pxb_pcie);
>>       }
>>       ret = g_test_run();
>>       boot_sector_cleanup(disk);
>

Igor Mammedov July 18, 2016, 2:54 p.m. UTC | #3

On Mon, 18 Jul 2016 17:17:48 +0300
Marcel Apfelbaum <[e-mail]> wrote:

> On 07/18/2016 04:34 PM, Igor Mammedov wrote:
> > On Sun, 17 Jul 2016 19:53:09 +0300
> > Marcel Apfelbaum <[e-mail]> wrote:
> >  
> >> Add an ivshmem device with 1G shared memory to
> >> pxb in order to check the ACPI code of 64bit MMIO allocation.  
> > what is forcing ivshmem to be mapped above 4G?  
> 
> For PC machine: nothing -> ivshmem gets mapped in 32-bit area
> For Q35 machine: there is not enough space for mapping 1G
> in 32-bit area, and maybe there are also alignment constrains.
> You can use this test in verbose mode and see exactly how
> the ranges are distributed. If you want me to run it and add the output,
> please let me know.
No need to post diff, I can do V=1 manually,
the question though is: doesn't PC need ivshmem to go above 4G as well?

> 
> Thanks,
> Marcel
> 
> >  
> >>
> >> Suggested-by: Igor Mammedov <[e-mail]>
> >> Signed-off-by: Marcel Apfelbaum <[e-mail]>
> >> Tested-by: Laszlo Ersek <[e-mail]>
> >> ---
> >>   tests/bios-tables-test.c | 37 +++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 37 insertions(+)
> >>
> >> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> >> index de4019e..b23c6b0 100644
> >> --- a/tests/bios-tables-test.c
> >> +++ b/tests/bios-tables-test.c
> >> @@ -864,6 +864,41 @@ static void test_acpi_piix4_tcg_ipmi(void)
> >>       free_test_data(&data);
> >>   }
> >>
> >> +static void test_acpi_piix4_tcg_pxb(void)
> >> +{
> >> +    test_data data;
> >> +
> >> +    memset(&data, 0, sizeof(data));
> >> +    data.machine = MACHINE_PC;
> >> +    data.variant = ".pxb";
> >> +    data.required_struct_types = base_required_struct_types;
> >> +    data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types);
> >> +    test_acpi_one("-machine accel=tcg"
> >> +                  " -device pxb,id=pxb,bus_nr=0x80,bus=pci.0"
> >> +                  " -object memory-backend-file,size=1G,mem-path=/tmp/shmem,share,id=mb"
> >> +                  " -device ivshmem-plain,memdev=mb,bus=pxb",
> >> +                  &data);
> >> +    free_test_data(&data);
> >> +}
> >> +
> >> +static void test_acpi_q35_tcg_pxb_pcie(void)
> >> +{
> >> +    test_data data;
> >> +
> >> +    memset(&data, 0, sizeof(data));
> >> +    data.machine = MACHINE_Q35;
> >> +    data.variant = ".pxb_pcie";
> >> +    data.required_struct_types = base_required_struct_types;
> >> +    data.required_struct_types_len = ARRAY_SIZE(ipmi_required_struct_types);
> >> +    test_acpi_one("-machine q35,accel=tcg"
> >> +                  " -device pxb-pcie,id=pxb,bus_nr=0x80,bus=pcie.0"
> >> +                  " -device ioh3420,id=rp,bus=pxb,slot=1"
> >> +                  " -object memory-backend-file,size=1G,mem-path=/tmp/shmem,share,id=mb"
> >> +                  " -device ivshmem-plain,memdev=mb,bus=rp",
> >> +                  &data);
> >> +    free_test_data(&data);
> >> +}
> >> +
> >>   int main(int argc, char *argv[])
> >>   {
> >>       const char *arch = qtest_get_arch();
> >> @@ -884,6 +919,8 @@ int main(int argc, char *argv[])
> >>           qtest_add_func("acpi/q35/tcg/ipmi", test_acpi_q35_tcg_ipmi);
> >>           qtest_add_func("acpi/piix4/tcg/cpuhp", test_acpi_piix4_tcg_cphp);
> >>           qtest_add_func("acpi/q35/tcg/cpuhp", test_acpi_q35_tcg_cphp);
> >> +        qtest_add_func("acpi/piix4/tcg/pxb", test_acpi_piix4_tcg_pxb);
> >> +        qtest_add_func("acpi/q35/tcg/pxb-pcie", test_acpi_q35_tcg_pxb_pcie);
> >>       }
> >>       ret = g_test_run();
> >>       boot_sector_cleanup(disk);  
> >  
> 
>

Laszlo Ersek July 18, 2016, 5:52 p.m. UTC | #4

On 07/18/16 15:34, Igor Mammedov wrote:
> On Sun, 17 Jul 2016 19:53:09 +0300
> Marcel Apfelbaum <[e-mail]> wrote:
> 
>> Add an ivshmem device with 1G shared memory to
>> pxb in order to check the ACPI code of 64bit MMIO allocation.
> what is forcing ivshmem to be mapped above 4G?

Speaking for OVMF: unlike SeaBIOS, the edk2 PCI bus driver prefers to
map 64-bit BARs outside of 32-bit address space, regardless of how much
room is left in the 32-bit MMIO aperture.

Thanks
Laszlo

Marcel Apfelbaum July 18, 2016, 7:27 p.m. UTC | #5

On 07/18/2016 05:54 PM, Igor Mammedov wrote:
> On Mon, 18 Jul 2016 17:17:48 +0300
> Marcel Apfelbaum <[e-mail]> wrote:
>
>> On 07/18/2016 04:34 PM, Igor Mammedov wrote:
>>> On Sun, 17 Jul 2016 19:53:09 +0300
>>> Marcel Apfelbaum <[e-mail]> wrote:
>>>
>>>> Add an ivshmem device with 1G shared memory to
>>>> pxb in order to check the ACPI code of 64bit MMIO allocation.
>>> what is forcing ivshmem to be mapped above 4G?
>>
>> For PC machine: nothing -> ivshmem gets mapped in 32-bit area
>> For Q35 machine: there is not enough space for mapping 1G
>> in 32-bit area, and maybe there are also alignment constrains.
>> You can use this test in verbose mode and see exactly how
>> the ranges are distributed. If you want me to run it and add the output,
>> please let me know.
> No need to post diff, I can do V=1 manually,
> the question though is: doesn't PC need ivshmem to go above 4G as well?

We have ivshmem in the PC, but if doesn't go over 4G.
I left it that way in order to show what happens in both cases. (32-bit and 64-bit)
Since is the same code for PC/Q35 it should be enough.

Thanks,
Marcel

>
>>
>> Thanks,
>> Marcel
>>
>>>
>>>>
>>>> Suggested-by: Igor Mammedov <[e-mail]>
>>>> Signed-off-by: Marcel Apfelbaum <[e-mail]>
>>>> Tested-by: Laszlo Ersek <[e-mail]>
>>>> ---
>>>>    tests/bios-tables-test.c | 37 +++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 37 insertions(+)
>>>>
>>>> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
>>>> index de4019e..b23c6b0 100644
>>>> --- a/tests/bios-tables-test.c
>>>> +++ b/tests/bios-tables-test.c
>>>> @@ -864,6 +864,41 @@ static void test_acpi_piix4_tcg_ipmi(void)
>>>>        free_test_data(&data);
>>>>    }
>>>>
>>>> +static void test_acpi_piix4_tcg_pxb(void)
>>>> +{
>>>> +    test_data data;
>>>> +
>>>> +    memset(&data, 0, sizeof(data));
>>>> +    data.machine = MACHINE_PC;
>>>> +    data.variant = ".pxb";
>>>> +    data.required_struct_types = base_required_struct_types;
>>>> +    data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types);
>>>> +    test_acpi_one("-machine accel=tcg"
>>>> +                  " -device pxb,id=pxb,bus_nr=0x80,bus=pci.0"
>>>> +                  " -object memory-backend-file,size=1G,mem-path=/tmp/shmem,share,id=mb"
>>>> +                  " -device ivshmem-plain,memdev=mb,bus=pxb",
>>>> +                  &data);
>>>> +    free_test_data(&data);
>>>> +}
>>>> +
>>>> +static void test_acpi_q35_tcg_pxb_pcie(void)
>>>> +{
>>>> +    test_data data;
>>>> +
>>>> +    memset(&data, 0, sizeof(data));
>>>> +    data.machine = MACHINE_Q35;
>>>> +    data.variant = ".pxb_pcie";
>>>> +    data.required_struct_types = base_required_struct_types;
>>>> +    data.required_struct_types_len = ARRAY_SIZE(ipmi_required_struct_types);
>>>> +    test_acpi_one("-machine q35,accel=tcg"
>>>> +                  " -device pxb-pcie,id=pxb,bus_nr=0x80,bus=pcie.0"
>>>> +                  " -device ioh3420,id=rp,bus=pxb,slot=1"
>>>> +                  " -object memory-backend-file,size=1G,mem-path=/tmp/shmem,share,id=mb"
>>>> +                  " -device ivshmem-plain,memdev=mb,bus=rp",
>>>> +                  &data);
>>>> +    free_test_data(&data);
>>>> +}
>>>> +
>>>>    int main(int argc, char *argv[])
>>>>    {
>>>>        const char *arch = qtest_get_arch();
>>>> @@ -884,6 +919,8 @@ int main(int argc, char *argv[])
>>>>            qtest_add_func("acpi/q35/tcg/ipmi", test_acpi_q35_tcg_ipmi);
>>>>            qtest_add_func("acpi/piix4/tcg/cpuhp", test_acpi_piix4_tcg_cphp);
>>>>            qtest_add_func("acpi/q35/tcg/cpuhp", test_acpi_q35_tcg_cphp);
>>>> +        qtest_add_func("acpi/piix4/tcg/pxb", test_acpi_piix4_tcg_pxb);
>>>> +        qtest_add_func("acpi/q35/tcg/pxb-pcie", test_acpi_q35_tcg_pxb_pcie);
>>>>        }
>>>>        ret = g_test_run();
>>>>        boot_sector_cleanup(disk);
>>>
>>
>>
>

Marcel Apfelbaum July 18, 2016, 7:32 p.m. UTC | #6

On 07/18/2016 08:52 PM, Laszlo Ersek wrote:
> On 07/18/16 15:34, Igor Mammedov wrote:
>> On Sun, 17 Jul 2016 19:53:09 +0300
>> Marcel Apfelbaum <[e-mail]> wrote:
>>
>>> Add an ivshmem device with 1G shared memory to
>>> pxb in order to check the ACPI code of 64bit MMIO allocation.
>> what is forcing ivshmem to be mapped above 4G?
>
> Speaking for OVMF: unlike SeaBIOS, the edk2 PCI bus driver prefers to
> map 64-bit BARs outside of 32-bit address space, regardless of how much
> room is left in the 32-bit MMIO aperture.

Thanks Laszlo, we need to support make check with ovmf (not immediate, of course).
There is no integration yet with QEMU, right? Having a set of binaries like with SeaBIOS
makes sense? Are there any licensing issues?

Thanks,
Marcel

>
> Thanks
> Laszlo
>

Laszlo Ersek July 18, 2016, 8:08 p.m. UTC | #7

Adding Gerd

On 07/18/16 21:32, Marcel Apfelbaum wrote:
> On 07/18/2016 08:52 PM, Laszlo Ersek wrote:
>> On 07/18/16 15:34, Igor Mammedov wrote:
>>> On Sun, 17 Jul 2016 19:53:09 +0300
>>> Marcel Apfelbaum <[e-mail]> wrote:
>>>
>>>> Add an ivshmem device with 1G shared memory to
>>>> pxb in order to check the ACPI code of 64bit MMIO allocation.
>>> what is forcing ivshmem to be mapped above 4G?
>>
>> Speaking for OVMF: unlike SeaBIOS, the edk2 PCI bus driver prefers to
>> map 64-bit BARs outside of 32-bit address space, regardless of how much
>> room is left in the 32-bit MMIO aperture.
> 
> Thanks Laszlo, we need to support make check with ovmf (not immediate,
> of course).
> There is no integration yet with QEMU, right? Having a set of binaries
> like with SeaBIOS
> makes sense?

Yes, it makes sense.

> Are there any licensing issues?

If QEMU can bundle 2-clause BSDL binaries, then there shouldn't be.

For the legally inclined, it might be safest to review the
*Pkg/License.txt files, for all the *Pkg top-level directories from
which OVMF pulls in at least one module.

Thanks
Laszlo

Igor Mammedov July 19, 2016, 7:34 a.m. UTC | #8

On Sun, 17 Jul 2016 19:53:09 +0300
Marcel Apfelbaum <[e-mail]> wrote:

> Add an ivshmem device with 1G shared memory to
> pxb in order to check the ACPI code of 64bit MMIO allocation.
> 
> Suggested-by: Igor Mammedov <[e-mail]>
> Signed-off-by: Marcel Apfelbaum <[e-mail]>
> Tested-by: Laszlo Ersek <[e-mail]>
Reviewed-by: Igor Mammedov <[e-mail]>

> ---
>  tests/bios-tables-test.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index de4019e..b23c6b0 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -864,6 +864,41 @@ static void test_acpi_piix4_tcg_ipmi(void)
>      free_test_data(&data);
>  }
>  
> +static void test_acpi_piix4_tcg_pxb(void)
> +{
> +    test_data data;
> +
> +    memset(&data, 0, sizeof(data));
> +    data.machine = MACHINE_PC;
> +    data.variant = ".pxb";
> +    data.required_struct_types = base_required_struct_types;
> +    data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types);
> +    test_acpi_one("-machine accel=tcg"
> +                  " -device pxb,id=pxb,bus_nr=0x80,bus=pci.0"
> +                  " -object memory-backend-file,size=1G,mem-path=/tmp/shmem,share,id=mb"
> +                  " -device ivshmem-plain,memdev=mb,bus=pxb",
> +                  &data);
> +    free_test_data(&data);
> +}
> +
> +static void test_acpi_q35_tcg_pxb_pcie(void)
> +{
> +    test_data data;
> +
> +    memset(&data, 0, sizeof(data));
> +    data.machine = MACHINE_Q35;
> +    data.variant = ".pxb_pcie";
> +    data.required_struct_types = base_required_struct_types;
> +    data.required_struct_types_len = ARRAY_SIZE(ipmi_required_struct_types);
> +    test_acpi_one("-machine q35,accel=tcg"
> +                  " -device pxb-pcie,id=pxb,bus_nr=0x80,bus=pcie.0"
> +                  " -device ioh3420,id=rp,bus=pxb,slot=1"
> +                  " -object memory-backend-file,size=1G,mem-path=/tmp/shmem,share,id=mb"
> +                  " -device ivshmem-plain,memdev=mb,bus=rp",
> +                  &data);
> +    free_test_data(&data);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>      const char *arch = qtest_get_arch();
> @@ -884,6 +919,8 @@ int main(int argc, char *argv[])
>          qtest_add_func("acpi/q35/tcg/ipmi", test_acpi_q35_tcg_ipmi);
>          qtest_add_func("acpi/piix4/tcg/cpuhp", test_acpi_piix4_tcg_cphp);
>          qtest_add_func("acpi/q35/tcg/cpuhp", test_acpi_q35_tcg_cphp);
> +        qtest_add_func("acpi/piix4/tcg/pxb", test_acpi_piix4_tcg_pxb);
> +        qtest_add_func("acpi/q35/tcg/pxb-pcie", test_acpi_q35_tcg_pxb_pcie);
>      }
>      ret = g_test_run();
>      boot_sector_cleanup(disk);

Marcel Apfelbaum July 19, 2016, 8:10 a.m. UTC | #9

On 07/19/2016 10:34 AM, Igor Mammedov wrote:
> On Sun, 17 Jul 2016 19:53:09 +0300
> Marcel Apfelbaum <[e-mail]> wrote:
>
>> Add an ivshmem device with 1G shared memory to
>> pxb in order to check the ACPI code of 64bit MMIO allocation.
>>
>> Suggested-by: Igor Mammedov <[e-mail]>
>> Signed-off-by: Marcel Apfelbaum <[e-mail]>
>> Tested-by: Laszlo Ersek <[e-mail]>
> Reviewed-by: Igor Mammedov <[e-mail]>


Thanks Igor!
Marcel

>
>> ---
>>   tests/bios-tables-test.c | 37 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
>> index de4019e..b23c6b0 100644
>> --- a/tests/bios-tables-test.c
>> +++ b/tests/bios-tables-test.c
>> @@ -864,6 +864,41 @@ static void test_acpi_piix4_tcg_ipmi(void)
>>       free_test_data(&data);
>>   }
>>
>> +static void test_acpi_piix4_tcg_pxb(void)
>> +{
>> +    test_data data;
>> +
>> +    memset(&data, 0, sizeof(data));
>> +    data.machine = MACHINE_PC;
>> +    data.variant = ".pxb";
>> +    data.required_struct_types = base_required_struct_types;
>> +    data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types);
>> +    test_acpi_one("-machine accel=tcg"
>> +                  " -device pxb,id=pxb,bus_nr=0x80,bus=pci.0"
>> +                  " -object memory-backend-file,size=1G,mem-path=/tmp/shmem,share,id=mb"
>> +                  " -device ivshmem-plain,memdev=mb,bus=pxb",
>> +                  &data);
>> +    free_test_data(&data);
>> +}
>> +
>> +static void test_acpi_q35_tcg_pxb_pcie(void)
>> +{
>> +    test_data data;
>> +
>> +    memset(&data, 0, sizeof(data));
>> +    data.machine = MACHINE_Q35;
>> +    data.variant = ".pxb_pcie";
>> +    data.required_struct_types = base_required_struct_types;
>> +    data.required_struct_types_len = ARRAY_SIZE(ipmi_required_struct_types);
>> +    test_acpi_one("-machine q35,accel=tcg"
>> +                  " -device pxb-pcie,id=pxb,bus_nr=0x80,bus=pcie.0"
>> +                  " -device ioh3420,id=rp,bus=pxb,slot=1"
>> +                  " -object memory-backend-file,size=1G,mem-path=/tmp/shmem,share,id=mb"
>> +                  " -device ivshmem-plain,memdev=mb,bus=rp",
>> +                  &data);
>> +    free_test_data(&data);
>> +}
>> +
>>   int main(int argc, char *argv[])
>>   {
>>       const char *arch = qtest_get_arch();
>> @@ -884,6 +919,8 @@ int main(int argc, char *argv[])
>>           qtest_add_func("acpi/q35/tcg/ipmi", test_acpi_q35_tcg_ipmi);
>>           qtest_add_func("acpi/piix4/tcg/cpuhp", test_acpi_piix4_tcg_cphp);
>>           qtest_add_func("acpi/q35/tcg/cpuhp", test_acpi_q35_tcg_cphp);
>> +        qtest_add_func("acpi/piix4/tcg/pxb", test_acpi_piix4_tcg_pxb);
>> +        qtest_add_func("acpi/q35/tcg/pxb-pcie", test_acpi_q35_tcg_pxb_pcie);
>>       }
>>       ret = g_test_run();
>>       boot_sector_cleanup(disk);
>

Gerd Hoffmann July 19, 2016, 9:06 a.m. UTC | #10

Hi,

> > There is no integration yet with QEMU, right? Having a set of binaries
> > like with SeaBIOS
> > makes sense?
> 
> Yes, it makes sense.

Indeed.  edk2 isn't exactly small though.  Adding the submodule would
make the release tarballs noticable larger, and the prebuild binaries
are not exactly small too ...

I remember we discussed moving all the pre-built binaries to a separate
repo a while back.  Maybe we should think about that idea again?

> > Are there any licensing issues?
> 
> If QEMU can bundle 2-clause BSDL binaries, then there shouldn't be.

I think with the FatPkg issue finally solved now there shouldn't be
fundamental roadblocks any more.

Next question would be which architectures we want build edk2 for?

 (1) x64 ovmf is clear.

 (2) ia32 ovmf too?  Will anybody use it?

 (3) ArmVirtPkg for aarch64 should be built IMO.

 (4) What about ArmVirtPkg for 32bit arm?

 (5) There is ArmPlatformPkg/ArmVExpressPkg.
     Anyone tried that with qemu-system-{arm,aarch64} -M vexpress-* ?

While being at it:  Is there any way to setup a 32bit arm guest with a
bootloader, so you don't have to somehow copy the guest kernel from the
image to boot?

At least the upstream kernel doesn't support acpi @ 32bit arm, so the
guest kernel can't find and use the hardware info provided by
ArmVirtPkg.  Instead it depends on the bootloader passing a fdt.  But
the bootloaders (grub+uboot) expect a file with the fdt somewhere.
There isn't a file in the first place for -M virt.  There is one for the
vexpress boards, which actually works (for v9).  But it lists only the
hardware physical vexpress boards have, any virtio-mmio devices you add
are not listed there.  So sdcard storage is the only option.  Hmm.

Comments?

cheers,
  Gerd

Peter Maydell July 19, 2016, 9:30 a.m. UTC | #11

On 19 July 2016 at 10:06, Gerd Hoffmann <[e-mail]> wrote:
> Next question would be which architectures we want build edk2 for?
>
>  (1) x64 ovmf is clear.
>
>  (2) ia32 ovmf too?  Will anybody use it?
>
>  (3) ArmVirtPkg for aarch64 should be built IMO.
>
>  (4) What about ArmVirtPkg for 32bit arm?

I think this would be a good idea.

>  (5) There is ArmPlatformPkg/ArmVExpressPkg.
>      Anyone tried that with qemu-system-{arm,aarch64} -M vexpress-* ?
>
> While being at it:  Is there any way to setup a 32bit arm guest with a
> bootloader, so you don't have to somehow copy the guest kernel from the
> image to boot?
>
> At least the upstream kernel doesn't support acpi @ 32bit arm, so the
> guest kernel can't find and use the hardware info provided by
> ArmVirtPkg.  Instead it depends on the bootloader passing a fdt.  But
> the bootloaders (grub+uboot) expect a file with the fdt somewhere.
> There isn't a file in the first place for -M virt.  There is one for the
> vexpress boards, which actually works (for v9).  But it lists only the
> hardware physical vexpress boards have, any virtio-mmio devices you add
> are not listed there.  So sdcard storage is the only option.  Hmm.

We should be aiming to get 'virt' to work for the 32-bit case.
vexpress-a9/a15 is trying to model real hardware and has a
lot of irritating constraints as a result (like no PCI, only
SD card storage).

This probably means sorting out passing through the DTB
from QEMU into UEFI and then into the boot loader.

thanks
-- PMM

Laszlo Ersek July 19, 2016, 9:59 a.m. UTC | #12

On 07/19/16 11:06, Gerd Hoffmann wrote:
>   Hi,
> 
>>> There is no integration yet with QEMU, right? Having a set of binaries
>>> like with SeaBIOS
>>> makes sense?
>>
>> Yes, it makes sense.
> 
> Indeed.  edk2 isn't exactly small though.  Adding the submodule would
> make the release tarballs noticable larger, and the prebuild binaries
> are not exactly small too ...
> 
> I remember we discussed moving all the pre-built binaries to a separate
> repo a while back.  Maybe we should think about that idea again?
> 
>>> Are there any licensing issues?
>>
>> If QEMU can bundle 2-clause BSDL binaries, then there shouldn't be.
> 
> I think with the FatPkg issue finally solved now there shouldn't be
> fundamental roadblocks any more.
> 
> Next question would be which architectures we want build edk2 for?
> 
>  (1) x64 ovmf is clear.
> 
>  (2) ia32 ovmf too?  Will anybody use it?

There are three bitness-combinations:
- Ia32: both PEI and DXE phases are 32-bit
- Ia32X64: PEI is 32-bit, DXE is 64-bit
- X64: both PEI and DXE are 64-bit

The bitness of the DXE phase must match the bitness of the operatig
system. So Ia32 can only boot 32-bit OSes (*), and the other two can
only boot 64-bit OSes.

(*) This is a bit relaxed for Linux guests, because Linux supports being
booted on a 32-bit DXE phase firmware, and then switches to 64-bit
itself. IIRC the idea here was to enable 64-bit Linux to run on tablets
and similar hardware that has a 64-bit CPU, but only comes with a fully
32-bit firmware (see
<https://blogs.intel.com/evangelists/2015/07/22/why-cheap-systems-run-32-bit-uefi-on-x64-systems/>).
But, without such OS-level tricks, in general Ia32 DXE implies 32-bit OS.

Then the next question is, what's the status of 32-bit UEFI OSes? Simple:

- 32-bit UEFI Windows exists, but it doesn't boot on OVMF. I don't know
why. I've spend a lot of time debugging it, on and off, with the
(sporadic) help of a Microsoft developer. We narrowed it down somewhat,
but it remains unsolved.

- 32-bit Linux may exist, but the distros I tried are either not
official (see Fedlet
<https://www.happyassassin.net/fedlet-a-fedora-remix-for-bay-trail-tablets/>)
or I didn't get any usable results with them (IIRC I tried 32-bit UEFI
Debain once before, and it didn't work at all; but I'm fuzzy on that,
sorry).

Matthew Garrett also advises against (fully) 32-bit firmware:
<https://mjg59.dreamwidth.org/26734.html>

The distinction between Ia32X64 and X64 is less clear cut. The
difference between them is inivisible to the user (the bitness of the
PEI phase is not exposed to the OS), but in practice there is one
visible difference: the combination

  SMM driver stack + X64 PEI + S3 resume enabled on QEMU

is not supported by edk2 at the moment. So if the binary should support
SMM + S3, and allow the user not to bother about disabling S3 on the
QEMU command line, then Ia32X64 is the better choice.

Furthermore, the SMM driver stack limits the firmware to pc-q35-2.5+
machine types (no i440fx at all, and no q35 earlier than 2.5). This
might not be useful for everyone, I'm not sure.

CSM should never be enabled (SeaBIOS is bundled with QEMU already).

Enabling Secure Boot in the OVMF binary is orthogonal to all of the
above, but it has a licensing impact. It embeds (a subset of) OpenSSL in
the binary, and changes the terms from "2-clause BSDL" to "2-clause BSDL
and OpenSSL license" ("and" in the restrictive, not permissive, sense).
I'm unsure if QEMU is willing and able to distribute such binaries.

For the widest and simplest usability, X64 (without the SMM driver stack
and without Secure Boot) is likely the best.

>  (3) ArmVirtPkg for aarch64 should be built IMO.

Yup.

>  (4) What about ArmVirtPkg for 32bit arm?

It is a supported configuration in edk2, but I don't know how useful it
would be. We should ask Ard I guess.

>  (5) There is ArmPlatformPkg/ArmVExpressPkg.
>      Anyone tried that with qemu-system-{arm,aarch64} -M vexpress-* ?

Historically I may have used it I think, but that was before the arrival
of ArmVirtPkg (nee ArmPlatformPkg/ArmVirtualizationPkg) for the "virt"
machine type.

> While being at it:  Is there any way to setup a 32bit arm guest with a
> bootloader, so you don't have to somehow copy the guest kernel from the
> image to boot?

I'm unaware of any limitation in ArmVirtPkg or edk2 that would prevent
this; I just think OS bother to support 32-bit UEFI even less on ARM
than they do on x86.

> At least the upstream kernel doesn't support acpi @ 32bit arm, so the
> guest kernel can't find and use the hardware info provided by
> ArmVirtPkg.  Instead it depends on the bootloader passing a fdt.  But
> the bootloaders (grub+uboot) expect a file with the fdt somewhere.

ArmVirtPkg gets a generated DTB from QEMU at the base of system memory,
stashes it safely, uses it, and (unless disabled explicitly by a node in
the DTB) exposes it to the UEFI OS through a dedicated UEFI system
configuration table. Linux is capable of using this sysconfig table
already, so those boot loaders should be taught the same, assuming
32-bit UEFI ARM is so important.

QEMU                   ArmVirtQemu
 | ----- DT via RAM ------> |
 | --- ACPI via fw_cfg ---> |
 | -- SMBIOS via fw_cfg --> |                               guest kernel
                            | ---- DT via UEFI config table ----> |
                            | --- ACPI via UEFI config table ---> |
                            | -- SMBIOS via UEFI config table --> |

(According to my last information, the upstream kernel still prefers DT
over ACPI. I'm aware of two methods to talk it out of that:
- "acpi=force" on the guest kernel cmdline (dynamically),
- the "-D PURE_ACPI_BOOT_ENABLE" ArmVirtQemu build option
<https://github.com/tianocore/edk2/commit/b359fb9115b2>, which
statically removes the right hand side "DT" edge from the above diagram.)

Anyway, I digress. Point is, GRUB is already UEFI capable (I don't know
uboot), so GRUB should be able to look up the DTB sysconfig table, and
use that. The sysconfig table in question is identified by the GUID

  B1B621D5-F19C-41A5-830B-D9152C69AAE0

(or written in C:
<https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/Include/Guid/Fdt.h>.)

> There isn't a file in the first place for -M virt.

It's not necessary, see above. If you would like to investigate the DTB
that QEMU generates, you can use:

  $ qemu-system-(arm|aarch64) \
      -nographic -machine virt,dumpdtb=virt-dump.dtb
  $ dtc -I dtb -O dts <virt-dump.dtb | less

but that's not the right way to carry the DTB from QEMU to guest code
(at least on the "virt" machine type).

> There is one for the
> vexpress boards, which actually works (for v9).  But it lists only the
> hardware physical vexpress boards have, any virtio-mmio devices you add
> are not listed there.

Right, the generated DTB lists all the virtio-mmio transports.

>  So sdcard storage is the only option.  Hmm.

Not necessarily; it can be considered a UEFI GRUB enhancement IMO.

Thanks,
Laszlo

Gerd Hoffmann July 19, 2016, 10:05 a.m. UTC | #13

Hi,

> >  (4) What about ArmVirtPkg for 32bit arm?
> 
> I think this would be a good idea.

> We should be aiming to get 'virt' to work for the 32-bit case.
> vexpress-a9/a15 is trying to model real hardware and has a
> lot of irritating constraints as a result (like no PCI, only
> SD card storage).
> 
> This probably means sorting out passing through the DTB
> from QEMU into UEFI and then into the boot loader.

Looking at aarch64 it looks like the guest kernel doesn't use acpi but
got a fdt somehow.  Dunno how that happened.  I guess ArmVirtPkg exports
the FDT using EFI interfaces, then either the kernel gets it directly or
grub is able to get and pass on the FDT.  In any case it seems think the
same should work for 32bit without too much trouble.

cheers,
  Gerd

Laszlo Ersek July 19, 2016, 10:13 a.m. UTC | #14

thinko:

On 07/19/16 11:59, Laszlo Ersek wrote:

> ArmVirtPkg gets a generated DTB from QEMU at the base of system memory,
> stashes it safely, uses it, and (unless disabled explicitly by a node in
> the DTB) exposes it to the UEFI OS through a dedicated UEFI system
> configuration table.

replace

  unless disabled explicitly by a node in the DTB

with

  unless disabled explicitly with "-D PURE_ACPI_BOOT_ENABLE", mentioned
  elsewhere in the message

Thanks
Laszlo

Laszlo Ersek July 19, 2016, 10:40 a.m. UTC | #15

On 07/19/16 12:05, Gerd Hoffmann wrote:
>   Hi,
> 
>>>  (4) What about ArmVirtPkg for 32bit arm?
>>
>> I think this would be a good idea.
> 
>> We should be aiming to get 'virt' to work for the 32-bit case.
>> vexpress-a9/a15 is trying to model real hardware and has a
>> lot of irritating constraints as a result (like no PCI, only
>> SD card storage).
>>
>> This probably means sorting out passing through the DTB
>> from QEMU into UEFI and then into the boot loader.

Yes, the piece that is missing is that the boot loader should be capable
of looking up the UEFI sysconfig table with the well-known GUID that
exposes the DTB.

(NB: such a sysconfig table is not *required* by any means, especially
not on aarch64. It is valid for the firmware to not expose any DTB to
stuff that comes after it, in an "ACPI only" configuration.)

... Actually, I'm getting confused about this. I just suggested that the
boot loader look up the right UEFI sysconfig table. But, if the boot
loader is already UEFI capable, why does it need the DTB at all? Gerd
named the virtio-mmio nodes in the DTB, but now I don't see how they are
relevant -- the boot loader should just use the UEFI services to load
and start the OS. Why does it need to know about virtio-mmio at all?

> Looking at aarch64 it looks like the guest kernel doesn't use acpi but
> got a fdt somehow.  Dunno how that happened.

The aarch64 upstream kernel (to my knowledge) defaults to DTB, unless it
gets "acpi=force" on the command line, or ArmVirtQemu is built with -D
PURE_ACPI_BOOT_ENABLE" (in which case the kernel will have no DTB to
look at, only ACPI).

>  I guess ArmVirtPkg exports
> the FDT using EFI interfaces,

Yes (unless built with "-D PURE_ACPI_BOOT_ENABLE").

> then either the kernel gets it directly

The DTB is installed as a UEFI sysconfig table with a well-known GUID.
All guest code that can look up UEFI sysconfig tables can locate and
read it.

> or
> grub is able to get and pass on the FDT.  In any case it seems think the
> same should work for 32bit without too much trouble.

It should be possible, but as I said, now I'm doubting that grub should
be involved in this at all.

First, the DTB reaches the kernel, from the firmware, without any
assitance from grub, through the sysconfig table.

Second, grub should not need the DTB for its own purposes *at all* -- a
boot loader on a UEFI system should use UEFI services only, for booting
the OS. The DTB might be necessary for low-level hardware drivers, I
guess, but in a UEFI guest, grub should not use low-level hw drivers.

... I mean, this is how grub already works in x86_64 and aarch64 UEFI
guests. I think we're sitting on the horse backwards. DTB has probably
been a "last resort" for 32-bit ARM GRUB, because there used to be no
UEFI, hence no UEFI services, hence only the DTB allowed GRUB to do
something with the hardware

But now that there *is* UEFI for 32-bit ARM (guests, anyway), the
bootloader should not use UEFI only as a means to fetch the DTB, and
then do whatever it's been doing earlier. Instead, UEFI should be used
to boot the OS.

Thanks
Laszlo

Gerd Hoffmann July 19, 2016, 10:48 a.m. UTC | #16

Hi,

> >  (2) ia32 ovmf too?  Will anybody use it?
> 
> Then the next question is, what's the status of 32-bit UEFI OSes? Simple:

> [ summary: bad ]

Yep, that matches the impression I have.
Guess we don't want bother then.

> Enabling Secure Boot in the OVMF binary is orthogonal to all of the
> above, but it has a licensing impact. It embeds (a subset of) OpenSSL in
> the binary, and changes the terms from "2-clause BSDL" to "2-clause BSDL
> and OpenSSL license" ("and" in the restrictive, not permissive, sense).
> I'm unsure if QEMU is willing and able to distribute such binaries.
> 
> For the widest and simplest usability, X64 (without the SMM driver stack
> and without Secure Boot) is likely the best.

Yes (also note the smm-enabled one doesn't run on i440fx).

So the options I see are (a) build without smm or (b) build two
variants.

> Anyway, I digress. Point is, GRUB is already UEFI capable (I don't know
> uboot), so GRUB should be able to look up the DTB sysconfig table, and
> use that. The sysconfig table in question is identified by the GUID
> 
>   B1B621D5-F19C-41A5-830B-D9152C69AAE0

grub already does that on aarch64, but not on arm.
So that should be fixable without too much effort.

u-boot is out for -M virt due to missing virtio drivers.

>   $ qemu-system-(arm|aarch64) \
>       -nographic -machine virt,dumpdtb=virt-dump.dtb
>   $ dtc -I dtb -O dts <virt-dump.dtb | less
> 
> but that's not the right way to carry the DTB from QEMU to guest code

Exactly.  That just creates a reverse problem.  Instead of copying the
kernel from the image you now have to copy the dtb file to the image.

> > There is one for the
> > vexpress boards, which actually works (for v9).  But it lists only the
> > hardware physical vexpress boards have, any virtio-mmio devices you add
> > are not listed there.
> 
> Right, the generated DTB lists all the virtio-mmio transports.

But u-boot looks for dtb files in /boot/dtbs/$kernelver/ instead of
using the one provided by qemu.  So virtio isn't there (virtio-mmio @
vexpress works for a direct kernel boot).

cheers,
  Gerd

Laszlo Ersek July 19, 2016, 11:42 a.m. UTC | #17

On 07/19/16 12:48, Gerd Hoffmann wrote:
>   Hi,
> 
>>>  (2) ia32 ovmf too?  Will anybody use it?
>>
>> Then the next question is, what's the status of 32-bit UEFI OSes? Simple:
> 
>> [ summary: bad ]
> 
> Yep, that matches the impression I have.
> Guess we don't want bother then.
> 
>> Enabling Secure Boot in the OVMF binary is orthogonal to all of the
>> above, but it has a licensing impact. It embeds (a subset of) OpenSSL in
>> the binary, and changes the terms from "2-clause BSDL" to "2-clause BSDL
>> and OpenSSL license" ("and" in the restrictive, not permissive, sense).
>> I'm unsure if QEMU is willing and able to distribute such binaries.
>>
>> For the widest and simplest usability, X64 (without the SMM driver stack
>> and without Secure Boot) is likely the best.
> 
> Yes (also note the smm-enabled one doesn't run on i440fx).
> 
> So the options I see are (a) build without smm or (b) build two
> variants.

I think people who just want to run "UEFI payloads" are served well
enough by SMM-less. There are some developers who would benefit from a
-D SMM_REQUIRE build as well, but that would be because they actually
focus on SMM, I believe, so they can build their own firmware.

If this is about the convenience of QEMU end-users, then I vote (a). I'd
certainly like to avoid fielding bug reports that boil down to "I booted
the -D SMM_REQUIRE build on i440fx".

>> Anyway, I digress. Point is, GRUB is already UEFI capable (I don't know
>> uboot), so GRUB should be able to look up the DTB sysconfig table, and
>> use that. The sysconfig table in question is identified by the GUID
>>
>>   B1B621D5-F19C-41A5-830B-D9152C69AAE0
> 
> grub already does that on aarch64,

Huh, indeed. I see "grub-core/loader/arm64/fdt.c". What does UEFI grub
need the DTB for in the first place?

Hm... looks like it pokes initrd information into it. Interesting.

> but not on arm.
> So that should be fixable without too much effort.

I guess so.

I'll mention though that "just" for passing in the initrd, the DTB
shouldn't be necessary, at least if the kernel is built with the EFI
stub. Then "initrd=filename" can be passed on the kernel command line,
and the EFI stub should load it, using UEFI services, from the same
directory that the vmlinuz binary (= itself) came from.

... I can never keep the 37 ways to boot the linux kernel in my mind for
longer than 2 minutes. :/

Thanks
Laszlo

Marcel Apfelbaum July 19, 2016, 2:25 p.m. UTC | #18

On 07/19/2016 02:42 PM, Laszlo Ersek wrote:
> On 07/19/16 12:48, Gerd Hoffmann wrote:
>>    Hi,
>>
>>>>   (2) ia32 ovmf too?  Will anybody use it?
>>>
>>> Then the next question is, what's the status of 32-bit UEFI OSes? Simple:
>>
>>> [ summary: bad ]
>>
>> Yep, that matches the impression I have.
>> Guess we don't want bother then.
>>
>>> Enabling Secure Boot in the OVMF binary is orthogonal to all of the
>>> above, but it has a licensing impact. It embeds (a subset of) OpenSSL in
>>> the binary, and changes the terms from "2-clause BSDL" to "2-clause BSDL
>>> and OpenSSL license" ("and" in the restrictive, not permissive, sense).
>>> I'm unsure if QEMU is willing and able to distribute such binaries.
>>>
>>> For the widest and simplest usability, X64 (without the SMM driver stack
>>> and without Secure Boot) is likely the best.
>>
>> Yes (also note the smm-enabled one doesn't run on i440fx).
>>
>> So the options I see are (a) build without smm or (b) build two
>> variants.
>
> I think people who just want to run "UEFI payloads" are served well
> enough by SMM-less. There are some developers who would benefit from a
> -D SMM_REQUIRE build as well, but that would be because they actually
> focus on SMM, I believe, so they can build their own firmware.
>
> If this is about the convenience of QEMU end-users, then I vote (a). I'd
> certainly like to avoid fielding bug reports that boil down to "I booted
> the -D SMM_REQUIRE build on i440fx".
>

I agree. For x86_64, OVMF 64-bit binaries without SMM or secure-boot
support are enough for a wide "audience".


Thanks,
Marcel

[...]

Patch

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index de4019e..b23c6b0 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -864,6 +864,41 @@  static void test_acpi_piix4_tcg_ipmi(void)
     free_test_data(&data);
 }
 
+static void test_acpi_piix4_tcg_pxb(void)
+{
+    test_data data;
+
+    memset(&data, 0, sizeof(data));
+    data.machine = MACHINE_PC;
+    data.variant = ".pxb";
+    data.required_struct_types = base_required_struct_types;
+    data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types);
+    test_acpi_one("-machine accel=tcg"
+                  " -device pxb,id=pxb,bus_nr=0x80,bus=pci.0"
+                  " -object memory-backend-file,size=1G,mem-path=/tmp/shmem,share,id=mb"
+                  " -device ivshmem-plain,memdev=mb,bus=pxb",
+                  &data);
+    free_test_data(&data);
+}
+
+static void test_acpi_q35_tcg_pxb_pcie(void)
+{
+    test_data data;
+
+    memset(&data, 0, sizeof(data));
+    data.machine = MACHINE_Q35;
+    data.variant = ".pxb_pcie";
+    data.required_struct_types = base_required_struct_types;
+    data.required_struct_types_len = ARRAY_SIZE(ipmi_required_struct_types);
+    test_acpi_one("-machine q35,accel=tcg"
+                  " -device pxb-pcie,id=pxb,bus_nr=0x80,bus=pcie.0"
+                  " -device ioh3420,id=rp,bus=pxb,slot=1"
+                  " -object memory-backend-file,size=1G,mem-path=/tmp/shmem,share,id=mb"
+                  " -device ivshmem-plain,memdev=mb,bus=rp",
+                  &data);
+    free_test_data(&data);
+}
+
 int main(int argc, char *argv[])
 {
     const char *arch = qtest_get_arch();
@@ -884,6 +919,8 @@  int main(int argc, char *argv[])
         qtest_add_func("acpi/q35/tcg/ipmi", test_acpi_q35_tcg_ipmi);
         qtest_add_func("acpi/piix4/tcg/cpuhp", test_acpi_piix4_tcg_cphp);
         qtest_add_func("acpi/q35/tcg/cpuhp", test_acpi_q35_tcg_cphp);
+        qtest_add_func("acpi/piix4/tcg/pxb", test_acpi_piix4_tcg_pxb);
+        qtest_add_func("acpi/q35/tcg/pxb-pcie", test_acpi_q35_tcg_pxb_pcie);
     }
     ret = g_test_run();
     boot_sector_cleanup(disk);