Re: [PATCHv7 14/14] x86/tdx: Add unaccepted memory support

From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
To: Borislav Petkov <bp@alien8.de>
Cc: Andy Lutomirski <luto@kernel.org>,
	Sean Christopherson <seanjc@google.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Joerg Roedel <jroedel@suse.de>, Ard Biesheuvel <ardb@kernel.org>,
	Andi Kleen <ak@linux.intel.com>,
	Kuppuswamy Sathyanarayanan
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	David Rientjes <rientjes@google.com>,
	Vlastimil Babka <vbabka@suse.cz>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Peter Zijlstra <peterz@infradead.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Ingo Molnar <mingo@redhat.com>,
	Varad Gautam <varad.gautam@suse.com>,
	Dario Faggioli <dfaggioli@suse.com>,
	Dave Hansen <dave.hansen@intel.com>,
	Mike Rapoport <rppt@kernel.org>,
	David Hildenbrand <david@redhat.com>,
	marcelo.cerri@canonical.com, tim.gardner@canonical.com,
	khalid.elmously@canonical.com, philip.cox@canonical.com,
	x86@kernel.org, linux-mm@kvack.org, linux-coco@lists.linux.dev,
	linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCHv7 14/14] x86/tdx: Add unaccepted memory support
Date: Tue, 9 Aug 2022 14:45:44 +0300	[thread overview]
Message-ID: <20220809114544.q4f7cetqgv2gj5tx@box.shutemov.name> (raw)
In-Reply-To: <Yt//ZMrxtTwtKBqu@zn.tnic>

On Tue, Jul 26, 2022 at 04:51:16PM +0200, Borislav Petkov wrote:
> On Tue, Jun 14, 2022 at 03:02:31PM +0300, Kirill A. Shutemov wrote:
> > +static bool is_tdx_guest(void)
> > +{
> > +	static bool once;
> > +	static bool is_tdx;
> > +
> > +	if (!IS_ENABLED(CONFIG_INTEL_TDX_GUEST))
> > +		return false;
> > +
> > +	if (!once) {
> > +		u32 eax, sig[3];
> > +
> > +		cpuid_count(TDX_CPUID_LEAF_ID, 0, &eax,
> > +			    &sig[0], &sig[2],  &sig[1]);
> > +		is_tdx = !memcmp(TDX_IDENT, sig, sizeof(sig));
> > +		once = true;
> > +	}
> > +
> > +	return is_tdx;
> > +}
> 
> early_tdx_detect() already calls this CPUID function. It assigns
> function pointers too.
> 
> So why can't you assign an accept_memory() function pointer there and
> get rid of this sprinkled if (tdx) everywhere?

This code called from EFI stub which runs before decompressor code and
therefore before early_tdx_detect().

> > diff --git a/arch/x86/boot/compressed/tdx.c b/arch/x86/boot/compressed/tdx.c
> > index 918a7606f53c..8518a75e5dd5 100644
> > --- a/arch/x86/boot/compressed/tdx.c
> > +++ b/arch/x86/boot/compressed/tdx.c
> > @@ -3,12 +3,15 @@
> >  #include "../cpuflags.h"
> >  #include "../string.h"
> >  #include "../io.h"
> > +#include "align.h"
> >  #include "error.h"
> > +#include "pgtable_types.h"
> >  
> >  #include <vdso/limits.h>
> >  #include <uapi/asm/vmx.h>
> >  
> >  #include <asm/shared/tdx.h>
> > +#include <asm/page_types.h>
> >  
> >  /* Called from __tdx_hypercall() for unrecoverable failure */
> >  void __tdx_hypercall_failed(void)
> > @@ -75,3 +78,78 @@ void early_tdx_detect(void)
> >  	pio_ops.f_outb = tdx_outb;
> >  	pio_ops.f_outw = tdx_outw;
> >  }
> > +
> > +static unsigned long try_accept_one(phys_addr_t start, unsigned long len,
> > +				    enum pg_level level)
> 
> That's pretty much a copy of the same function in arch/x86/coco/tdx/tdx.c.
> 
> Yeah, you need a tdx-shared.c which you include in both places just like
> it is done with sev-shared.c

Okay, will look into this.

> > +		accept_size = try_accept_one(start, len, PG_LEVEL_1G);
> > +		if (!accept_size)
> > +			accept_size = try_accept_one(start, len, PG_LEVEL_2M);
> > +		if (!accept_size)
> > +			accept_size = try_accept_one(start, len, PG_LEVEL_4K);
> > +		if (!accept_size)
> > +			error("Accepting memory failed\n");
> > +		start += accept_size;
> 
> This series of calls to try_accept_one() appear in at least three
> places. Please carve them out into a separate function can put it in
> tdx-shared.c.

Okay.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov