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

[Michael Roth <michael.roth@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