kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Marc Orr <marcorr@google.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Zixuan Wang <zxwang42@gmail.com>,
	kvm@vger.kernel.org, drjones@redhat.com, erdemaktas@google.com,
	rientjes@google.com, seanjc@google.com, brijesh.singh@amd.com,
	Thomas.Lendacky@amd.com, varad.gautam@suse.com, jroedel@suse.de,
	bp@suse.de
Subject: Re: [kvm-unit-tests PATCH v1 0/7] x86_64 UEFI set up process refactor and scripts fixes
Date: Sun, 31 Oct 2021 09:14:25 -0700	[thread overview]
Message-ID: <CAA03e5GZ6HnW8uk+2nh_vZcKvtt+wcdVchm4cjRm_yPFC-P7Eg@mail.gmail.com> (raw)
In-Reply-To: <d6c56f03-1da7-1ebf-1d2e-0ec1aa0b241c@redhat.com>

On Sun, Oct 31, 2021 at 12:28 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 31/10/21 06:56, Zixuan Wang wrote:
> > Hello,
> >
> > This patch series refactors the x86_64 UEFI set up process and fixes the
> > `run-tests.sh` script to run under UEFI. The patches are organized as
> > three parts.
> >
> > The first part (patches 1-2) refactors the x86_64 UEFI set up process.
> > The previous UEFI setup calls arch-specific setup functions twice and
> > generates arch-specific data structure. As Andrew suggested [1], we
> > refactor this process to make only one call to the arch-specific
> > function and generate arch-neutral data structures. This simplifies the
> > set up process and makes it easier to develop UEFI support for other
> > architectures.
> >
> > The second part (patch 3) converts several x86 test cases to
> > Position-Independent Code (PIC) to run under UEFI. This patch is ported
> > from the initial UEFI support patchset [2] with fixes to the 32-bit
> > compilation.
> >
> > The third part (patches 4-7) fixes the UEFI runner scripts. Patch 4 sets
> > UEFI OVMF image as readonly. Patch 5 fixes test cases' return code under
> > UEFI, enabling Patch 6-7 to fix the `run-tests.sh` script under UEFI.
> >
> > This patch set is based on the `uefi` branch.
>
> Thank you, for patches 1-6 I have squashed the patches when applicable
> (1, 4, 5, 6) and queued the others (2 and 3).
>
> I did not queue patch 7 yet, it seems okay but I want to understand
> better the changes it needs in the harness and what is missing.  I'll
> take a look during the week.

SGTM, thank you! Zixuan and I discussed a few things that are missing:

1. Test cases that take the `-append` arg are currently marked `SKIP`.
Two issues need to be resolved here. First, we're not using QEMU's
`-kernel` flag for EFI test cases [1]. And the `-append` flag does not
work without the `-kernel` flag. I don't understand the details on why
we don't use the `-kernel` flag myself. Maybe Zixuan can elaborate.
Second, assuming we fix the first issue, then we need to enlighten the
KVM-Unit-Tests under UEFI to parse kernel command line arguments and
pass them down to the test cases via `argv`. Zixuan pointed out to me
that there is some prior work from Drew [2] that we should be able to
follow to make this work. So I'm hoping that Zixuan and I can work
together on solving these issues to get the argument passing working
next.
2. We need a way to annotate test cases in `x86/unittests.cfg` as
known to work under SEV. I'm thinking of doing this via new (very
broad) test groups in `unittests.cfg`. I _think_ SEV is the primary
scenario we care about. However, folks may care about running the test
cases under UEFI outside of SEV. For example, last time I checked,
emulator runs OK under UEFI minus SEV-ES but fails under SEV-ES. And
similarly, while most test cases work under UEFI minus SEV, there are
a few that do mis-behave -- and it probably makes sense to document
this (e.g., via annotations in `unittests.cfg`). Also, there are many
variations of SEV (SEV, SEV-ES, SEV-SNP)... And hopefully some of this
will eventually be applicable to TDX as well. So many testgroups is
not a good solution. I'm not sure.
3. Multi-CPU needs to be made to work under UEFI. For now, patch #7
forces all EFI test cases to run with 1 vCPU. I chatted with Brijesh,
and he mentioned that Varad would like to work on this. However, if
anything here changes, please let me know, because we can work on this
as well. But for now, I'm not planning to work on it so we can avoid
duplicating work.
4. UEFI runs a lot slower than SEABIOS. It doesn't help that the test
harness launches QEMU more than once for each test case (i.e., it runs
the `_NO_FILE_4Uhere_` scenario to check QEMU arguments). I'm not sure
how much of an issue this is in practice. Depending on the answer, I
know Zixuan had some ideas on how to speed this up in the current test
harness. Or maybe we can explore an alternative to the
`_NO_FILE_4Uhere_` approach instead.

Zixuan: Please add/correct anything as needed!

[1] https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/blob/uefi/x86/run#L42-44
[2] https://github.com/rhdrjones/kvm-unit-tests/blob/target-efi/scripts/mkefi.sh

Thanks,
Marc

  reply	other threads:[~2021-10-31 16:14 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-31  5:56 [kvm-unit-tests PATCH v1 0/7] x86_64 UEFI set up process refactor and scripts fixes Zixuan Wang
2021-10-31  5:56 ` [kvm-unit-tests PATCH v1 1/7] x86 UEFI: Remove mixed_mode Zixuan Wang
2021-10-31  5:56 ` [kvm-unit-tests PATCH v1 2/7] x86 UEFI: Refactor set up process Zixuan Wang
2021-11-05 18:54   ` Sean Christopherson
2021-11-09 17:16     ` Zixuan Wang
2021-10-31  5:56 ` [kvm-unit-tests PATCH v1 3/7] x86 UEFI: Convert x86 test cases to PIC Zixuan Wang
2021-10-31  5:56 ` [kvm-unit-tests PATCH v1 4/7] x86 UEFI: Set UEFI OVMF as readonly Zixuan Wang
2021-10-31  5:56 ` [kvm-unit-tests PATCH v1 5/7] x86 UEFI: Exit QEMU with return code Zixuan Wang
2021-10-31 10:01   ` Paolo Bonzini
2021-10-31 21:36     ` Zixuan Wang
2021-10-31  5:56 ` [kvm-unit-tests PATCH v1 6/7] scripts: Generalize EFI check Zixuan Wang
2021-10-31  7:13   ` Paolo Bonzini
2021-10-31 15:35     ` Marc Orr
2021-10-31  5:56 ` [kvm-unit-tests PATCH v1 7/7] x86 UEFI: Make run_tests.sh (mostly) work under UEFI Zixuan Wang
2021-10-31  7:28 ` [kvm-unit-tests PATCH v1 0/7] x86_64 UEFI set up process refactor and scripts fixes Paolo Bonzini
2021-10-31 16:14   ` Marc Orr [this message]
2021-10-31 21:54     ` Zixuan Wang
2021-11-01  7:11       ` Andrew Jones
2021-11-01 22:35         ` Zixuan Wang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAA03e5GZ6HnW8uk+2nh_vZcKvtt+wcdVchm4cjRm_yPFC-P7Eg@mail.gmail.com \
    --to=marcorr@google.com \
    --cc=Thomas.Lendacky@amd.com \
    --cc=bp@suse.de \
    --cc=brijesh.singh@amd.com \
    --cc=drjones@redhat.com \
    --cc=erdemaktas@google.com \
    --cc=jroedel@suse.de \
    --cc=kvm@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=rientjes@google.com \
    --cc=seanjc@google.com \
    --cc=varad.gautam@suse.com \
    --cc=zxwang42@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).