Re: [RFC 06/16] KVM: selftests: add library for creating/interacting with SEV guests

[Michael Roth <michael.roth@amd.com>]

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