On Mon, Feb 14, 2022 at 5:34 AM Varad Gautam <varad.gautam@xxxxxxxx> wrote:
>
> Hi Marc,
>
> On 2/13/22 4:48 AM, Marc Orr wrote:
> > On Wed, Feb 9, 2022 at 8:43 AM Varad Gautam <varad.gautam@xxxxxxxx> wrote:
> >>
> >> Make x86/efi/run check for AMDSEV envvar and set corresponding
> >> SEV/SEV-ES parameters on the qemu cmdline, to make it convenient
> >> to launch SEV/SEV-ES tests.
> >>
> >> Since the C-bit position depends on the runtime host, fetch it
> >> via cpuid before guest launch.
> >>
> >> AMDSEV can be set to `sev` or `sev-es`.
> >>
> >> Signed-off-by: Varad Gautam <varad.gautam@xxxxxxxx>
> >> ---
> >> x86/efi/README.md | 5 +++++
> >> x86/efi/run | 38 ++++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 43 insertions(+)
> >>
> >> diff --git a/x86/efi/README.md b/x86/efi/README.md
> >> index a39f509..1222b30 100644
> >> --- a/x86/efi/README.md
> >> +++ b/x86/efi/README.md
> >> @@ -30,6 +30,11 @@ the env variable `EFI_UEFI`:
> >>
> >> EFI_UEFI=/path/to/OVMF.fd ./x86/efi/run ./x86/msr.efi
> >>
> >> +To run the tests under AMD SEV/SEV-ES, set env variable `AMDSEV=sev` or
> >> +`AMDSEV=sev-es`. This adds the desired guest policy to qemu command line.
> >> +
> >> + AMDSEV=sev-es EFI_UEFI=/path/to/OVMF.fd ./x86/efi/run ./x86/amd_sev.efi
> >> +
> >> ## Code structure
> >>
> >> ### Code from GNU-EFI
> >> diff --git a/x86/efi/run b/x86/efi/run
> >> index ac368a5..9bf0dc8 100755
> >> --- a/x86/efi/run
> >> +++ b/x86/efi/run
> >> @@ -43,6 +43,43 @@ fi
> >> mkdir -p "$EFI_CASE_DIR"
> >> cp "$EFI_SRC/$EFI_CASE.efi" "$EFI_CASE_BINARY"
> >>
> >> +amdsev_opts=
> >> +if [ -n "$AMDSEV" ]; then
> >> + # Guest policy bits, used to form QEMU command line.
> >> + readonly AMDSEV_POLICY_NODBG=$(( 1 << 0 ))
> >> + readonly AMDSEV_POLICY_ES=$(( 1 << 2 ))
> >> +
> >> + gcc -x c -o getcbitpos - <<EOF
> >> + /* CPUID Fn8000_001F_EBX bits 5:0 */
> >> + int get_cbit_pos(void)
> >> + {
> >> + int ebx;
> >> + __asm__("mov \$0x8000001f , %eax\n\t");
> >> + __asm__("cpuid\n\t");
> >> + __asm__("mov %%ebx, %0\n\t":"=r" (ebx));
> >> + return (ebx & 0x3f);
> >> + }
> >> + int main(void)
> >> + {
> >> + return get_cbit_pos();
> >> + }
> >> +EOF
> >
> > We could do this in bash directly, using the cpuid driver:
> > https://man7.org/linux/man-pages/man4/cpuid.4.html
> >
> > I'm not a Linux shell wizard, but I found an example of a script using
> > this module here:
> > https://git.irsamc.ups-tlse.fr/dsanchez/spectre-meltdown-checker/src/branch/master/spectre-meltdown-checker.sh
> >
> > After studying that script (for like an hour, lol), I was able to
> > extract the cbit position. Below, I explain how to do this with the
> > cpuid driver. My only complaint about using the cpuid driver is that
> > it's awfully hard to follow. So I'd be OK to stick with the C code
> > that you've got. Though it may be better to break out the C code into
> > an actual .c file, rather than embed it in the script like this, and
> > magically build it and clean it up, which seems pretty hacky. I know I
> > was doing something similar with a dummy.c file embedded in the run
> > script file to get the run_tests.sh script to work, and Paolo ended up
> > moving that into an explicit build target in the x86/ directory.
> >
> > Getting the c bit position in pure bash, using the cpuid driver.
> > $ ebx=$(dd if=/dev/cpu/0/cpuid bs=16 skip=134217729 count=16
> > 2>/dev/null | od -j 240 -An -t u4 | awk '{print $'"2"'}')
> > $ echo $(( $ebx&0x3f ))
> >
>
> Tom also suggested magic using the cpuid module earlier [1], but I would
> like to avoid a dependency on CONFIG_X86_CPUID here. Besides the readability
> of the C snippet, I believe gcc (ie userspace) is easier to install on a set
> of test hosts already prepared with CONFIG_X86_CPUID=n, than to
> enable/deploy/install the cpuid kmod there, which becomes important when
> testing a large number of hosts at once.
>
> Unlike x86/dummy.c, the C code doesn't run in a guest context, which is why
> I'm hesitant to place it as a build target under x86/. I "hid" it within
> the run script since it's only needed when constructing the qemu cmdline,
> and packaging a 'getcbitpos' binary didn't make much sense. Thoughts?
>
> [1] https://lore.kernel.org/kvm/1a79ea5b-71dd-2782-feba-0d733f8c2fbf@xxxxxxx/
>
> Thanks,
> Varad
Ah. Got it now. I had forgotten about Tom's reply. And you make good
points about the cpuid binary this patch is building being unique from
other binaries in that it does not run under the guest...
Also, I agree with minimizing dependencies on the machine under test.
If we go with the current approach, is there any reason not to just
split out the C code into a standalone .c file? Also, any reason not
to build it when we build the rest of the x86 binaries? I agree it
should not reside in the x86/ directory, since it is not a guest-side
program as you mentioned. But I'm wondering if we can compile it in
advance of running the test, rather than while running the test.
