Re: [PATCH v5 3/3] x86/tdx: Add Quote generation support

[Kai Huang]

On Sat, 2022-05-07 at 03:42 +0300, Kirill A. Shutemov wrote:
> On Fri, May 06, 2022 at 12:11:03PM +1200, Kai Huang wrote:
> > Kirill, what's your opinion?
> 
> I said before that I think DMA API is the right tool here.
> 
> Speculation about future of DMA in TDX is irrelevant here. If semantics
> change we will need to re-evaluate all users. VirtIO uses DMA API and it
> is conceptually the same use-case: communicate with the host.

Virtio is designed for device driver to use, so it's fine to use DMA API. And
real DMA can happen to the virtio DMA buffers.  Attestation doesn't have such
assumption.

DMA API has it's limitations.  I don't see the protocol to convert GPA from
private to shared is so complicated (see below), and I think regardless this
attestation use case, it's perhaps worth to provide an additional simple
infrastructure for such case so it can be used when needed.

> 
> But vmap() + set_memory_decrypted() also works and Sathya already has code
> for it. I'm fine with this.

Dave said (again) he wanted to avoid breaking up direct mapping.

https://lore.kernel.org/lkml/5d34ac93-09dc-ea93-bffe-f3995647cd5b@xxxxxxxxxxxxxxx/T/#m37778b8af5d72c3db79e3cfa4b46ee37836f771c

So we need to use seither use DMA API (which already breaks direct-mapping for
swiotlb), or we use vmap() + MapGPA() as I mentioned below.

> 
> Going a step below to manual MapGPA() is just wrong. We introduced
> abstructions for a reason. Protocol of changing GPA status is not trivial.
> We should not spread it across all kernel codebase.
> 

I kinda disagree with this.  In fact, the protocol of changing GPA status isn't
that complicated.  TD guest can have both private and shared mappings to the
same GPA simultaneously.  We don't need to change all the mappings when
converting private page to shared.

For instance, we can use vmap() to have a shared mapping to a page, while the
page is still mapped as private in direct-mapping.  TD uses MapGPA() to tell VMM
which mapping it wants to use, and it just needs to guarantee that the private
(direct) mapping won't be used.  Speculative fetch using the direct mapping is
fine, as long as the core won't consume the data.  The only thing we need to
guarantee is we need to flush any dirty cachelines before MapGPA(). My
understanding is we don't even need to flush the TLB of the private mapping.

And reading the GHCI MapGPA() again, to me MapGPA() itself requires VMM to
guarantee the TLB and cache flush:

"
If the GPA (range) was already mapped as an active, private page, the host VMM
may remove the private page from the TD by following the “Removing TD Private
Pages” sequence in the Intel TDX-module specification [3] to safely block the
mapping(s), flush the TLB and cache, and remove the mapping(s). The VMM is
designed to be able to then map the specified GPA (range) in the shared-EPT
structure and allow the TD to access the page(s) as a shared GPA (range).
"

You can see the cache flush is guaranteed by VMM.

Btw, the use of word "may" in "host VMM may remove..." in above paragraph is
horrible.  It should use "must", just like to the "converting shared to private"
case:

"
If the Start GPA specified is a private GPA (GPA.S bit is clear), this MapGPA
TDG.VP.VMCALL can be used to help request the host VMM map the specific, private
page(s) (which mapping may involve converting the backing-physical page from a
shared page to a private page). As intended in this case, the VMM must unmap the
GPA from the shared-EPT region and invalidate the TLB and caches for the TD
vcpus to help ensure no stale mappings and cache contents exist.
"

As you can see "must" is used in "the VMM must unmap the GPA from the shared-EPT
...".

So I don't see why TD guest kernel cannot have a simple protocol to vmap() a
page (or couple of pages) as shared on-demand, like below:

	page = alloc_page();

	addr = vmap(page,  pgprot_decrypted(PAGE_KERNEL));

	clflush_cache_range(page_address(page), PAGE_SIZE);

	MapGPA(page_to_phys(page) | cc_mkdec(0), PAGE_SIZE);

And we can even avoid above clflush_cache_range() if I understand correctly.

Or  I missed something?


-- 
Thanks,
-Kai