linux-kselftest.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Michael Roth <michael.roth@amd.com>
To: Mingwei Zhang <mizhang@google.com>
Cc: <linux-kselftest@vger.kernel.org>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <x86@kernel.org>,
	Nathan Tempelman <natet@google.com>,
	Marc Orr <marcorr@google.com>,
	Steve Rutherford <srutherford@google.com>,
	Sean Christopherson <seanjc@google.com>,
	"Brijesh Singh" <brijesh.singh@amd.com>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	"Varad Gautam" <varad.gautam@suse.com>,
	Shuah Khan <shuah@kernel.org>,
	"Vitaly Kuznetsov" <vkuznets@redhat.com>,
	David Woodhouse <dwmw@amazon.co.uk>,
	"Ricardo Koller" <ricarkol@google.com>,
	Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	"H . Peter Anvin" <hpa@zytor.com>,
	Krish Sadhukhan <krish.sadhukhan@oracle.com>,
	Peter Gonda <pgonda@google.com>
Subject: Re: [PATCH v2 08/13] KVM: selftests: add SEV boot tests
Date: Tue, 21 Dec 2021 09:40:36 -0600	[thread overview]
Message-ID: <20211221154036.ivef7wuoblavlmsf@amd.com> (raw)
In-Reply-To: <Yb/hGzeiRi0AwfV6@google.com>

On Mon, Dec 20, 2021 at 01:49:15AM +0000, Mingwei Zhang wrote:
> On Thu, Dec 16, 2021, Michael Roth wrote:
> > A common aspect of booting SEV guests is checking related CPUID/MSR
> > bits and accessing shared/private memory. Add a basic test to cover
> > this.
> >
> > This test will be expanded to cover basic boot of SEV-ES and SEV-SNP in
> > subsequent patches.
> >
> > Signed-off-by: Michael Roth <michael.roth@amd.com>
> > ---
> >  tools/testing/selftests/kvm/.gitignore        |   1 +
> >  tools/testing/selftests/kvm/Makefile          |   1 +
> >  .../selftests/kvm/x86_64/sev_all_boot_test.c  | 255 ++++++++++++++++++
> >  3 files changed, 257 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 4a801cba9c62..cc73de938a2a 100644
> > --- a/tools/testing/selftests/kvm/.gitignore
> > +++ b/tools/testing/selftests/kvm/.gitignore
> > @@ -43,6 +43,7 @@
> >  /x86_64/xen_vmcall_test
> >  /x86_64/xss_msr_test
> >  /x86_64/vmx_pmu_msrs_test
> > +/x86_64/sev_all_boot_test
> >  /access_tracking_perf_test
> >  /demand_paging_test
> >  /dirty_log_test
> > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> > index ccc382a827f1..6f250e190fde 100644
> > --- a/tools/testing/selftests/kvm/Makefile
> > +++ b/tools/testing/selftests/kvm/Makefile
> > @@ -81,6 +81,7 @@ 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/vmx_pi_mmio_test
> >  TEST_GEN_PROGS_x86_64 += x86_64/sev_migrate_tests
> > +TEST_GEN_PROGS_x86_64 += x86_64/sev_all_boot_test
> >  TEST_GEN_PROGS_x86_64 += demand_paging_test
> >  TEST_GEN_PROGS_x86_64 += dirty_log_test
> >  TEST_GEN_PROGS_x86_64 += dirty_log_perf_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..329a740a7cb2
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c
> > @@ -0,0 +1,255 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Basic SEV boot tests.
> > + *
> > + * Copyright (C) 2021 Advanced Micro Devices
> > + */
> > +#define _GNU_SOURCE /* for program_invocation_short_name */
> > +#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 VCPU_ID			2
> > +#define PAGE_SIZE		4096
> > +#define PAGE_STRIDE		32
> > +
> > +#define SHARED_PAGES		8192
> > +#define SHARED_VADDR_MIN	0x1000000
> > +
> > +#define PRIVATE_PAGES		2048
> > +#define PRIVATE_VADDR_MIN	(SHARED_VADDR_MIN + SHARED_PAGES * PAGE_SIZE)
> > +
> > +#define TOTAL_PAGES		(512 + SHARED_PAGES + PRIVATE_PAGES)
> > +
> > +static void fill_buf(uint8_t *buf, size_t pages, size_t stride, uint8_t val)
> > +{
> > +	int i, j;
> > +
> > +	for (i = 0; i < pages; i++)
> > +		for (j = 0; j < PAGE_SIZE; j += stride)
> > +			buf[i * PAGE_SIZE + j] = val;
> > +}
> > +
> > +static bool check_buf(uint8_t *buf, size_t pages, size_t stride, uint8_t val)
> > +{
> > +	int i, j;
> > +
> > +	for (i = 0; i < pages; i++)
> > +		for (j = 0; j < PAGE_SIZE; j += stride)
> > +			if (buf[i * PAGE_SIZE + j] != val)
> > +				return false;
> > +
> > +	return true;
> > +}
> > +
> > +static void guest_test_start(struct ucall *uc)
> > +{
> > +	/* Initial guest check-in. */
> > +	GUEST_SHARED_SYNC(uc, 1);
> > +}
> > +
> > +static void test_start(struct kvm_vm *vm, struct ucall *uc)
> > +{
> > +	vcpu_run(vm, VCPU_ID);
> > +
> > +	/* Initial guest check-in. */
> > +	CHECK_SHARED_SYNC(vm, VCPU_ID, uc, 1);
> > +}
> > +
> > +static void
> > +guest_test_common(struct ucall *uc, uint8_t *shared_buf, uint8_t *private_buf)
> > +{
> > +	bool success;
> > +
> > +	/* Initial check-in for common. */
> > +	GUEST_SHARED_SYNC(uc, 100);
> 
> Probably, you want to use macros to represent those states which should
> make it more clear. Otherwise, it is quite cumbersome for readers to
> remember the meaning (or state) of 100/101/102.

I agree with that approach in general, but in this case it's a bit
awkward since, these aren't necessarily similarly grouped tests where each
test falls under a certain category of TEST_GROUP_x where a macro would
provide additional information about the nature of the tests related to a
particular SYNC() call, in some cases it's an aggregate of multiple
things, for instance...


> > +
> > +	/* Ensure initial shared pages are intact. */
> > +	success = check_buf(shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x41);
> > +	GUEST_SHARED_ASSERT(uc, success);
> > +
> > +	/* Ensure initial private pages are intact/encrypted. */
> > +	success = check_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x42);
> > +	GUEST_SHARED_ASSERT(uc, success);
> > +
> > +	/* Ensure host userspace can't read newly-written encrypted data. */
> > +	fill_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x43);
> > +
> > +	GUEST_SHARED_SYNC(uc, 101);
> 
> ditto.

...here 101 exit is both a checkpoint that the 2 previous tests have passed,
and that a 3rd test is ready for the host-side to complete on its end.

> > +
> > +	/* Ensure guest can read newly-written shared data from host. */
> > +	success = check_buf(shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x44);
> > +	GUEST_SHARED_ASSERT(uc, success);
> > +
> > +	/* Ensure host can read newly-written shared data from guest. */
> > +	fill_buf(shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x45);
> > +
> > +	GUEST_SHARED_SYNC(uc, 102);
> 
> ditto.

...here 102 exit is for the "Ensure host can read newly-written shared
data" test, since the host is needed to complete the test. But prior to
that there was another test checking things in the other direction:
"Ensure guest can read newly-written shared data from host."

I could try adding additional SYNC() calls to get things closer to a
1:1 mapping between test/SYNC() so we can give each particular
test/SYNC() point a unique/meaningful identifier, but that's potentially
a lot of additional exits/SYNC()s depending on how tests are added over
time, and it's compounded further by the following situation, where a
conversion of this file would leave us with something like:

  guest:
    //set up some tests relating to x for host to complete
    SYNC(..., TEST_GROUP_x_READY)
  
    //complete tests relating to y
    SYNC(..., TEST_GROUP_y_COMPLETE)
  
  host:
    vmrun()
    CHECK(..., TEST_GROUP_x_READY)
    //complete tests relating to x
  
    //set up some tests relating to y for guest to complete
    vmrun()

    CHECK(..., TEST_GROUP_y_COMPLETE)

where it's easy for a reader to get confused as to where
TEST_GROUP_x_COMPLETE is, whether it's related to the subsequent
*_y_COMPLETE, etc, because they might be expecting the same sort of pairing
we get for guest->host with SYNC()/CHECK(), but for host->guest the pairing
is more like vmrun()/SYNC()/CHECK(), so attempting to write things to
alleviate that possible confusion, we need to introduce exits that purely
serve the purpose of helping the reader along, and end up with something
like:

  guest:
    //set up some tests relating to x for host to complete
    SYNC(..., TEST_GROUP_x_READY)

    SYNC(..., TEST_GROUP_x_COMPLETE)
  
    //complete tests relating to y
    SYNC(..., TEST_GROUP_y_COMPLETE)
  
  host:
    vmrun()
    CHECK(..., TEST_GROUP_x_READY)
    //complete tests relating to x

    //resume guest to it lets us know it is done with that test group
    vmrun()
    CHECK(..., TEST_GROUP_x_COMPLETE)

    //set up some tests relating to y for guest to complete
    vmrun()
    
    CHECK(..., TEST_GROUP_y_COMPLETE)

But what about TEST_GROUP_y_READY? Where is that set up? Take 2:

  guest:
    //set up some tests relating to x for host to complete
    SYNC(..., TEST_GROUP_x_READY)

    SYNC(..., TEST_GROUP_x_COMPLETE)

    //let host know we're ready to run TEST_GROUP_y
    SYNC(..., TEST_GROUP_y_READY)
  
    //complete tests relating to y
    SYNC(..., TEST_GROUP_y_COMPLETE)
  
  host:
    vmrun()
    CHECK(..., TEST_GROUP_x_READY)
    //complete tests relating to x

    //resume guest to it lets us know it is done with that test group
    vmrun()
    CHECK(..., TEST_GROUP_x_COMPLETE)

    //set up some tests relating to y for guest to complete
    vmrun()
    CHECK(..., TEST_GROUP_y_READY)
    vmrun()
    CHECK(..., TEST_GROUP_y_COMPLETE)

...which to me seems less readable that just treating each SYNC() as a
unique checkpoint and relying on comments to explain the individual
tests: 

  guest:
    //set up some tests relating to x for host to complete

    SYNC(..., CHECKPOINT1)
  
    //complete tests relating to y

    SYNC(..., CHECKPOINT2)
  
  host:
    vmrun()
    CHECK(..., CHECKPOINT1)

    //complete tests relating to x
  
    //set up some tests relating to y for guest to complete
    vmrun()

    CHECK(..., CHECKPOINT2)

Really don't mind changing things, but just doesn't seem like it would
make things more readable than the current approach. I could add
actual CHECKPOINT1/2/3 macros to make it clearer how SYNC() is being
used here, but it seems like a waste adding a bunch of '#define
CHECKPOINT1 1' etc.

> > +}
> > +
> > +static void
> > +test_common(struct kvm_vm *vm, struct ucall *uc,
> > +		  uint8_t *shared_buf, uint8_t *private_buf)
> > +{
> > +	bool success;
> > +
> > +	/* Initial guest check-in. */
> > +	vcpu_run(vm, VCPU_ID);
> > +	CHECK_SHARED_SYNC(vm, VCPU_ID, uc, 100);
> > +
> > +	/* Ensure initial private pages are intact/encrypted. */
> > +	success = check_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x42);
> > +	TEST_ASSERT(!success, "Initial guest memory not encrypted!");
> > +
> > +	vcpu_run(vm, VCPU_ID);
> > +	CHECK_SHARED_SYNC(vm, VCPU_ID, uc, 101);
> > +
> > +	/* Ensure host userspace can't read newly-written encrypted data. */
> > +	success = check_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x43);
> 
> I am not sure if it is safe here. Since the cache coherency is not there
> for neither SEV or SEV-ES. Reading confidential memory from host side
> will generate cache lines that is not coherent with the guest. So might
> be better to add clfush here?

On the guest side, the cachelines are tagged based on ASID, so in this case
the guest would populate it's own cachelines when it writes new data to
private buf.

On a host without SME coherency bit, there is a possibility that whatever
data the host had previously written to private_buf with C=0/ASID=0, prior
to the guest writing to it, might still be present in the cache, but for
this test that's okay since the guest has purposely written new data to
confirm that the host does not see the new data. What data the host
*actually* sees, stale cache data vs. new reads of guest private memory
with C=0 (e.g. ciphertext) are both okay as far as the test is concerned.
clflush() would probably make sense here, but if failure to do so
somehow results in the above assumptions not holding, and the test ends
up seeing the newly-written data, we definitely want this test to fail
loudly, so leaving out the clflush() to cover that corner case seems
like a good idea.

On a host with SME coherency bit, I believe (based on a recent writeup from
David Kaplan) each new read/write with a different C-bit/ASID than what was
previously written would force an eviction, so we don't need to worry about
the stale cases above, and the test should work in that scenario as
well.

Thanks!

-Mike

  reply	other threads:[~2021-12-21 15:40 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-16 17:13 [PATCH v2 00/13] KVM: selftests: Add tests for SEV and SEV-ES guests Michael Roth
2021-12-16 17:13 ` [PATCH v2 01/13] KVM: selftests: move vm_phy_pages_alloc() earlier in file Michael Roth
2021-12-16 17:13 ` [PATCH v2 02/13] KVM: selftests: sparsebit: add const where appropriate Michael Roth
2021-12-16 17:13 ` [PATCH v2 03/13] KVM: selftests: add hooks for managing encrypted guest memory Michael Roth
2021-12-16 17:13 ` [PATCH v2 04/13] KVM: selftests: handle encryption bits in page tables Michael Roth
2021-12-16 17:13 ` [PATCH v2 05/13] KVM: selftests: add support for encrypted vm_vaddr_* allocations Michael Roth
2021-12-16 17:13 ` [PATCH v2 06/13] KVM: selftests: ensure ucall_shared_alloc() allocates shared memory Michael Roth
2021-12-16 17:13 ` [PATCH v2 07/13] KVM: selftests: add library for creating/interacting with SEV guests Michael Roth
2021-12-16 20:35   ` Peter Gonda
2021-12-17 16:17     ` Michael Roth
2021-12-22 14:52       ` Paolo Bonzini
2021-12-16 17:13 ` [PATCH v2 08/13] KVM: selftests: add SEV boot tests Michael Roth
2021-12-20  1:49   ` Mingwei Zhang
2021-12-21 15:40     ` Michael Roth [this message]
2021-12-21 17:26       ` Michael Roth
2021-12-22 14:55       ` Paolo Bonzini
2021-12-16 17:13 ` [PATCH v2 09/13] KVM: SVM: include CR3 in initial VMSA state for SEV-ES guests Michael Roth
2021-12-22 14:25   ` Paolo Bonzini
2021-12-16 17:13 ` [PATCH v2 10/13] KVM: selftests: account for error code in #VC exception frame Michael Roth
2021-12-16 17:13 ` [PATCH v2 11/13] KVM: selftests: add support for creating SEV-ES guests Michael Roth
2021-12-16 17:13 ` [PATCH v2 12/13] KVM: selftests: add library for handling SEV-ES-related exits Michael Roth
2021-12-16 17:13 ` [PATCH v2 13/13] KVM: selftests: add SEV-ES boot tests Michael Roth
2021-12-22 14:56 ` [PATCH v2 00/13] KVM: selftests: Add tests for SEV and SEV-ES guests Paolo Bonzini
2022-01-04 23:41   ` Michael Roth

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211221154036.ivef7wuoblavlmsf@amd.com \
    --to=michael.roth@amd.com \
    --cc=bp@alien8.de \
    --cc=brijesh.singh@amd.com \
    --cc=dwmw@amazon.co.uk \
    --cc=hpa@zytor.com \
    --cc=jmattson@google.com \
    --cc=joro@8bytes.org \
    --cc=krish.sadhukhan@oracle.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=marcorr@google.com \
    --cc=mingo@redhat.com \
    --cc=mizhang@google.com \
    --cc=natet@google.com \
    --cc=pgonda@google.com \
    --cc=ricarkol@google.com \
    --cc=seanjc@google.com \
    --cc=shuah@kernel.org \
    --cc=srutherford@google.com \
    --cc=tglx@linutronix.de \
    --cc=thomas.lendacky@amd.com \
    --cc=varad.gautam@suse.com \
    --cc=vkuznets@redhat.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).