Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 24 Mar 2023 11:23:51 -0700
In-Reply-To: <20230110175057.715453-8-pgonda@google.com>
Mime-Version: 1.0
References: <20230110175057.715453-1-pgonda@google.com> <20230110175057.715453-8-pgonda@google.com>
Message-ID: <ZB3qtwN2BtSDgU0Y@google.com>
Subject: Re: [PATCH V6 7/7] KVM: selftests: Add simple sev vm testing
From:   Sean Christopherson <seanjc@google.com>
To:     Peter Gonda <pgonda@google.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        Vishal Annapurve <vannapurve@google.com>,
        Ackerley Tng <ackerleytng@google.com>,
        Andrew Jones <andrew.jones@linux.dev>,
        Michael Roth <michael.roth@amd.com>
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Tue, Jan 10, 2023, Peter Gonda wrote:
> A very simple of booting SEV guests that checks related CPUID bits. This
> is a stripped down version of "[PATCH v2 08/13] KVM: selftests: add SEV
> boot tests" from Michael but much simpler.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Vishal Annapurve <vannapurve@google.com>
> Cc: Ackerley Tng <ackerleytng@google.com>
> cc: Andrew Jones <andrew.jones@linux.dev>
> Suggested-by: Michael Roth <michael.roth@amd.com>
> Signed-off-by: Peter Gonda <pgonda@google.com>
> ---
>  tools/testing/selftests/kvm/.gitignore        | 84 +++++++++++++++++++
>  tools/testing/selftests/kvm/Makefile          |  1 +
>  .../selftests/kvm/x86_64/sev_all_boot_test.c  | 84 +++++++++++++++++++
>  3 files changed, 169 insertions(+)
>  create mode 100644 tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c
> 
> diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
> index 6d9381d60172..6d826957c6ae 100644
> --- a/tools/testing/selftests/kvm/.gitignore
> +++ b/tools/testing/selftests/kvm/.gitignore
> @@ -1,7 +1,91 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> +<<<<<<< HEAD
>  *
>  !/**/
>  !*.c
>  !*.h
>  !*.S
>  !*.sh
> +=======

For the love of all that is holy, please take the time to actually look at what
you're posting.

> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index b7cfb15712d1..66d7ab3da990 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -111,6 +111,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/tsc_msrs_test
>  TEST_GEN_PROGS_x86_64 += x86_64/vmx_pmu_caps_test
>  TEST_GEN_PROGS_x86_64 += x86_64/xen_shinfo_test
>  TEST_GEN_PROGS_x86_64 += x86_64/xen_vmcall_test
> +TEST_GEN_PROGS_x86_64 += x86_64/sev_all_boot_test
>  TEST_GEN_PROGS_x86_64 += x86_64/sev_migrate_tests
>  TEST_GEN_PROGS_x86_64 += x86_64/amx_test
>  TEST_GEN_PROGS_x86_64 += x86_64/max_vcpuid_cap_test
> diff --git a/tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c b/tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c
> new file mode 100644
> index 000000000000..e9e4d7305bc1
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c

"all_boot" is kinda odd.  Maybe just sev_basic_test?

> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Basic SEV boot tests.
> + *
> + */

Meh, I'd leave off the comment, the testname should be pretty self-explanatory.

> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +
> +#include "test_util.h"
> +#include "kvm_util.h"
> +#include "processor.h"
> +#include "svm_util.h"
> +#include "linux/psp-sev.h"
> +#include "sev.h"
> +
> +#define NR_SYNCS 1
> +
> +#define MSR_AMD64_SEV_BIT  1

msr-index.h exists in tools, use the defines from there.

> +
> +static void guest_run_loop(struct kvm_vcpu *vcpu)

Another pointless helper with one caller.  _If_ more subtests come along _and_
need this separate loop, then by all means.  But right now, it's just noise.

> +{
> +	struct ucall uc;
> +	int i;
> +
> +	for (i = 0; i <= NR_SYNCS; ++i) {

There's zero reason to cap the number of loops, and it can lead to false passes,
e.g. if the guest gets stuck on GUEST_SYNC() then this will pass.  Just do

	for (;;) {

> +		vcpu_run(vcpu);
> +		switch (get_ucall(vcpu, &uc)) {
> +		case UCALL_SYNC:
> +			continue;
> +		case UCALL_DONE:
> +			return;
> +		case UCALL_ABORT:
> +			REPORT_GUEST_ASSERT(uc);
> +		default:
> +			TEST_FAIL("Unexpected exit: %s",
> +				  exit_reason_str(vcpu->run->exit_reason));
> +		}
> +	}
> +}
> +
> +static void is_sev_enabled(void)
> +{
> +	uint64_t sev_status;
> +
> +	GUEST_ASSERT(this_cpu_has(X86_FEATURE_SEV));
> +
> +	sev_status = rdmsr(MSR_AMD64_SEV);
> +	GUEST_ASSERT(sev_status & 0x1);
> +}
> +
> +static void guest_sev_code(void)
> +{
> +	GUEST_SYNC(1);
> +
> +	is_sev_enabled();

Again, completely unnecessary helper.  And is_blah_enabled() _strongly_ suggests
a pure function that returns true/false.

All in all, this can be slimmed down to:

// SPDX-License-Identifier: GPL-2.0-only
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/ioctl.h>

#include "test_util.h"
#include "kvm_util.h"
#include "processor.h"
#include "svm_util.h"
#include "linux/psp-sev.h"
#include "sev.h"

static void guest_sev_code(void)
{
	GUEST_ASSERT(this_cpu_has(X86_FEATURE_SEV));
	GUEST_ASSERT(rdmsr(MSR_AMD64_SEV) & MSR_AMD64_SEV_ENABLED);

	GUEST_DONE();
}

static void test_sev(void *guest_code, uint64_t policy)
{
	struct kvm_vcpu *vcpu;
	struct kvm_vm *vm;
	struct ucall uc;
	int i;

	vm = vm_sev_create_with_one_vcpu(policy, guest_code, &vcpu);

	for (;;) {
		vcpu_run(vcpu);

		switch (get_ucall(vcpu, &uc)) {
		case UCALL_SYNC:
			continue;
		case UCALL_DONE:
			return;
		case UCALL_ABORT:
			REPORT_GUEST_ASSERT(uc);
		default:
			TEST_FAIL("Unexpected exit: %s",
				  exit_reason_str(vcpu->run->exit_reason));
		}
	}

	kvm_vm_free(vm);
}

int main(int argc, char *argv[])
{
	TEST_REQUIRE(is_kvm_sev_supported());

	test_sev(guest_sev_code, SEV_POLICY_NO_DBG);
	test_sev(guest_sev_code, 0);

	return 0;
}