Re: [RFC PATCH 00/10] KVM: selftests: Add support for test-selectable ucall implementations

[Paolo Bonzini <pbonzini@redhat.com>]

On 12/10/21 17:46, Michael Roth wrote:
> These patches are based on kvm/next, and are also available at:
> 
>    https://github.com/mdroth/linux/commits/sev-selftests-ucall-rfc1

Thanks, this is a nice implementation of the concept.  I'll check s390 
before the next merge window, as I intend to merge this and the SEV 
tests (which I have already confirmed to work; I haven't yet checked 
SEV-ES because it's a bit harder for me to install new kernels on the 
SEV-ES machine I have access to).

Paolo

> == BACKGROUND ==
> 
> These patches are a prerequisite for adding selftest support for SEV guests
> and possibly other confidential computing implementations in the future.
> They were motivated by a suggestion Paolo made in response to the initial
> SEV selftest RFC:
> 
>    https://lore.kernel.org/lkml/20211025035833.yqphcnf5u3lk4zgg@amd.com/T/#m959b56f9fb4ae6ab973f6ab50fe3ddfacd7c5617
> 
> Since the changes touch multiple archs and ended up creating a bit more churn
> than expected, I thought it would be a good idea to carve this out into a
> separate standalone series for reviewers who may be more interested in the
> ucall changes than anything SEV-related.
> 
> To summarize, x86 relies on a ucall based on using PIO intructions to generate
> an exit to userspace and provide the GVA of a dynamically-allocated ucall
> struct that resides in guest memory and contains information about how to
> handle/interpret the exit. This doesn't work for SEV guests for 3 main reasons:
> 
>    1) The guest memory is generally encrypted during run-time, so the guest
>       needs to ensure the ucall struct is allocated in shared memory.
>    2) The guest page table is also encrypted, so the address would need to be a
>       GPA instead of a GVA.
>    3) The guest vCPU register may also be encrypted in the case of
>       SEV-ES/SEV-SNP, so the approach of examining vCPU register state has
>       additional requirements such as requiring guest code to implement a #VC
>       handler that can provide the appropriate registers via a vmgexit.
> 
> To address these issues, the SEV selftest RFC1 patchset introduced a set of new
> SEV-specific interfaces that closely mirrored the functionality of
> ucall()/get_ucall(), but relied on a pre-allocated/static ucall buffer in
> shared guest memory so it that guest code could pass messages/state to the host
> by simply writing to this pre-arranged shared memory region and then generating
> an exit to userspace (via a halt instruction).
> 
> Paolo suggested instead implementing support for test/guest-specific ucall
> implementations that could be used as an alternative to the default PIO-based
> ucall implementations as-needed based on test/guest requirements, while still
> allowing for tests to use a common set interfaces like ucall()/get_ucall().
> 
> == OVERVIEW ==
> 
> This series implements the above functionality by introducing a new ucall_ops
> struct that can be used to register a particular ucall implementation as need,
> then re-implements x86/arm64/s390x in terms of the ucall_ops.
> 
> But for the purposes of introducing a new ucall_ops implementation appropriate
> for SEV, there are a couple issues that resulted in the need for some additional
> ucall interfaces as well:
> 
>    a) ucall() doesn't take a pointer to the ucall struct it modifies, so to make
>       it work in the case of an implementation that relies a pre-allocated ucall
>       struct in shared guest memory some sort of global lookup functionality
>       would be needed to locate the appropriate ucall struct for a particular
>       VM/vcpu combination, and this would need to be made accessible for use by
>       the guest as well. guests would then need some way of determining what
>       VM/vcpu identifiers they need to use to do the lookup, which to do reliably
>       would likely require seeding the guest with those identifiers in advance,
>       which is possible, but much more easily achievable by simply adding a
>       ucall() alternative that accepts a pointer to the ucall struct for that
>       particular VM/vcpu.
> 
>    b) get_ucall() *does* take a pointer to a ucall struct, but currently zeroes
>       it out and uses it to copy the guest's ucall struct into. It *could* be
>       re-purposed to handle the case where the pointer is an actual pointer to
>       the ucall struct in shared guest memory, but that could cause problems
>       since callers would need some idea of what the underlying ucall
>       implementation expects. Ideally the interfaces would be agnostic to the
>       ucall implementation.
> 
> So to address those issues, this series also allows ucall implementations to
> optionally be extended to support a set of 'shared' ops that are used in the
> following manner:
> 
>    host:
>      uc_gva = ucall_shared_alloc()
>      setup_vm_args(vm, uc_gva)
> 
>    guest:
>      ucall_shared(uc_gva, ...)
> 
>    host:
>      uget_ucall_shared(uc_gva, ...)
> 
> and then implements a new ucall implementation, ucall_ops_halt, based around
> these shared interfaces and halt instructions.
> 
> While this doesn't really meet the initial goal of re-using the existing
> ucall interfaces as-is, the hope is that these *_shared interfaces are
> general enough to be re-usable things other than SEV, or at least improve on
> code readability over the initial SEV-specific interfaces.
> 
> Any review/comments are greatly appreciated!
> 
> ----------------------------------------------------------------
> Michael Roth (10):
>        kvm: selftests: move base kvm_util.h declarations to kvm_util_base.h
>        kvm: selftests: move ucall declarations into ucall_common.h
>        kvm: selftests: introduce ucall_ops for test/arch-specific ucall implementations
>        kvm: arm64: selftests: use ucall_ops to define default ucall implementation
>        (COMPILE-TESTED ONLY) kvm: s390: selftests: use ucall_ops to define default ucall implementation
>        kvm: selftests: add ucall interfaces based around shared memory
>        kvm: selftests: add ucall_shared ops for PIO
>        kvm: selftests: introduce ucall implementation based on halt instructions
>        kvm: selftests: add GUEST_SHARED_* macros for shared ucall implementations
>        kvm: selftests: add ucall_test to test various ucall functionality
> 
>   tools/testing/selftests/kvm/.gitignore             |   1 +
>   tools/testing/selftests/kvm/Makefile               |   5 +-
>   .../testing/selftests/kvm/include/aarch64/ucall.h  |  18 +
>   tools/testing/selftests/kvm/include/kvm_util.h     | 408 +--------------------
>   .../testing/selftests/kvm/include/kvm_util_base.h  | 368 +++++++++++++++++++
>   tools/testing/selftests/kvm/include/s390x/ucall.h  |  18 +
>   tools/testing/selftests/kvm/include/ucall_common.h | 147 ++++++++
>   tools/testing/selftests/kvm/include/x86_64/ucall.h |  19 +
>   tools/testing/selftests/kvm/lib/aarch64/ucall.c    |  43 +--
>   tools/testing/selftests/kvm/lib/s390x/ucall.c      |  45 +--
>   tools/testing/selftests/kvm/lib/ucall_common.c     | 133 +++++++
>   tools/testing/selftests/kvm/lib/x86_64/ucall.c     |  82 +++--
>   tools/testing/selftests/kvm/ucall_test.c           | 182 +++++++++
>   13 files changed, 982 insertions(+), 487 deletions(-)
>   create mode 100644 tools/testing/selftests/kvm/include/aarch64/ucall.h
>   create mode 100644 tools/testing/selftests/kvm/include/kvm_util_base.h
>   create mode 100644 tools/testing/selftests/kvm/include/s390x/ucall.h
>   create mode 100644 tools/testing/selftests/kvm/include/ucall_common.h
>   create mode 100644 tools/testing/selftests/kvm/include/x86_64/ucall.h
>   create mode 100644 tools/testing/selftests/kvm/lib/ucall_common.c
>   create mode 100644 tools/testing/selftests/kvm/ucall_test.c
> 
>