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

From: Borislav Petkov <bp@alien8.de>
To: "Kirill A. Shutemov" <kirill@shutemov.name>
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:01:12 +0200	[thread overview]
Message-ID: <Yl3RmPhdZieSr8W2@zn.tnic> (raw)
In-Reply-To: <20220418202431.whvql4w57c7l5vpw@box.shutemov.name>

On Mon, Apr 18, 2022 at 11:24:31PM +0300, Kirill A. Shutemov wrote:
> <linux/bitmap> doesn't include <linux/kernel.h> or similar things.
> Is it okay for now?

No, it is not ok because those linux/ includes are moving targets. They
keep changing and then that indirectly influences the decompressor.

How much functionality from linux/bitmap.h do you actually need?

> But the goal of the function is not to accept the memory, but mark it
> as unaccepted in the bitmap.

Really?

+	 * Accept small regions that might not be able to be represented
+	 * in the bitmap:
+	 */
+	if (end - start < 2 * PMD_SIZE) {
+		__accept_memory(start, end);

That looks like it is accepting to me.

> 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.

So there *might* be some potential future use. Nothing concrete at the
moment.

> 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.

So I have not read a single argument for why the unaccepted memory gunk
should be separate.

We have perfectly fine mem_encrypt.[ch] files everywhere which already
contain code which deals with the kernel running as encrypted guest. The
unaccepted memory stuff is part of that - not something separate.

If it gets to get used for something different, sure, then it can be
carved out because it might need to be built separately, without the
rest of the encryption code. But as it is now, it doesn't have to. So
please put it in those files.

> 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?

Ok, thanks.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette