Re: [PATCH v2 08/13] KVM: selftests: add SEV boot tests

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 11:26:04 -0600	[thread overview]
Message-ID: <20211221172604.7hg4digpwp2mnidv@amd.com> (raw)
In-Reply-To: <20211221154036.ivef7wuoblavlmsf@amd.com>

On Tue, Dec 21, 2021 at 09:40:36AM -0600, Michael Roth wrote:
> On Mon, Dec 20, 2021 at 01:49:15AM +0000, Mingwei Zhang wrote:
> > On Thu, Dec 16, 2021, Michael Roth wrote:
> > > +}
> > > +
> > > +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.

Actually it might be good to check both of those cases, e.g.:

  //check private buf (possibly with stale cache for sme_coherency=0)
  clflush()
  //check private buf again (with fresh read of guest memory)

I'll take a look at that.

-Mike