On Mon, Oct 11, 2021 at 08:15:37PM -0500, Michael Roth wrote: > On Sun, Oct 10, 2021 at 08:17:00PM -0700, Marc Orr wrote: > > On Wed, Oct 6, 2021 at 1:40 PM Michael Roth <michael.roth@xxxxxxx> wrote: > > > > > > Add interfaces to allow tests to create/manage SEV guests. The > > > additional state associated with these guests is encapsulated in a new > > > struct sev_vm, which is a light wrapper around struct kvm_vm. These > > > VMs will use vm_set_memory_encryption() and vm_get_encrypted_phy_pages() > > > under the covers to configure and sync up with the core kvm_util > > > library on what should/shouldn't be treated as encrypted memory. > > > > > > Signed-off-by: Michael Roth <michael.roth@xxxxxxx> > > > --- > > > tools/testing/selftests/kvm/Makefile | 1 + > > > .../selftests/kvm/include/x86_64/sev.h | 62 ++++ > > > tools/testing/selftests/kvm/lib/x86_64/sev.c | 303 ++++++++++++++++++ > > > 3 files changed, 366 insertions(+) > > > create mode 100644 tools/testing/selftests/kvm/include/x86_64/sev.h > > > create mode 100644 tools/testing/selftests/kvm/lib/x86_64/sev.c > > > > > > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile > > > index 5832f510a16c..c7a5e1c69e0c 100644 > > > --- a/tools/testing/selftests/kvm/Makefile > > > +++ b/tools/testing/selftests/kvm/Makefile > > > @@ -35,6 +35,7 @@ endif > > > > > > LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/rbtree.c lib/sparsebit.c lib/test_util.c lib/guest_modes.c lib/perf_test_util.c > > > LIBKVM_x86_64 = lib/x86_64/apic.c lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S > > > +LIBKVM_x86_64 += lib/x86_64/sev.c > > > > Regarding RFC-level feedback: First off, I'm super jazzed with what > > I'm seeing so far! (While this is my first review, I've been studying > > the patches up through the SEV boot test, i.e., patch #7). One thing > > I'm wondering is: the way this is structured is to essentially split > > the test cases into non-SEV and SEV. I'm wondering how hard it would > > be to add some flag or environment variable to set up pre-existing > > tests to run under SEV. Or is this something you all thought about, > > and decided that it does not make sense? > > > > Looking at how the guest memory is handled, it seems like it's not far > > off from handling SEV transparently across all test cases. I'd think > > that we could just default all memory to use the encryption bit, and > > then have test cases, such as the test case in patch #7, clear the > > encryption bit for shared pages. However, I think the VM creation > > would need a bit more refactoring to work with other test cases. > > I think it's possible, but there's a few missing pieces: > > 1) As you indicated, existing tests which rely on vm_create(), > vm_create_default(), vm_create_default_with_vcpus(), etc. would either > need to be updated with whatever new interface provides this 'use-sev' > flag, or it would need to happen underneath the covers based on said > environment variable/global/etc. There's also the question of where > to hook in the sev_vm_launch_start() hooks. Maybe the first time a > vcpu_run() is issued? Or maybe some explict call each test will need > to be updated to call just prior to initial execution. > > 2) Many of the existing tests use the GUESY_SYNC/ucall stuff to handle > synchronization between host userspace and guest kernel, which relies on > guests issuing PIO instructions to particular port addresses to cause an > exit back to host userspace, with various parameters passed via register > arguments. > > - For SEV this would almost work as-is, but some tests might rely on > things like memory addresses being passed in this way so would need > to audit the code and mark that memory as shared where needed. > > - For SEV-ES/SEV-SNP, there's a bit more work since: > > - The registers will not be accessible through the existing > KVM_GET_REGS mechanism. It may be possible to set some flag/hook to > set/access arguments through some other mechanism like a shared > buffer for certain VM types though. > > - Additionally, the #VC handler only supports CPUID currently, and > leverages that fact to avoid doing any significant instruction > decoding. Instead the SEV tests use HLT instructions to handle exits > to host userspace, which may not work for some tests. So unless > there's some other mechanism that SEV/non-SEV tests could utilize > rather that PIO, the #VC handler would need to support PIO, which > would be nice to have either way, but would likely involve > pulling in the intruction decoder library used in the kernel, or > some subset/re-implementation of it at least. > > 3) Similar to SEV-ES/SEV-SNP requirements for 1), tests which generate > PIO/MMIO and other NAE events would need appropriate support for those > events in the #VC handler. Nice-to-have either way, but not sure atm > how much it would be to implement all of that. Also any tests relying > on things like KVM_GET_REGS/KVM_GET_SREGS are non-starters. One more I should mention: 4) After encryption, the page table is no longer usable for translations by stuff like addr_gva2gpa(), so tests would either need to be audited/updated to do these translations upfront and only rely on cached/stored values thereafter, or perhaps a "shadow" copy could be maintained by kvm_util so the translations will continue to work after encryption.
