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

["Kirill A. Shutemov" <kirill@shutemov.name>]

On Sat, Apr 16, 2022 at 12:24:26AM +0200, Borislav Petkov wrote:
> On Wed, Apr 06, 2022 at 02:43:38AM +0300, Kirill A. Shutemov wrote:
> > diff --git a/Documentation/x86/zero-page.rst b/Documentation/x86/zero-page.rst
> > index f088f5881666..8e3447a4b373 100644
> > --- a/Documentation/x86/zero-page.rst
> > +++ b/Documentation/x86/zero-page.rst
> > @@ -42,4 +42,5 @@ Offset/Size	Proto	Name		Meaning
> >  2D0/A00		ALL	e820_table		E820 memory map table
> >  						(array of struct e820_entry)
> >  D00/1EC		ALL	eddbuf			EDD data (array of struct edd_info)
> > +ECC/008		ALL	unaccepted_memory	Bitmap of unaccepted memory (1bit == 2M)
> 
> There's a perfectly fine spot at 0x78:
> 
> 	__u8  _pad3[8];                                 /* 0x078 */
> 
> why not take that one?

Good point. Will do.

> 
> >  ===========	=====	=======================	=================================================
> > diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> > index 8fd0e6ae2e1f..09993797efa2 100644
> > --- a/arch/x86/boot/compressed/Makefile
> > +++ b/arch/x86/boot/compressed/Makefile
> > @@ -102,6 +102,7 @@ endif
> >  
> >  vmlinux-objs-$(CONFIG_ACPI) += $(obj)/acpi.o
> >  vmlinux-objs-$(CONFIG_INTEL_TDX_GUEST) += $(obj)/tdx.o $(obj)/tdcall.o
> > +vmlinux-objs-$(CONFIG_UNACCEPTED_MEMORY) += $(obj)/bitmap.o $(obj)/unaccepted_memory.o
> >  
> >  vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_thunk_$(BITS).o
> >  efi-obj-$(CONFIG_EFI_STUB) = $(objtree)/drivers/firmware/efi/libstub/lib.a
> > diff --git a/arch/x86/boot/compressed/bitmap.c b/arch/x86/boot/compressed/bitmap.c
> > new file mode 100644
> > index 000000000000..bf58b259380a
> > --- /dev/null
> > +++ b/arch/x86/boot/compressed/bitmap.c
> > @@ -0,0 +1,24 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Taken from lib/string.c */
> > +
> > +#include <linux/bitmap.h>
> 
> verify_include_paths: Warning: Kernel-proper include at arch/x86/boot/compressed/bitmap.c:4 [+#include <linux/bitmap.h>]
> 
> Same game as before: put the stuff you need into a separate or a shared
> header and avoid the linux/ namespace include.

I'm confused here. What is wrong with linux/ include namespace?

Yes, we had story with <asm/io.h> that actually caused issue in
decompression code, but linux/ has a lot of perfectly portable
library-like stuff.

Could you explain what rules are?

> > @@ -0,0 +1,53 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +#include "error.h"
> > +#include "misc.h"
> > +
> > +static inline void __accept_memory(phys_addr_t start, phys_addr_t end)
> > +{
> > +	/* Platform-specific memory-acceptance call goes here */
> > +	error("Cannot accept memory");
> > +}
> > +
> > +void mark_unaccepted(struct boot_params *params, u64 start, u64 end)
> 
> That name is kinda misleading? It is not only marking as unaccepted - it
> is also accepting weird 2M misaligned chunks...

Hm. accept_or_mark_unaccepted()?

> > +{
> > +	/*
> > +	 * The accepted memory bitmap only works at PMD_SIZE granularity.
> > +	 * If a request comes in to mark memory as unaccepted which is not
> > +	 * PMD_SIZE-aligned, simply accept the memory now since it can not be
> > +	 * *marked* as unaccepted.
> > +	 */
> 
> That comment goes over the function name.
> 
> > +	/*
> > +	 * Accept small regions that might not be able to be represented
> > +	 * in the bitmap:
> > +	 */
> > +	if (end - start < 2 * PMD_SIZE) {
> > +		__accept_memory(start, end);
> > +		return;
> > +	}
> > +
> > +	/*
> > +	 * No matter how the start and end are aligned, at least one unaccepted
> > +	 * PMD_SIZE area will remain.
> > +	 */
> > +
> > +	/* Immediately accept a <PMD_SIZE piece at the start: */
> 
> Immediately? As opposed to delayed?

Yes. Otherwise accept is delayed until the first allocation of the memory.

> > +	if (start & ~PMD_MASK) {
> > +		__accept_memory(start, round_up(start, PMD_SIZE));
> > +		start = round_up(start, PMD_SIZE);
> > +	}
> > +
> > +	/* Immediately accept a <PMD_SIZE piece at the end: */
> > +	if (end & ~PMD_MASK) {
> > +		__accept_memory(round_down(end, PMD_SIZE), end);
> > +		end = round_down(end, PMD_SIZE);
> > +	}
> > +
> > +	/*
> > +	 * 'start' and 'end' are now both PMD-aligned.
> > +	 * Record the range as being unaccepted:
> > +	 */
> > +	bitmap_set((unsigned long *)params->unaccepted_memory,
> > +		   start / PMD_SIZE, (end - start) / PMD_SIZE);
> > +}
> > diff --git a/arch/x86/include/asm/unaccepted_memory.h b/arch/x86/include/asm/unaccepted_memory.h
> 
> Why do you need a separate header?
> 
> We already have
> 
> arch/x86/include/asm/mem_encrypt.h
> 
> and this is kinda very much related...

I don't see it.

Memory encryption can be a reason to have unaccepted 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.

<asm/mem_encrypt.h> is very AMD SME/SEV centric. I'm not sure it need to
exist in the way it is now.

> > +	u64 max_addr = 0;
> > +	int i;
> >  
> >  	status = efi_get_memory_map(map);
> >  	if (status != EFI_SUCCESS)
> > @@ -589,9 +601,57 @@ static efi_status_t allocate_e820(struct boot_params *params,
> >  		if (status != EFI_SUCCESS)
> >  			goto out;
> >  	}
> 
> This whole chunk you're adding here begs to be a separate function with
> the big fat comment placed over the function name.
> 
> Might just as well call it after allocate_e820() has been called.

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.

> > +
> > +	if (!IS_ENABLED(CONFIG_UNACCEPTED_MEMORY))
> > +		goto out;
> > +
> > +	/* Check if there's any unaccepted memory and find the max address */
> > +	for (i = 0; i < nr_desc; i++) {
> > +		efi_memory_desc_t *d;
> > +
> > +		d = efi_early_memdesc_ptr(*map->map, *map->desc_size, i);
> > +		if (d->type == EFI_UNACCEPTED_MEMORY)
> > +			unaccepted_memory_present = true;
> > +		if (d->phys_addr + d->num_pages * PAGE_SIZE > max_addr)
> > +			max_addr = d->phys_addr + d->num_pages * PAGE_SIZE;
> > +	}
> > +
> > +	/*
> > +	 * If unaccepted memory is present allocate a bitmap to track what
> > +	 * memory has to be accepted before access.
> > +	 *
> > +	 * One bit in the bitmap represents 2MiB in the address space:
> > +	 * A 4k bitmap can track 64GiB of physical address space.
> > +	 *
> > +	 * In the worst case scenario -- a huge hole in the middle of the
> > +	 * address space -- It needs 256MiB to handle 4PiB of the address
> > +	 * space.
> 
> And you're saying that that efi_allocate_pages() below can really give a
> 256M contiguous chunk?

Yes, that's assumption. Is it too high ask to deal with 4PiB of PA?

-- 
 Kirill A. Shutemov