[kvm-unit-tests PATCH v2] x86/efi: Allow specifying AMD SEV/SEV-ES guest launch policy to run

[kvm-unit-tests PATCH v2] x86/efi: Allow specifying AMD SEV/SEV-ES guest launch policy to run

[Varad Gautam]

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@suse.com>
---
 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
+
+	cbitpos=$(./getcbitpos ; echo $?) || rm ./getcbitpos
+	policy=
+	if [ "$AMDSEV" = "sev" ]; then
+		policy="$(( $AMDSEV_POLICY_NODBG ))"
+	elif [ "$AMDSEV" = "sev-es" ]; then
+		policy="$(( $AMDSEV_POLICY_NODBG | $AMDSEV_POLICY_ES ))"
+	else
+		echo "Cannot set AMDSEV policy. AMDSEV must be one of 'sev', 'sev-es'."
+		exit 2
+	fi
+
+	amdsev_opts="-object sev-guest,id=sev0,cbitpos=${cbitpos},reduced-phys-bits=1,policy=${policy} \
+		     -machine memory-encryption=sev0"
+fi
+
 # Run test case with 256MiB QEMU memory. QEMU default memory size is 128MiB.
 # After UEFI boot up and we call `LibMemoryMap()`, the largest consecutive
 # memory region is ~42MiB. Although this is sufficient for many test cases to
@@ -61,4 +98,5 @@ cp "$EFI_SRC/$EFI_CASE.efi" "$EFI_CASE_BINARY"
 	-nographic \
 	-m 256 \
 	"$@" \
+	$amdsev_opts \
 	-smp "$EFI_SMP"
-- 
2.34.1

Re: [kvm-unit-tests PATCH v2] x86/efi: Allow specifying AMD SEV/SEV-ES guest launch policy to run

[Marc Orr]

On Wed, Feb 9, 2022 at 8:43 AM Varad Gautam <varad.gautam@suse.com> 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@suse.com>
> ---
>  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 ))

Breaking it down:

# Use dd to read the 0x8000001f leaf via the cpuid driver:
# bs=16: block size of 16 bytes; required by the driver per it's man page
# skip=134217729: This is the CPUID leaf, 0x8000001f as a decimal number,
#      divided by the block size
# count=16: We actually only want to read a count=1 16 byte block
#      because {eax, ebx, ecx, edx} is a single 16 byte block.
However, our CPUID leaf,
#      0x8000001f, doesn't divide evenly by 16. It has a remainder of
15. So read the
#      previous 15 sixteen-byte blocks, plus the block we actually want to read.
$ dd if=/dev/cpu/0/cpuid bs=16 skip=134217729 count=16 2>/dev/null

# Use od to convert the binary data returned by the cpuid driver into ascii.
# -j 240: Skip the first 15 sixteen-byte blocks that we only read to
appease the 16 byte block size. (15 * 16 = 240).
# -An: Don't label the output with indexes.
# -t u4: Output the data as 4-byte unsigned decimal #'s.
od -j 240 -An -t u4

The od command above outputs the four CPUID values {eax, ebx, ecx,
edx}. On my machine:
      65551        367        509        100

Finally, use awk to take the second one -- ebx. And then take the
lower 6 bits for the c-bit position.

> +
> +       cbitpos=$(./getcbitpos ; echo $?) || rm ./getcbitpos
> +       policy=
> +       if [ "$AMDSEV" = "sev" ]; then
> +               policy="$(( $AMDSEV_POLICY_NODBG ))"
> +       elif [ "$AMDSEV" = "sev-es" ]; then
> +               policy="$(( $AMDSEV_POLICY_NODBG | $AMDSEV_POLICY_ES ))"
> +       else
> +               echo "Cannot set AMDSEV policy. AMDSEV must be one of 'sev', 'sev-es'."
> +               exit 2
> +       fi
> +
> +       amdsev_opts="-object sev-guest,id=sev0,cbitpos=${cbitpos},reduced-phys-bits=1,policy=${policy} \
> +                    -machine memory-encryption=sev0"
> +fi
> +
>  # Run test case with 256MiB QEMU memory. QEMU default memory size is 128MiB.
>  # After UEFI boot up and we call `LibMemoryMap()`, the largest consecutive
>  # memory region is ~42MiB. Although this is sufficient for many test cases to
> @@ -61,4 +98,5 @@ cp "$EFI_SRC/$EFI_CASE.efi" "$EFI_CASE_BINARY"
>         -nographic \
>         -m 256 \
>         "$@" \
> +       $amdsev_opts \
>         -smp "$EFI_SMP"
> --
> 2.34.1
>

Re: [kvm-unit-tests PATCH v2] x86/efi: Allow specifying AMD SEV/SEV-ES guest launch policy to run

[Varad Gautam]

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@suse.com> 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@suse.com>
>> ---
>>  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@amd.com/

Thanks,
Varad


> Breaking it down:
> 
> # Use dd to read the 0x8000001f leaf via the cpuid driver:
> # bs=16: block size of 16 bytes; required by the driver per it's man page
> # skip=134217729: This is the CPUID leaf, 0x8000001f as a decimal number,
> #      divided by the block size
> # count=16: We actually only want to read a count=1 16 byte block
> #      because {eax, ebx, ecx, edx} is a single 16 byte block.
> However, our CPUID leaf,
> #      0x8000001f, doesn't divide evenly by 16. It has a remainder of
> 15. So read the
> #      previous 15 sixteen-byte blocks, plus the block we actually want to read.
> $ dd if=/dev/cpu/0/cpuid bs=16 skip=134217729 count=16 2>/dev/null
> 
> # Use od to convert the binary data returned by the cpuid driver into ascii.
> # -j 240: Skip the first 15 sixteen-byte blocks that we only read to
> appease the 16 byte block size. (15 * 16 = 240).
> # -An: Don't label the output with indexes.
> # -t u4: Output the data as 4-byte unsigned decimal #'s.
> od -j 240 -An -t u4
> 
> The od command above outputs the four CPUID values {eax, ebx, ecx,
> edx}. On my machine:
>       65551        367        509        100
> 
> Finally, use awk to take the second one -- ebx. And then take the
> lower 6 bits for the c-bit position.
> 
>> +
>> +       cbitpos=$(./getcbitpos ; echo $?) || rm ./getcbitpos
>> +       policy=
>> +       if [ "$AMDSEV" = "sev" ]; then
>> +               policy="$(( $AMDSEV_POLICY_NODBG ))"
>> +       elif [ "$AMDSEV" = "sev-es" ]; then
>> +               policy="$(( $AMDSEV_POLICY_NODBG | $AMDSEV_POLICY_ES ))"
>> +       else
>> +               echo "Cannot set AMDSEV policy. AMDSEV must be one of 'sev', 'sev-es'."
>> +               exit 2
>> +       fi
>> +
>> +       amdsev_opts="-object sev-guest,id=sev0,cbitpos=${cbitpos},reduced-phys-bits=1,policy=${policy} \
>> +                    -machine memory-encryption=sev0"
>> +fi
>> +
>>  # Run test case with 256MiB QEMU memory. QEMU default memory size is 128MiB.
>>  # After UEFI boot up and we call `LibMemoryMap()`, the largest consecutive
>>  # memory region is ~42MiB. Although this is sufficient for many test cases to
>> @@ -61,4 +98,5 @@ cp "$EFI_SRC/$EFI_CASE.efi" "$EFI_CASE_BINARY"
>>         -nographic \
>>         -m 256 \
>>         "$@" \
>> +       $amdsev_opts \
>>         -smp "$EFI_SMP"
>> --
>> 2.34.1
>>
> 

Re: [kvm-unit-tests PATCH v2] x86/efi: Allow specifying AMD SEV/SEV-ES guest launch policy to run

[Marc Orr]

On Mon, Feb 14, 2022 at 5:34 AM Varad Gautam <varad.gautam@suse.com> 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@suse.com> 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@suse.com>
> >> ---
> >>  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@amd.com/
>
> 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.