Re: [PATCHv4 3/8] efi/x86: Implement support for unaccepted memory

From: "Kirill A. Shutemov" <kirill@shutemov.name>
To: Borislav Petkov <bp@alien8.de>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
	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>,
	Brijesh Singh <brijesh.singh@amd.com>,
	Mike Rapoport <rppt@kernel.org>,
	David Hildenbrand <david@redhat.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: [PATCHv4 3/8] efi/x86: Implement support for unaccepted memory
Date: Mon, 18 Apr 2022 23:24:31 +0300	[thread overview]
Message-ID: <20220418202431.whvql4w57c7l5vpw@box.shutemov.name> (raw)
In-Reply-To: <Yl2UHOQ4iZJ29k0q@zn.tnic>

On Mon, Apr 18, 2022 at 06:38:52PM +0200, Borislav Petkov wrote:
> > Could you explain what rules are?
> 
> Library-like stuff like types.h, linkage.h, etc we could include for now
> but including linux/kernel.h which pulls in everything but the kitchen
> sink is bad.

<linux/bitmap> doesn't include <linux/kernel.h> or similar things.
Is it okay for now?

> > Hm. accept_or_mark_unaccepted()?
> 
> What's wrong with early_accept_memory()?

But the goal of the function is not to accept the memory, but mark it
as unaccepted in the bitmap. Your proposal is more confusing, not less.

> > > Immediately? As opposed to delayed?
> > 
> > Yes. Otherwise accept is delayed until the first allocation of the memory.
> 
> Yes, put that in the comment pls.

Okay.

> memory, but it is not
> > 1:1 match. Unaccepted memory can be present without memory ecnryption if
> > data secruty and integrity guaranteed by other means.
> 
> Really?
> 
> Please elaborate. I thought memory acceptance is a feature solely for
> TDX and SNP guests to use.

Conceptionally, it is just memory that requires additional action before
it can be accessed. Yes, at the moment TDX and SEV are the only users.
It is implementation detail that TDX and SEV use memory encryption.

> > <asm/mem_encrypt.h> is very AMD SME/SEV centric.
> 
> So?
> 
> > I'm not sure it need to exist in the way it is now.
> 
> I'm not sure what your argument actually is for having yet another
> separate header vs putting it in a header which already deals with that
> stuff.

Because I don't think it is a good fit. Frankly, even <asm/coco.h> fits
better, although I'm no a fan either.

Do we have file shortage? I would rather keep it separate.

> > Okay, I will move it into a separate function, but it has to be called
> > from allocate_e820() because it allocates and free the map.
> 
> You mean, you want for allocate_e820() to call this new function because
> both allocate and free?
> 
> Might have to explain what you mean here exactly.

Both allocate_e820() and handling unaccepted memory requires access to the
efi memory map. We only need the size of memory map for e820, while
unaccepted memory requires walking the map. We can serve both by
requesting the map from the firmware once. It requires allocation and
freeing memory for the map.

Makes sense?

-- 
 Kirill A. Shutemov