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

From: Kai Huang <kai.huang@intel.com>
To: "Kirill A. Shutemov" <kirill@shutemov.name>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
	Dave Hansen <dave.hansen@intel.com>,
	Sathyanarayanan Kuppuswamy 
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	x86@kernel.org, "H . Peter Anvin" <hpa@zytor.com>,
	Tony Luck <tony.luck@intel.com>, Andi Kleen <ak@linux.intel.com>,
	Wander Lairson Costa <wander@redhat.com>,
	Isaku Yamahata <isaku.yamahata@gmail.com>,
	marcelo.cerri@canonical.com, tim.gardner@canonical.com,
	khalid.elmously@canonical.com, philip.cox@canonical.com,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 3/3] x86/tdx: Add Quote generation support
Date: Tue, 10 May 2022 11:54:12 +1200	[thread overview]
Message-ID: <75d4755c9a376df2e98a267e10e60da3bd178b17.camel@intel.com> (raw)
In-Reply-To: <20220509120927.7rg6v5pyc3f4pxsh@box.shutemov.name>

On Mon, 2022-05-09 at 15:09 +0300, Kirill A. Shutemov wrote:
> On Mon, May 09, 2022 at 03:37:22PM +1200, Kai Huang wrote:
> > 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.
> 
> Whether attestation driver uses struct device is implementation detail.
> I don't see what is you point.

No real DMA is involved in attestation.

> 
> > 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?
> 
> For completeness, cover free path too. Are you going to opencode page
> accept too?

Call __tdx_module_call(TDX_ACCEPT_PAGE, ...) right after MapGPA() to convert
back to private.  I don't think there is any problem?

> 
> Private->Shared conversion is destructive. You have to split SEPT, flush
> TLB. Backward conversion even more costly.

I think I won't call it destructive.

And I suggested before, we can allocate a default size buffer (i.e. 4 pages),
which is large enough to cover all requests for now, during driver
initialization.  This avoids IOCTL time conversion.  We should still have code
in the IOCTL to check the request buffer size and when it is larger than the
default, the old should be freed a larger one should be allocated.  But for now
this code path will never happen.

Btw above is based on assumption that we don't support concurrent IOCTLs.  This
version Sathya somehow changed to support concurrent IOCTLs but this was a
surprise as I thought we somehow agreed we don't need to support this.

Anyway, now I don't have strong opinion here.  To me alloc_pages() +
set_memory_decrypted() is also fine (seems AMD is using this anyway).   Will let
Dave to decide.

-- 
Thanks,
-Kai