Re: [kvm-unit-tests RFC PATCH 1/5] efi: Compile standalone binaries for EFI

Re: [kvm-unit-tests RFC PATCH 1/5] efi: Compile standalone binaries for EFI

[Sean Christopherson]

On Tue, Aug 16, 2022, Zixuan Wang wrote:
> Currently the standalone binaries do not work with EFI as the scripts
> are not aware of EFI-related files. More specifically, the scripts only
> search for .flat files, but EFI binaries are .efi files.
> 
> This patch fixes this by introducing a new 'efi' option for

Avoid this patch, and phrase changelogs as command.  E.g.

  Introduce an "efi" option in unittests.cfg <reasoning...>

> unittests.cfg. This patch does not contain any modifications to use this
> new efi option. Those updates will be folded into the follow-up patch.
> 
> Signed-off-by: Zixuan Wang <zxwang@fb.com>
> ---
>  scripts/common.bash     | 24 ++++++++++++++++++++++--
>  scripts/mkstandalone.sh | 17 ++++++++++++++++-
>  x86/unittests.cfg       |  3 +++
>  3 files changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/common.bash b/scripts/common.bash
> index 7b983f7..7af9d62 100644
> --- a/scripts/common.bash
> +++ b/scripts/common.bash
> @@ -1,5 +1,21 @@
>  source config.mak
>  
> +function unittest_enabled()
> +{
> +	test_name="$1"
> +	test_efi="$2"
> +
> +	if [ -z "${test_name}" ]; then
> +		false
> +	elif [ "${CONFIG_EFI}" == "y" ] && [ "${test_efi}" == "no" ]; then
> +		false
> +	elif [ "${CONFIG_EFI}" == "n" ] && [ "${test_efi}" == "only" ]; then

Having to tag every test as EFI-friendly is going to get annoying, and without
context it's not super obvious that "efi = yes" means EFI-friendly _and_ legacy-
friendly.

Rather than "efi = {yes,no,only}", what about "efi = {unsupported,required}"?
I.e. tag only tests that don't support all flavors of firmware.

Re: [kvm-unit-tests RFC PATCH 2/5] x86/efi: Fix efi runner scripts for standalone

[Sean Christopherson]

On Tue, Aug 16, 2022, Zixuan Wang wrote:
> Fix the efi runner scripts to run in the standalone mode:

Capitalize EFI for consistency.

> 1. Define the `x86/run` qemu runner as a function because `x86/run` is
> embedded into the standalone and cannot be called using its file name.
> 
> 2. Disabling the `config.mak` checks in the standalone mode as it's not
> available.

s/Disabling/Disable

> 
> 3. Use the dummy test file name provided by standalone's EFI_DUMMY
> variable, because the dummy test case is embedded into the standalone
> files and exported as a tmp file at run time.

This patch probably needs to be split into three patches, one for each of the
above changes.  A changelog that contains a list of things that are fixed is usually
a good hint that the patch is bundling too many things together.

Re: [kvm-unit-tests RFC PATCH 3/5] x86/efi: Update unittests.cfg to build standalones

[Sean Christopherson]

On Tue, Aug 16, 2022, Zixuan Wang wrote:
> This patch updates all EFI test cases with an 'efi' option in

Avoid "This patch".

> unittests.cfg to build them as standalone test cases.
> 
> With this patch, `make standalone` and `make install` should generate
> standalone EFI binaries, instead of reporting file not found errors.

Isn't this a fix that unrelated to standalone mode?  E.g. I see the same behavior
when running KUT in non-standalone mode due run_tests.sh trying to run tests that
aren't built for EFI.

Re: [kvm-unit-tests RFC PATCH 5/5] x86/efi: Update README with standalone instructions

[Sean Christopherson]

On Tue, Aug 16, 2022, Zixuan Wang wrote:
> Update the `x86/efi/README.md` with instructions to build and run
> standalone test cases with UEFI.
> 
> Signed-off-by: Zixuan Wang <zxwang@fb.com>
> ---
>  x86/efi/README.md | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/x86/efi/README.md b/x86/efi/README.md
> index aa1dbcd..f740225 100644
> --- a/x86/efi/README.md
> +++ b/x86/efi/README.md
> @@ -30,6 +30,16 @@ the env variable `EFI_UEFI`:
>  
>      EFI_UEFI=/path/to/OVMF.fd ./x86/efi/run ./x86/msr.efi
>  
> +### Build and standalone test cases with UEFI

s/and// ?

> +
> +To build:
> +
> +    ./configure --enable-efi
> +    make standalone
> +    (send tests/some-test somewhere)
> +    (go to somewhere)
> +    EFI_UEFI=/path/to/OVMF.fd ./some-test

This makes me wonder if the scripts should try to locate OVMF.fd, same as they
do for QEMU.  It's odd that specifying QEMU is optional, but for EFI_UEFI it's
mandatory.