mbox series

[v3,0/9] tests: Add test cases for TPM 1.2 ACPI tables

Message ID 20210712150949.165725-1-stefanb@linux.vnet.ibm.com (mailing list archive)
Headers show
Series tests: Add test cases for TPM 1.2 ACPI tables | expand

Message

Stefan Berger July 12, 2021, 3:09 p.m. UTC
This series of patches adds test case for TPM 1.2 ACPI tables.

  Stefan

v3:
  - Define enum TPMVersion for when CONFIG_TPM is not defined
    affected patches 2 and 6

v2:
  - Proper handling of renaming of files holding expected ACPI data


Stefan Berger (9):
  tests: Rename TestState to TPMTestState
  tests: Add tpm_version field to TPMTestState and fill it
  tests: acpi: Prepare for renaming of TPM2 related ACPI files
  tests: Add suffix 'tpm2' or 'tpm12' to ACPI table files
  tests: acpi: tpm2: Add the renamed ACPI files and drop old ones
  tests: tpm: Create TPM 1.2 response in TPM emulator
  tests: acpi: prepare for new TPM 1.2 related tables
  tests: acpi: Add test cases for TPM 1.2 with TCPA table
  tests: acpi: tpm1.2: Add expected TPM 1.2 ACPI blobs

 tests/data/acpi/q35/DSDT.tis.tpm12            | Bin 0 -> 8465 bytes
 .../data/acpi/q35/{DSDT.tis => DSDT.tis.tpm2} | Bin
 tests/data/acpi/q35/TCPA.tis.tpm12            | Bin 0 -> 50 bytes
 .../data/acpi/q35/{TPM2.tis => TPM2.tis.tpm2} | Bin
 tests/qtest/bios-tables-test.c                |  20 ++++++++++-----
 tests/qtest/tpm-crb-test.c                    |   5 ++--
 tests/qtest/tpm-emu.c                         |  24 +++++++++++++-----
 tests/qtest/tpm-emu.h                         |  18 ++++++++++---
 tests/qtest/tpm-tis-device-test.c             |   3 ++-
 tests/qtest/tpm-tis-test.c                    |   3 ++-
 tests/qtest/tpm-tis-util.c                    |   2 +-
 11 files changed, 55 insertions(+), 20 deletions(-)
 create mode 100644 tests/data/acpi/q35/DSDT.tis.tpm12
 rename tests/data/acpi/q35/{DSDT.tis => DSDT.tis.tpm2} (100%)
 create mode 100644 tests/data/acpi/q35/TCPA.tis.tpm12
 rename tests/data/acpi/q35/{TPM2.tis => TPM2.tis.tpm2} (100%)

Comments

Philippe Mathieu-Daudé July 12, 2021, 3:29 p.m. UTC | #1
Hi Stefan,

On 7/12/21 5:09 PM, Stefan Berger wrote:
> This series of patches adds test case for TPM 1.2 ACPI tables.
> 
>   Stefan
> 
> v3:
>   - Define enum TPMVersion for when CONFIG_TPM is not defined
>     affected patches 2 and 6

I think in 11fb99e6f48..e542b71805d we missed an extra patch
for qtests. Probably (untested):

-- >8 --
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index ee7347b7275..eeaa0d7302b 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -60,10 +60,14 @@
   (config_all_devices.has_key('CONFIG_USB_UHCI') and
                     \
    config_all_devices.has_key('CONFIG_USB_EHCI') ?
['usb-hcd-ehci-test'] : []) +            \
   (config_all_devices.has_key('CONFIG_USB_XHCI_NEC') ?
['usb-hcd-xhci-test'] : []) +        \
-  (config_all_devices.has_key('CONFIG_TPM_CRB') ? ['tpm-crb-test'] :
[]) +                  \
-  (config_all_devices.has_key('CONFIG_TPM_CRB') ?
['tpm-crb-swtpm-test'] : []) +            \
-  (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-test']
: []) +              \
-  (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ?
['tpm-tis-swtpm-test'] : []) +        \
+  (config_host.has_key('CONFIG_TPM') and \
+   config_all_devices.has_key('CONFIG_TPM_CRB') ? ['tpm-crb-test'] :
[]) +                  \
+  (config_host.has_key('CONFIG_TPM') and \
+   config_all_devices.has_key('CONFIG_TPM_CRB') ?
['tpm-crb-swtpm-test'] : []) +            \
+  (config_host.has_key('CONFIG_TPM') and \
+   config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-test']
: []) +              \
+  (config_host.has_key('CONFIG_TPM') and \
+   config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ?
['tpm-tis-swtpm-test'] : []) +        \
   (config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test']
: []) +              \
   (config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ?
['fuzz-e1000e-test'] : []) +   \
   (config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] :
[]) +                 \
---

Cc'ing Paolo/Thomas because there surely exists a clever way to do it...
Stefan Berger July 12, 2021, 3:47 p.m. UTC | #2
On 7/12/21 11:29 AM, Philippe Mathieu-Daudé wrote:
> Hi Stefan,
>
> On 7/12/21 5:09 PM, Stefan Berger wrote:
>> This series of patches adds test case for TPM 1.2 ACPI tables.
>>
>>    Stefan
>>
>> v3:
>>    - Define enum TPMVersion for when CONFIG_TPM is not defined
>>      affected patches 2 and 6
> I think in 11fb99e6f48..e542b71805d we missed an extra patch
> for qtests. Probably (untested):

Shouldn't we have seen test compilation errors already?

I didn't go down this route for the build system (as you show below) 
because in this series we are testing ACPI tables and I introduce the 
reference to enum TPMVersion here, which wasn't needed before. The 
alternative may be to go into 8/9 and eliminate all TPM code if 
CONFIG_TPM is not set. The introduction of the enum now passes the tests 
with --enable-tpm and --disable-tpm.

Otherwise the BIOS test are skipped due to this here:


static void test_acpi_tcg_tpm(const char *machine, const char *tpm_if,
                               uint64_t base, enum TPMVersion tpm_version)
{
#ifdef CONFIG_TPM
[...]

#else
     g_test_skip("TPM disabled");
#endif
}

So I didn't want to clutter this code with more #ifdef CONFIG_TPM but 
maybe that would be the right solution.

     Stefan

>
> -- >8 --
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index ee7347b7275..eeaa0d7302b 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -60,10 +60,14 @@
>     (config_all_devices.has_key('CONFIG_USB_UHCI') and
>                       \
>      config_all_devices.has_key('CONFIG_USB_EHCI') ?
> ['usb-hcd-ehci-test'] : []) +            \
>     (config_all_devices.has_key('CONFIG_USB_XHCI_NEC') ?
> ['usb-hcd-xhci-test'] : []) +        \
> -  (config_all_devices.has_key('CONFIG_TPM_CRB') ? ['tpm-crb-test'] :
> []) +                  \
> -  (config_all_devices.has_key('CONFIG_TPM_CRB') ?
> ['tpm-crb-swtpm-test'] : []) +            \
> -  (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-test']
> : []) +              \
> -  (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ?
> ['tpm-tis-swtpm-test'] : []) +        \
> +  (config_host.has_key('CONFIG_TPM') and \
> +   config_all_devices.has_key('CONFIG_TPM_CRB') ? ['tpm-crb-test'] :
> []) +                  \
> +  (config_host.has_key('CONFIG_TPM') and \
> +   config_all_devices.has_key('CONFIG_TPM_CRB') ?
> ['tpm-crb-swtpm-test'] : []) +            \
> +  (config_host.has_key('CONFIG_TPM') and \
> +   config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-test']
> : []) +              \
> +  (config_host.has_key('CONFIG_TPM') and \
> +   config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ?
> ['tpm-tis-swtpm-test'] : []) +        \
>     (config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test']
> : []) +              \
>     (config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ?
> ['fuzz-e1000e-test'] : []) +   \
>     (config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] :
> []) +                 \
> ---
>
> Cc'ing Paolo/Thomas because there surely exists a clever way to do it...
>
Philippe Mathieu-Daudé July 12, 2021, 4:26 p.m. UTC | #3
+Markus

On 7/12/21 5:47 PM, Stefan Berger wrote:
> 
> On 7/12/21 11:29 AM, Philippe Mathieu-Daudé wrote:
>> Hi Stefan,
>>
>> On 7/12/21 5:09 PM, Stefan Berger wrote:
>>> This series of patches adds test case for TPM 1.2 ACPI tables.
>>>
>>>    Stefan
>>>
>>> v3:
>>>    - Define enum TPMVersion for when CONFIG_TPM is not defined
>>>      affected patches 2 and 6
>> I think in 11fb99e6f48..e542b71805d we missed an extra patch
>> for qtests. Probably (untested):
> 
> Shouldn't we have seen test compilation errors already?
> 
> I didn't go down this route for the build system (as you show below)
> because in this series we are testing ACPI tables and I introduce the
> reference to enum TPMVersion here, which wasn't needed before. The
> alternative may be to go into 8/9 and eliminate all TPM code if
> CONFIG_TPM is not set. The introduction of the enum now passes the tests
> with --enable-tpm and --disable-tpm.
> 
> Otherwise the BIOS test are skipped due to this here:
> 
> 
> static void test_acpi_tcg_tpm(const char *machine, const char *tpm_if,
>                               uint64_t base, enum TPMVersion tpm_version)
> {
> #ifdef CONFIG_TPM
> [...]
> 
> #else
>     g_test_skip("TPM disabled");
> #endif
> }
> 
> So I didn't want to clutter this code with more #ifdef CONFIG_TPM but
> maybe that would be the right solution.

IMO the "right" solution is to check via QMP if TMP is supported
or not. This is now doable since commit caff255a546 ("tpm: Return
QMP error when TPM is disabled in build").

Long term we'd like to decouple the tests/ build from the various
QEMU configurations, and build the tests once.

>> -- >8 --
>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>> index ee7347b7275..eeaa0d7302b 100644
>> --- a/tests/qtest/meson.build
>> +++ b/tests/qtest/meson.build
>> @@ -60,10 +60,14 @@
>>     (config_all_devices.has_key('CONFIG_USB_UHCI') and
>>                       \
>>      config_all_devices.has_key('CONFIG_USB_EHCI') ?
>> ['usb-hcd-ehci-test'] : []) +            \
>>     (config_all_devices.has_key('CONFIG_USB_XHCI_NEC') ?
>> ['usb-hcd-xhci-test'] : []) +        \
>> -  (config_all_devices.has_key('CONFIG_TPM_CRB') ? ['tpm-crb-test'] :
>> []) +                  \
>> -  (config_all_devices.has_key('CONFIG_TPM_CRB') ?
>> ['tpm-crb-swtpm-test'] : []) +            \
>> -  (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-test']
>> : []) +              \
>> -  (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ?
>> ['tpm-tis-swtpm-test'] : []) +        \
>> +  (config_host.has_key('CONFIG_TPM') and \
>> +   config_all_devices.has_key('CONFIG_TPM_CRB') ? ['tpm-crb-test'] :
>> []) +                  \
>> +  (config_host.has_key('CONFIG_TPM') and \
>> +   config_all_devices.has_key('CONFIG_TPM_CRB') ?
>> ['tpm-crb-swtpm-test'] : []) +            \
>> +  (config_host.has_key('CONFIG_TPM') and \
>> +   config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-test']
>> : []) +              \
>> +  (config_host.has_key('CONFIG_TPM') and \
>> +   config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ?
>> ['tpm-tis-swtpm-test'] : []) +        \
>>     (config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test']
>> : []) +              \
>>     (config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ?
>> ['fuzz-e1000e-test'] : []) +   \
>>     (config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] :
>> []) +                 \
>> ---
>>
>> Cc'ing Paolo/Thomas because there surely exists a clever way to do it...
>>
>
Stefan Berger July 12, 2021, 6:10 p.m. UTC | #4
On 7/12/21 12:26 PM, Philippe Mathieu-Daudé wrote:
> +Markus
>
> On 7/12/21 5:47 PM, Stefan Berger wrote:
>> On 7/12/21 11:29 AM, Philippe Mathieu-Daudé wrote:
>>> Hi Stefan,
>>>
>>> On 7/12/21 5:09 PM, Stefan Berger wrote:
>>>> This series of patches adds test case for TPM 1.2 ACPI tables.
>>>>
>>>>     Stefan
>>>>
>>>> v3:
>>>>     - Define enum TPMVersion for when CONFIG_TPM is not defined
>>>>       affected patches 2 and 6
>>> I think in 11fb99e6f48..e542b71805d we missed an extra patch
>>> for qtests. Probably (untested):
>> Shouldn't we have seen test compilation errors already?
>>
>> I didn't go down this route for the build system (as you show below)
>> because in this series we are testing ACPI tables and I introduce the
>> reference to enum TPMVersion here, which wasn't needed before. The
>> alternative may be to go into 8/9 and eliminate all TPM code if
>> CONFIG_TPM is not set. The introduction of the enum now passes the tests
>> with --enable-tpm and --disable-tpm.
>>
>> Otherwise the BIOS test are skipped due to this here:
>>
>>
>> static void test_acpi_tcg_tpm(const char *machine, const char *tpm_if,
>>                                uint64_t base, enum TPMVersion tpm_version)
>> {
>> #ifdef CONFIG_TPM
>> [...]
>>
>> #else
>>      g_test_skip("TPM disabled");
>> #endif
>> }
>>
>> So I didn't want to clutter this code with more #ifdef CONFIG_TPM but
>> maybe that would be the right solution.
> IMO the "right" solution is to check via QMP if TMP is supported
> or not. This is now doable since commit caff255a546 ("tpm: Return
> QMP error when TPM is disabled in build").

That above g_test_skip() could be moved to the top of the function and 
be preceded by a QMP check for whether TPM is supported.
Markus Armbruster July 14, 2021, 2:43 p.m. UTC | #5
Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> +Markus
>
> On 7/12/21 5:47 PM, Stefan Berger wrote:
>> 
>> On 7/12/21 11:29 AM, Philippe Mathieu-Daudé wrote:
>>> Hi Stefan,
>>>
>>> On 7/12/21 5:09 PM, Stefan Berger wrote:
>>>> This series of patches adds test case for TPM 1.2 ACPI tables.
>>>>
>>>>    Stefan
>>>>
>>>> v3:
>>>>    - Define enum TPMVersion for when CONFIG_TPM is not defined
>>>>      affected patches 2 and 6
>>> I think in 11fb99e6f48..e542b71805d we missed an extra patch
>>> for qtests. Probably (untested):
>> 
>> Shouldn't we have seen test compilation errors already?
>> 
>> I didn't go down this route for the build system (as you show below)
>> because in this series we are testing ACPI tables and I introduce the
>> reference to enum TPMVersion here, which wasn't needed before. The
>> alternative may be to go into 8/9 and eliminate all TPM code if
>> CONFIG_TPM is not set. The introduction of the enum now passes the tests
>> with --enable-tpm and --disable-tpm.
>> 
>> Otherwise the BIOS test are skipped due to this here:
>> 
>> 
>> static void test_acpi_tcg_tpm(const char *machine, const char *tpm_if,
>>                               uint64_t base, enum TPMVersion tpm_version)
>> {
>> #ifdef CONFIG_TPM
>> [...]
>> 
>> #else
>>     g_test_skip("TPM disabled");
>> #endif
>> }
>> 
>> So I didn't want to clutter this code with more #ifdef CONFIG_TPM but
>> maybe that would be the right solution.
>
> IMO the "right" solution is to check via QMP if TMP is supported
> or not. This is now doable since commit caff255a546 ("tpm: Return
> QMP error when TPM is disabled in build").
>
> Long term we'd like to decouple the tests/ build from the various
> QEMU configurations, and build the tests once.

This argument applies only to macros from target-specific headers like
$TARGET-config-target.h, not to macros from config-host.h.  #ifdef
CONFIG_TPM should be fine, shouldn't it?
Philippe Mathieu-Daudé July 14, 2021, 8:16 p.m. UTC | #6
On 7/14/21 4:43 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> +Markus
>>
>> On 7/12/21 5:47 PM, Stefan Berger wrote:
>>>
>>> On 7/12/21 11:29 AM, Philippe Mathieu-Daudé wrote:
>>>> Hi Stefan,
>>>>
>>>> On 7/12/21 5:09 PM, Stefan Berger wrote:
>>>>> This series of patches adds test case for TPM 1.2 ACPI tables.
>>>>>
>>>>>    Stefan
>>>>>
>>>>> v3:
>>>>>    - Define enum TPMVersion for when CONFIG_TPM is not defined
>>>>>      affected patches 2 and 6
>>>> I think in 11fb99e6f48..e542b71805d we missed an extra patch
>>>> for qtests. Probably (untested):
>>>
>>> Shouldn't we have seen test compilation errors already?
>>>
>>> I didn't go down this route for the build system (as you show below)
>>> because in this series we are testing ACPI tables and I introduce the
>>> reference to enum TPMVersion here, which wasn't needed before. The
>>> alternative may be to go into 8/9 and eliminate all TPM code if
>>> CONFIG_TPM is not set. The introduction of the enum now passes the tests
>>> with --enable-tpm and --disable-tpm.
>>>
>>> Otherwise the BIOS test are skipped due to this here:
>>>
>>>
>>> static void test_acpi_tcg_tpm(const char *machine, const char *tpm_if,
>>>                               uint64_t base, enum TPMVersion tpm_version)
>>> {
>>> #ifdef CONFIG_TPM
>>> [...]
>>>
>>> #else
>>>     g_test_skip("TPM disabled");
>>> #endif
>>> }
>>>
>>> So I didn't want to clutter this code with more #ifdef CONFIG_TPM but
>>> maybe that would be the right solution.
>>
>> IMO the "right" solution is to check via QMP if TMP is supported
>> or not. This is now doable since commit caff255a546 ("tpm: Return
>> QMP error when TPM is disabled in build").
>>
>> Long term we'd like to decouple the tests/ build from the various
>> QEMU configurations, and build the tests once.
> 
> This argument applies only to macros from target-specific headers like
> $TARGET-config-target.h, not to macros from config-host.h.  #ifdef
> CONFIG_TPM should be fine, shouldn't it?

Some definitions depend on the host (OS, libraries installed, ...),
others depend on the --enable/--disable ./configure options.

IMO it would be nice if we could get qtests independent of the latter.

I suppose config-host.h holds both kinds.
Markus Armbruster July 15, 2021, 5:49 a.m. UTC | #7
Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 7/14/21 4:43 PM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>> 
>>> +Markus

[...]

>>> IMO the "right" solution is to check via QMP if TMP is supported
>>> or not. This is now doable since commit caff255a546 ("tpm: Return
>>> QMP error when TPM is disabled in build").
>>>
>>> Long term we'd like to decouple the tests/ build from the various
>>> QEMU configurations, and build the tests once.
>> 
>> This argument applies only to macros from target-specific headers like
>> $TARGET-config-target.h, not to macros from config-host.h.  #ifdef
>> CONFIG_TPM should be fine, shouldn't it?
>
> Some definitions depend on the host (OS, libraries installed, ...),
> others depend on the --enable/--disable ./configure options.
>
> IMO it would be nice if we could get qtests independent of the latter.

Why?

> I suppose config-host.h holds both kinds.

Yes.
Igor Mammedov July 19, 2021, 1:38 p.m. UTC | #8
On Thu, 15 Jul 2021 07:49:13 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
> > On 7/14/21 4:43 PM, Markus Armbruster wrote:  
> >> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> >>   
> >>> +Markus  
> 
> [...]
> 
> >>> IMO the "right" solution is to check via QMP if TMP is supported
> >>> or not. This is now doable since commit caff255a546 ("tpm: Return
> >>> QMP error when TPM is disabled in build").
> >>>
> >>> Long term we'd like to decouple the tests/ build from the various
> >>> QEMU configurations, and build the tests once.  
> >> 
> >> This argument applies only to macros from target-specific headers like
> >> $TARGET-config-target.h, not to macros from config-host.h.  #ifdef
> >> CONFIG_TPM should be fine, shouldn't it?  
> >
> > Some definitions depend on the host (OS, libraries installed, ...),
> > others depend on the --enable/--disable ./configure options.
> >
> > IMO it would be nice if we could get qtests independent of the latter.  
> 
> Why?

In another mail-thread Philippe mentioned that there is desire
to use qtest out of tree to test other QEMU binaries.

However, just probing for features at runtime aren't going
to help with the goal as tests are tailored for the latest
CLI/QMP/ABI. To make it work we would have practically
introduce versioned tests.

So I wonder why one external acceptance-tests suite is not
sufficient, that we would want to hijack relatively simple
internal qtest at expense of increased resources needed to
run/write unit tests.

> > I suppose config-host.h holds both kinds.  
> 
> Yes.
> 
>
Markus Armbruster July 19, 2021, 1:50 p.m. UTC | #9
Igor Mammedov <imammedo@redhat.com> writes:

> On Thu, 15 Jul 2021 07:49:13 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>> 
>> > On 7/14/21 4:43 PM, Markus Armbruster wrote:  
>> >> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>> >>   
>> >>> +Markus  
>> 
>> [...]
>> 
>> >>> IMO the "right" solution is to check via QMP if TMP is supported
>> >>> or not. This is now doable since commit caff255a546 ("tpm: Return
>> >>> QMP error when TPM is disabled in build").
>> >>>
>> >>> Long term we'd like to decouple the tests/ build from the various
>> >>> QEMU configurations, and build the tests once.  
>> >> 
>> >> This argument applies only to macros from target-specific headers like
>> >> $TARGET-config-target.h, not to macros from config-host.h.  #ifdef
>> >> CONFIG_TPM should be fine, shouldn't it?  
>> >
>> > Some definitions depend on the host (OS, libraries installed, ...),
>> > others depend on the --enable/--disable ./configure options.
>> >
>> > IMO it would be nice if we could get qtests independent of the latter.  
>> 
>> Why?
>
> In another mail-thread Philippe mentioned that there is desire
> to use qtest out of tree to test other QEMU binaries.
>
> However, just probing for features at runtime aren't going
> to help with the goal as tests are tailored for the latest
> CLI/QMP/ABI. To make it work we would have practically
> introduce versioned tests.
>
> So I wonder why one external acceptance-tests suite is not
> sufficient, that we would want to hijack relatively simple
> internal qtest at expense of increased resources needed to
> run/write unit tests.

Yes.  qtest was not designed for use with anything but HEAD, and I doubt
we can make it fit such uses at reasonable expense.

>
>> > I suppose config-host.h holds both kinds.  
>> 
>> Yes.
>> 
>>
Philippe Mathieu-Daudé July 19, 2021, 1:56 p.m. UTC | #10
On Mon, Jul 19, 2021 at 3:50 PM Markus Armbruster <armbru@redhat.com> wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
> > On Thu, 15 Jul 2021 07:49:13 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >> Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> >> >>> IMO the "right" solution is to check via QMP if TMP is supported
> >> >>> or not. This is now doable since commit caff255a546 ("tpm: Return
> >> >>> QMP error when TPM is disabled in build").
> >> >>>
> >> >>> Long term we'd like to decouple the tests/ build from the various
> >> >>> QEMU configurations, and build the tests once.
> >> >>
> >> >> This argument applies only to macros from target-specific headers like
> >> >> $TARGET-config-target.h, not to macros from config-host.h.  #ifdef
> >> >> CONFIG_TPM should be fine, shouldn't it?
> >> >
> >> > Some definitions depend on the host (OS, libraries installed, ...),
> >> > others depend on the --enable/--disable ./configure options.
> >> >
> >> > IMO it would be nice if we could get qtests independent of the latter.
> >>
> >> Why?
> >
> > In another mail-thread Philippe mentioned that there is desire
> > to use qtest out of tree to test other QEMU binaries.
> >
> > However, just probing for features at runtime aren't going
> > to help with the goal as tests are tailored for the latest
> > CLI/QMP/ABI. To make it work we would have practically
> > introduce versioned tests.
> >
> > So I wonder why one external acceptance-tests suite is not
> > sufficient, that we would want to hijack relatively simple
> > internal qtest at expense of increased resources needed to
> > run/write unit tests.
>
> Yes.  qtest was not designed for use with anything but HEAD, and I doubt
> we can make it fit such uses at reasonable expense.

One HEAD but multiple configurations...

If you want to simplify human time, can we simply run qtests once per
arch/OS but with all features enabled? Otherwise skip qtests?
Igor Mammedov July 19, 2021, 2:44 p.m. UTC | #11
On Mon, 19 Jul 2021 15:56:16 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On Mon, Jul 19, 2021 at 3:50 PM Markus Armbruster <armbru@redhat.com> wrote:
> > Igor Mammedov <imammedo@redhat.com> writes:  
> > > On Thu, 15 Jul 2021 07:49:13 +0200
> > > Markus Armbruster <armbru@redhat.com> wrote:  
> > >> Philippe Mathieu-Daudé <philmd@redhat.com> writes:  
> 
> > >> >>> IMO the "right" solution is to check via QMP if TMP is supported
> > >> >>> or not. This is now doable since commit caff255a546 ("tpm: Return
> > >> >>> QMP error when TPM is disabled in build").
> > >> >>>
> > >> >>> Long term we'd like to decouple the tests/ build from the various
> > >> >>> QEMU configurations, and build the tests once.  
> > >> >>
> > >> >> This argument applies only to macros from target-specific headers like
> > >> >> $TARGET-config-target.h, not to macros from config-host.h.  #ifdef
> > >> >> CONFIG_TPM should be fine, shouldn't it?  
> > >> >
> > >> > Some definitions depend on the host (OS, libraries installed, ...),
> > >> > others depend on the --enable/--disable ./configure options.
> > >> >
> > >> > IMO it would be nice if we could get qtests independent of the latter.  
> > >>
> > >> Why?  
> > >
> > > In another mail-thread Philippe mentioned that there is desire
> > > to use qtest out of tree to test other QEMU binaries.
> > >
> > > However, just probing for features at runtime aren't going
> > > to help with the goal as tests are tailored for the latest
> > > CLI/QMP/ABI. To make it work we would have practically
> > > introduce versioned tests.
> > >
> > > So I wonder why one external acceptance-tests suite is not
> > > sufficient, that we would want to hijack relatively simple
> > > internal qtest at expense of increased resources needed to
> > > run/write unit tests.  
> >
> > Yes.  qtest was not designed for use with anything but HEAD, and I doubt
> > we can make it fit such uses at reasonable expense.  
> 
> One HEAD but multiple configurations...

Even assuming reconfigure won't cause world rebuild,
It will be a win only if number of configuration probes
is small.
However it doesn't scale for large numbers and it might be
faster to rebuild affected tests in the end. (worst case: #probes * #targets)
I wonder if we can do probing once & cache it somewhere to avoid ^^^.


> If you want to simplify human time, can we simply run qtests once per
> arch/OS but with all features enabled? Otherwise skip qtests?
>
Markus Armbruster July 19, 2021, 3:13 p.m. UTC | #12
Igor Mammedov <imammedo@redhat.com> writes:

> On Mon, 19 Jul 2021 15:56:16 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
>> On Mon, Jul 19, 2021 at 3:50 PM Markus Armbruster <armbru@redhat.com> wrote:
>> > Igor Mammedov <imammedo@redhat.com> writes:  
>> > > On Thu, 15 Jul 2021 07:49:13 +0200
>> > > Markus Armbruster <armbru@redhat.com> wrote:  
>> > >> Philippe Mathieu-Daudé <philmd@redhat.com> writes:  
>> 
>> > >> >>> IMO the "right" solution is to check via QMP if TMP is supported
>> > >> >>> or not. This is now doable since commit caff255a546 ("tpm: Return
>> > >> >>> QMP error when TPM is disabled in build").
>> > >> >>>
>> > >> >>> Long term we'd like to decouple the tests/ build from the various
>> > >> >>> QEMU configurations, and build the tests once.  
>> > >> >>
>> > >> >> This argument applies only to macros from target-specific headers like
>> > >> >> $TARGET-config-target.h, not to macros from config-host.h.  #ifdef
>> > >> >> CONFIG_TPM should be fine, shouldn't it?  
>> > >> >
>> > >> > Some definitions depend on the host (OS, libraries installed, ...),
>> > >> > others depend on the --enable/--disable ./configure options.
>> > >> >
>> > >> > IMO it would be nice if we could get qtests independent of the latter.  
>> > >>
>> > >> Why?  
>> > >
>> > > In another mail-thread Philippe mentioned that there is desire
>> > > to use qtest out of tree to test other QEMU binaries.
>> > >
>> > > However, just probing for features at runtime aren't going
>> > > to help with the goal as tests are tailored for the latest
>> > > CLI/QMP/ABI. To make it work we would have practically
>> > > introduce versioned tests.
>> > >
>> > > So I wonder why one external acceptance-tests suite is not
>> > > sufficient, that we would want to hijack relatively simple
>> > > internal qtest at expense of increased resources needed to
>> > > run/write unit tests.  
>> >
>> > Yes.  qtest was not designed for use with anything but HEAD, and I doubt
>> > we can make it fit such uses at reasonable expense.  
>> 
>> One HEAD but multiple configurations...
>
> Even assuming reconfigure won't cause world rebuild,
> It will be a win only if number of configuration probes
> is small.
> However it doesn't scale for large numbers and it might be
> faster to rebuild affected tests in the end. (worst case: #probes * #targets)
> I wonder if we can do probing once & cache it somewhere to avoid ^^^.

As far as I can tell, the time to compile these tests is dwarved several
times over by the time to run them.  In other words, most of the waste
to optimize out hides in the test programs, not in compiling them.

>> If you want to simplify human time, can we simply run qtests once per
>> arch/OS but with all features enabled? Otherwise skip qtests?

How build configuration and tests interact is poorly understood.  We
instead use brute force: run all the tests for all configurations,
always.  To do better without sacrificing coverage, we need to
understand better.  Sadly, I don't see that happening.