sev/ghcb Add #VC handler tests

[AdamCDunlap]

Adds tests that make sure a #VC exception is raised while executing various instructions and that the #VC handler properly handles them via the ghcb. Instructions tested are cpuid and port IO.

Looking for feedback on the following:

• Is the raw-cpuid dependency OK, or would it be better to just use inline asm or something else?
• What's a good location to put these? I chose ghcb.rs, but they might make more sense somewhere else.
• I have tests for other #VC handler cases such as ones for msrs, dr7, wbinvd, rdtsc, mmio, and other IOIO instructions, but they fail since those cases aren't handled in the svsm handler. I could add these as #[should_panic] if we think they might be useful later.
• I did not test these with qemu since it doesn't work on hardware I have. The IOIO tests rely on a "testdev" device that echos characters sent to a specific port. I'm pretty sure qemu has a compatible device, but it might need to be explicitly enabled.

@p4zuu please take a look.

[joergroedel]

One of the new tests triggers an assertion:

[SVSM] test sev::ghcb::test::test_port_io_16 ...
[SVSM] ERROR: Panic: CPU[0] panicked at kernel/src/sev/ghcb.rs:664:9:
assertion `left == right` failed
  left: 17185
 right: 65535

Is that a known issue?

[p4zuu]

• Is the raw-cpuid dependency OK, or would it be better to just use inline asm or something else?

If it's not too much work, I would actually prefer inlining asm here, just to reduce the supply-chain attack surface. Since this is for testing only, I think we only need a minimal implementation.

• What's a good location to put these? I chose ghcb.rs, but they might make more sense somewhere else.

I am unsure too. Actually the CPUID handling of the current #VC handler is not touching the GHCB since it directly reads into the registered CPUID table (instead of calling the HV), so that might be an argument to move this somewhere else. Moreover, the purpose of this is to verify the #VC handler's behaviors, so I would simply put it into kernel/src/cpu/vc.rs.

• I have tests for other #VC handler cases such as ones for msrs, dr7, wbinvd, rdtsc, mmio, and other IOIO instructions, but they fail since those cases aren't handled in the svsm handler. I could add these as #[should_panic] if we think they might be useful later.

Yes, even if we currently don't support those, I agree with adding them as #[should_panic].

• I did not test these with qemu since it doesn't work on hardware I have. The IOIO tests rely on a "testdev" device that echos characters sent to a specific port. I'm pretty sure qemu has a compatible device, but it might need to be explicitly enabled.

I can try taking a look at this since I'm using qemu :)

[00xc]

• Is the raw-cpuid dependency OK, or would it be better to just use inline asm or something else?

If it's not too much work, I would actually prefer inlining asm here, just to reduce the supply-chain attack surface. Since this is for testing only, I think we only need a minimal implementation.

Rust provides a CPUID function for convenience: https://doc.rust-lang.org/beta/core/arch/x86_64/fn.__cpuid.html

• What's a good location to put these? I chose ghcb.rs, but they might make more sense somewhere else.

I am unsure too. Actually the CPUID handling of the current #VC handler is not touching the GHCB since it directly reads into the registered CPUID table (instead of calling the HV), so that might be an argument to move this somewhere else. Moreover, the purpose of this is to verify the #VC handler's behaviors, so I would simply put it into kernel/src/cpu/vc.rs.

I agree with this.

• I have tests for other #VC handler cases such as ones for msrs, dr7, wbinvd, rdtsc, mmio, and other IOIO instructions, but they fail since those cases aren't handled in the svsm handler. I could add these as #[should_panic] if we think they might be useful later.

Yes, even if we currently don't support those, I agree with adding them as #[should_panic].

Perhaps something like #[ignore = "not yet supported"] would be better. #[must_panic] is not really supported inside the SVSM and these tests won't run outside.

[AdamCDunlap]

One of the new tests triggers an assertion:

This doesn't happen with the google hypervisor, so I'm assuming it's an issue with the qemu testdev device. I'll try to figure out if it intentionally doesn't support 16-bit echos, but if so then I may just need to disable this test or modify qemu. Since I don't have a way of testing with qemu, it'd be nice to get some help here.

If it's not too much work, I would actually prefer inlining asm here, just to reduce the supply-chain attack surface.

Done

I would simply put it into kernel/src/cpu/vc.rs.

Done

Yes, even if we currently don't support those, I agree with adding them as #[should_panic].

Added a bunch more tests. Unfortunately, these tests are untested, but should be a decent starting point once support for those operations are added in the #VC handler.

[roy-hopkins]

One of the new tests triggers an assertion:

This doesn't happen with the google hypervisor, so I'm assuming it's an issue with the qemu testdev device. I'll try to figure out if it intentionally doesn't support 16-bit echos, but if so then I may just need to disable this test or modify qemu. Since I don't have a way of testing with qemu, it'd be nice to get some help here.

I've investigated this using QEMU. You need to add the pc-testdev device to the QEMU configuration for the test port to be active. Applying this patch to your branch fixes the tests for me:

diff --git a/scripts/launch_guest.sh b/scripts/launch_guest.sh
index b638303..9ebab2f 100755
--- a/scripts/launch_guest.sh
+++ b/scripts/launch_guest.sh
@@ -14,6 +14,7 @@ SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
 C_BIT_POS=`$SCRIPT_DIR/../utils/cbit`
 DEBUG_SERIAL=""
 QEMU_EXIT_DEVICE=""
+QEMU_TEST_IO_DEVICE=""

 while [[ $# -gt 0 ]]; do
   case $1 in
@@ -38,6 +39,7 @@ while [[ $# -gt 0 ]]; do
       ;;
     --unit-tests)
       QEMU_EXIT_DEVICE="-device isa-debug-exit,iobase=0xf4,iosize=0x04"
+      QEMU_TEST_IO_DEVICE="-device pc-testdev"
       shift
       ;;
     -*|--*)
@@ -110,6 +112,7 @@ $SUDO_CMD \
     -monitor none \
     -serial stdio \
     $DEBUG_SERIAL \
-    $QEMU_EXIT_DEVICE
+    $QEMU_EXIT_DEVICE \
+    $QEMU_TEST_IO_DEVICE

 stty intr ^C

[AdamCDunlap]

You need to add the pc-testdev device to the QEMU configuration for the test port to be active.

Thanks! I applied that commit to this PR

We already have read_msr() / write_msr() in kernel/src/cpu/msr.rs, could you reuse those?

Whoops, didn't notice them. Changed to use those.

I'd favor just using slice::from_raw_parts(), which is also less convoluted in my opinion. Also, you can test that at least one byte changed more easily with Iterator::any():

Thanks, I applied that change.

[p4zuu]

LGTM too, thanks Adam!