| Message ID | 20220405234343.74045-4-kirill.shutemov@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm, x86/cc: Implement support for unaccepted memory | expand |
On 4/5/22 16:43, Kirill A. Shutemov wrote: > +void mark_unaccepted(struct boot_params *params, u64 start, u64 end) > +{ > + /* > + * 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. > + */ > + > + /* > + * 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; > + } This is not my first time looking at this code and I still had to think about this a bit. That's not good. That pathological case here is actually something like this: | 4k | 2044k + 2044k | 4k | ^ 0x0 ^ 2MB ^ 4MB Where we have a 2MB-aligned 4k accepted area, a 4088k unaccepted area, then another 4k accepted area. That will not result in any bits being set in the accepted memory bitmap because no 2MB region is fully accepted. The one oddball case is this: | 4k | 2044k | 2048k | ^ 0x0 ^ 2MB ^ 4MB Which would fall into the if() above, but *can* have part of its range marked in the bitmap. Maybe we need something more like this: /* * Accept small regions that might not be able to be represented * in the bitmap. This is a bit imprecise and may accept some * areas that could have been represented in the bitmap instead. * But, the imprecision makes the code simpler by ensuring that * at least one bit will be set int the bitmap below. */ Although that's seeming a _bit_ verbose, and somewhat duplicates the comment below. > + /* > + * 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: */ > + 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 > new file mode 100644 > index 000000000000..cbc24040b853 > --- /dev/null > +++ b/arch/x86/include/asm/unaccepted_memory.h > @@ -0,0 +1,12 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Copyright (C) 2020 Intel Corporation */ > +#ifndef _ASM_X86_UNACCEPTED_MEMORY_H > +#define _ASM_X86_UNACCEPTED_MEMORY_H > + > +#include <linux/types.h> > + > +struct boot_params; > + > +void mark_unaccepted(struct boot_params *params, u64 start, u64 num); > + > +#endif > diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h > index b25d3f82c2f3..16bc686a198d 100644 > --- a/arch/x86/include/uapi/asm/bootparam.h > +++ b/arch/x86/include/uapi/asm/bootparam.h > @@ -217,7 +217,8 @@ struct boot_params { > struct boot_e820_entry e820_table[E820_MAX_ENTRIES_ZEROPAGE]; /* 0x2d0 */ > __u8 _pad8[48]; /* 0xcd0 */ > struct edd_info eddbuf[EDDMAXNR]; /* 0xd00 */ > - __u8 _pad9[276]; /* 0xeec */ > + __u64 unaccepted_memory; /* 0xeec */ > + __u8 _pad9[268]; /* 0xef4 */ > } __attribute__((packed)); > > /** > diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig > index 2c3dac5ecb36..b17ceec757d0 100644 > --- a/drivers/firmware/efi/Kconfig > +++ b/drivers/firmware/efi/Kconfig > @@ -243,6 +243,21 @@ config EFI_DISABLE_PCI_DMA > options "efi=disable_early_pci_dma" or "efi=no_disable_early_pci_dma" > may be used to override this option. > > +config UNACCEPTED_MEMORY > + bool > + depends on EFI_STUB > + depends on !KEXEC_CORE The changelog should probably say something about how the kexec() incompatibility is going to be rectified in the future. > + help > + Some Virtual Machine platforms, such as Intel TDX, require > + some memory to be "accepted" by the guest before it can be used. > + This mechanism helps prevent malicious hosts from making changes > + to guest memory. > + > + UEFI specification v2.9 introduced EFI_UNACCEPTED_MEMORY memory type. > + > + This option adds support for unaccepted memory and makes such memory > + usable by kernel. > + > endmenu BTW, what happens if this is compiled out? Do TDX guests just lose all the unaccepted memory? Should TDX be selecting this or something? > config EFI_EMBEDDED_FIRMWARE > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c > index 5502e176d51b..2c055afb1b11 100644 > --- a/drivers/firmware/efi/efi.c > +++ b/drivers/firmware/efi/efi.c > @@ -747,6 +747,7 @@ static __initdata char memory_type_name[][13] = { > "MMIO Port", > "PAL Code", > "Persistent", > + "Unaccepted", > }; > > char * __init efi_md_typeattr_format(char *buf, size_t size, > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c > index d18cac8ab436..e7601fd612aa 100644 > --- a/drivers/firmware/efi/libstub/x86-stub.c > +++ b/drivers/firmware/efi/libstub/x86-stub.c > @@ -9,12 +9,14 @@ > #include <linux/efi.h> > #include <linux/pci.h> > #include <linux/stddef.h> > +#include <linux/bitmap.h> > > #include <asm/efi.h> > #include <asm/e820/types.h> > #include <asm/setup.h> > #include <asm/desc.h> > #include <asm/boot.h> > +#include <asm/unaccepted_memory.h> > > #include "efistub.h" > > @@ -504,6 +506,13 @@ setup_e820(struct boot_params *params, struct setup_data *e820ext, u32 e820ext_s > e820_type = E820_TYPE_PMEM; > break; > > + case EFI_UNACCEPTED_MEMORY: > + if (!IS_ENABLED(CONFIG_UNACCEPTED_MEMORY)) > + continue; This seems worthy of a pr_info(). We're effectively throwing away memory with this "continue", right? > + e820_type = E820_TYPE_RAM; > + mark_unaccepted(params, d->phys_addr, > + d->phys_addr + PAGE_SIZE * d->num_pages); > + break; > default: > continue; > } > @@ -575,6 +584,9 @@ static efi_status_t allocate_e820(struct boot_params *params, > { > efi_status_t status; > __u32 nr_desc; > + bool unaccepted_memory_present = false; > + 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; > } > + > + 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. > + * > + * TODO: handle situation if params->unaccepted_memory has already set. > + * It's required to deal with kexec. Ahhh.... That's good info for the changelog. > + * The bitmap will be populated in setup_e820() according to the memory > + * map after efi_exit_boot_services(). > + */ > + if (unaccepted_memory_present) { > + unsigned long *unaccepted_memory = NULL; > + u64 size = DIV_ROUND_UP(max_addr, PMD_SIZE * BITS_PER_BYTE); > + > + status = efi_allocate_pages(size, > + (unsigned long *)&unaccepted_memory, > + ULONG_MAX); > + if (status != EFI_SUCCESS) > + goto out; > + memset(unaccepted_memory, 0, size); > + params->unaccepted_memory = (unsigned long)unaccepted_memory; > + } else { > + params->unaccepted_memory = 0; > + } > + > out: > efi_bs_call(free_pool, *map->map); > - return EFI_SUCCESS; > + return status; > + > } > > struct exit_boot_struct { > diff --git a/include/linux/efi.h b/include/linux/efi.h > index ccd4d3f91c98..b0240fdcaf5b 100644 > --- a/include/linux/efi.h > +++ b/include/linux/efi.h > @@ -108,7 +108,8 @@ typedef struct { > #define EFI_MEMORY_MAPPED_IO_PORT_SPACE 12 > #define EFI_PAL_CODE 13 > #define EFI_PERSISTENT_MEMORY 14 > -#define EFI_MAX_MEMORY_TYPE 15 > +#define EFI_UNACCEPTED_MEMORY 15 > +#define EFI_MAX_MEMORY_TYPE 16 > > /* Attribute values: */ > #define EFI_MEMORY_UC ((u64)0x0000000000000001ULL) /* uncached */
On Fri, Apr 08, 2022 at 10:26:14AM -0700, Dave Hansen wrote: > On 4/5/22 16:43, Kirill A. Shutemov wrote: > > +void mark_unaccepted(struct boot_params *params, u64 start, u64 end) > > +{ > > + /* > > + * 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. > > + */ > > + > > + /* > > + * 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; > > + } > > This is not my first time looking at this code and I still had to think > about this a bit. That's not good. That pathological case here is > actually something like this: > > | 4k | 2044k + 2044k | 4k | > ^ 0x0 ^ 2MB ^ 4MB > > Where we have a 2MB-aligned 4k accepted area, a 4088k unaccepted area, > then another 4k accepted area. That will not result in any bits being > set in the accepted memory bitmap because no 2MB region is fully accepted. > > The one oddball case is this: > > | 4k | 2044k | 2048k | > ^ 0x0 ^ 2MB ^ 4MB > > Which would fall into the if() above, but *can* have part of its range > marked in the bitmap. > > Maybe we need something more like this: > > /* > * Accept small regions that might not be able to be represented > * in the bitmap. This is a bit imprecise and may accept some > * areas that could have been represented in the bitmap instead. > * But, the imprecision makes the code simpler by ensuring that > * at least one bit will be set int the bitmap below. > */ Okay, will change. > > diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig > > index 2c3dac5ecb36..b17ceec757d0 100644 > > --- a/drivers/firmware/efi/Kconfig > > +++ b/drivers/firmware/efi/Kconfig > > @@ -243,6 +243,21 @@ config EFI_DISABLE_PCI_DMA > > options "efi=disable_early_pci_dma" or "efi=no_disable_early_pci_dma" > > may be used to override this option. > > > > +config UNACCEPTED_MEMORY > > + bool > > + depends on EFI_STUB > > + depends on !KEXEC_CORE > > The changelog should probably say something about how the kexec() > incompatibility is going to be rectified in the future. Okay. > > + help > > + Some Virtual Machine platforms, such as Intel TDX, require > > + some memory to be "accepted" by the guest before it can be used. > > + This mechanism helps prevent malicious hosts from making changes > > + to guest memory. > > + > > + UEFI specification v2.9 introduced EFI_UNACCEPTED_MEMORY memory type. > > + > > + This option adds support for unaccepted memory and makes such memory > > + usable by kernel. > > + > > endmenu > > BTW, what happens if this is compiled out? Do TDX guests just lose all > the unaccepted memory? No. It will not have access to unaccepted memory and will only use memory accepted by BIOS. > Should TDX be selecting this or something? Yes, it should and we do this. > > @@ -504,6 +506,13 @@ setup_e820(struct boot_params *params, struct setup_data *e820ext, u32 e820ext_s > > e820_type = E820_TYPE_PMEM; > > break; > > > > + case EFI_UNACCEPTED_MEMORY: > > + if (!IS_ENABLED(CONFIG_UNACCEPTED_MEMORY)) > > + continue; > > This seems worthy of a pr_info(). We're effectively throwing away > memory with this "continue", right? Yes. In this case we threat unaccepted as reserved and inaccessible to kernel. Maybe pr_warn() is more appropriate.
On Fri, Apr 08, 2022 at 10:26:14AM -0700, Dave Hansen wrote: > > + /* > > + * 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; > > + } > > This is not my first time looking at this code and I still had to think > about this a bit. That's not good. That pathological case here is > actually something like this: > > | 4k | 2044k + 2044k | 4k | > ^ 0x0 ^ 2MB ^ 4MB > > Where we have a 2MB-aligned 4k accepted area, a 4088k unaccepted area, > then another 4k accepted area. That will not result in any bits being > set in the accepted memory bitmap because no 2MB region is fully accepted. I could use that ascii art very well in a comment above it instead of having to paint it in my mind each time. Thx.
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? > =========== ===== ======================= ================================================= > 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. > +void __bitmap_set(unsigned long *map, unsigned int start, int len) > +{ > + unsigned long *p = map + BIT_WORD(start); > + const unsigned int size = start + len; > + int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG); > + unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start); > + > + while (len - bits_to_set >= 0) { > + *p |= mask_to_set; > + len -= bits_to_set; > + bits_to_set = BITS_PER_LONG; > + mask_to_set = ~0UL; > + p++; > + } > + if (len) { > + mask_to_set &= BITMAP_LAST_WORD_MASK(size); > + *p |= mask_to_set; > + } > +} > diff --git a/arch/x86/boot/compressed/unaccepted_memory.c b/arch/x86/boot/compressed/unaccepted_memory.c > new file mode 100644 > index 000000000000..d363acf59c08 > --- /dev/null > +++ b/arch/x86/boot/compressed/unaccepted_memory.c arch/x86/boot/compressed/mem.c simply. That "unaccepted_memory" everywhere is a mouthful and too specific. > @@ -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... > +{ > + /* > + * 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? > + 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... > diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig > index 2c3dac5ecb36..b17ceec757d0 100644 > --- a/drivers/firmware/efi/Kconfig > +++ b/drivers/firmware/efi/Kconfig > @@ -243,6 +243,21 @@ config EFI_DISABLE_PCI_DMA > options "efi=disable_early_pci_dma" or "efi=no_disable_early_pci_dma" > may be used to override this option. > > +config UNACCEPTED_MEMORY > + bool > + depends on EFI_STUB > + depends on !KEXEC_CORE > + help > + Some Virtual Machine platforms, such as Intel TDX, require > + some memory to be "accepted" by the guest before it can be used. > + This mechanism helps prevent malicious hosts from making changes > + to guest memory. > + > + UEFI specification v2.9 introduced EFI_UNACCEPTED_MEMORY memory type. > + > + This option adds support for unaccepted memory and makes such memory > + usable by kernel. ... by *the* kernel. > + > endmenu > > config EFI_EMBEDDED_FIRMWARE > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c > index 5502e176d51b..2c055afb1b11 100644 > --- a/drivers/firmware/efi/efi.c > +++ b/drivers/firmware/efi/efi.c > @@ -747,6 +747,7 @@ static __initdata char memory_type_name[][13] = { > "MMIO Port", > "PAL Code", > "Persistent", > + "Unaccepted", > }; > > char * __init efi_md_typeattr_format(char *buf, size_t size, > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c > index d18cac8ab436..e7601fd612aa 100644 > --- a/drivers/firmware/efi/libstub/x86-stub.c > +++ b/drivers/firmware/efi/libstub/x86-stub.c > @@ -9,12 +9,14 @@ > #include <linux/efi.h> > #include <linux/pci.h> > #include <linux/stddef.h> > +#include <linux/bitmap.h> > > #include <asm/efi.h> > #include <asm/e820/types.h> > #include <asm/setup.h> > #include <asm/desc.h> > #include <asm/boot.h> > +#include <asm/unaccepted_memory.h> > > #include "efistub.h" > > @@ -504,6 +506,13 @@ setup_e820(struct boot_params *params, struct setup_data *e820ext, u32 e820ext_s > e820_type = E820_TYPE_PMEM; > break; > > + case EFI_UNACCEPTED_MEMORY: > + if (!IS_ENABLED(CONFIG_UNACCEPTED_MEMORY)) > + continue; > + e820_type = E820_TYPE_RAM; > + mark_unaccepted(params, d->phys_addr, > + d->phys_addr + PAGE_SIZE * d->num_pages); > + break; > default: > continue; > } > @@ -575,6 +584,9 @@ static efi_status_t allocate_e820(struct boot_params *params, > { > efi_status_t status; > __u32 nr_desc; > + bool unaccepted_memory_present = false; This wholly written out "unaccepted_memory" everywhere is too much and too long. How about bool unaccept_mem; or so? > + 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. > + > + 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? > + * > + * TODO: handle situation if params->unaccepted_memory has already set. > + * It's required to deal with kexec. > + * > + * The bitmap will be populated in setup_e820() according to the memory > + * map after efi_exit_boot_services(). > + */ > + if (unaccepted_memory_present) { > + unsigned long *unaccepted_memory = NULL; So if you call this simply unsigned long *mem = ... > + u64 size = DIV_ROUND_UP(max_addr, PMD_SIZE * BITS_PER_BYTE); > + > + status = efi_allocate_pages(size, > + (unsigned long *)&unaccepted_memory, > + ULONG_MAX); ... you'd have this on a single line: status = efi_allocate_pages(size, (unsigned long *)&mem, ULONG_MAX); > + if (status != EFI_SUCCESS) > + goto out; > + memset(unaccepted_memory, 0, size); > + params->unaccepted_memory = (unsigned long)unaccepted_memory; ... and then have this assignment more readable: params->unaccepted_memory = (unsigned long)mem; as it shows the important var being ->unaccepted_memory and mem only a local helper.
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?
On Mon, Apr 18, 2022 at 06:55:45PM +0300, Kirill A. Shutemov wrote: > I'm confused here. What is wrong with linux/ include namespace? The problem is that you need all kinds of workarounds so that the decompressor builds. Just look at the beginning of arch/x86/boot/compressed/misc.h Even you had to do them: /* cpu_feature_enabled() cannot be used this early */ #define USE_EARLY_PGTABLE_L5 That thing sprinkled everywhere is not a clean solution. > 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. Yes, those are fine except that not everything that leaks into the decompressor code through includes is perfectly portable. > 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. So I'd like for the decompressor to be completely separate from kernel proper because it is a whole different thing and I want for us to be able to include headers in it without ugly workarounds just so that kernel proper include changes do not influence the decompressor. > Hm. accept_or_mark_unaccepted()? What's wrong with early_accept_memory()? > > Immediately? As opposed to delayed? > > Yes. Otherwise accept is delayed until the first allocation of the memory. Yes, put that in the comment pls. > 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. Really? Please elaborate. I thought memory acceptance is a feature solely for TDX and SNP guests to use. > <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. > 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. > > 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? From my experience, asking firmware to do stuff for ya is always a risky thing. I guess such a huge allocation, when it fails, will be caught early in platform verification so whatever...
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?
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.
On Mon, Apr 18, 2022 at 11:01:12PM +0200, Borislav Petkov wrote: > 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? Below is the bare minimum required to compile bitmap.c in decompresser. I only made it work on my config/compiler and did not care about all #ifdef branches. I find it strange that you go after <linux/bitmap.h> which has limited exposure while <linux/acpi.h> and <linux/efi.h> are there already. Starting small will backfire if once we find out that monstrous headers depend on what we try to replace. Bit fish has to be addressed first. What do you want me to do here? // <linux/bitmap.h> // #define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - 1))) #define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG - 1))) // <uapi/linux/swab.> /** * __swab64 - return a byteswapped 64-bit value * @x: value to byteswap */ #ifdef __HAVE_BUILTIN_BSWAP64__ #define __swab64(x) (__u64)__builtin_bswap64((__u64)(x)) #else #define __swab64(x) \ (__builtin_constant_p((__u64)(x)) ? \ ___constant_swab64(x) : \ __fswab64(x)) #endif static __always_inline unsigned long __swab(const unsigned long y) { #if __BITS_PER_LONG == 64 return __swab64(y); #else /* __BITS_PER_LONG == 32 */ return __swab32(y); #endif } // <linux/swab.h> # define swab __swab // <linux/bits.h> #define BIT_WORD(nr) ((nr) / BITS_PER_LONG) // <asm/bitops.h> // /** * __ffs - find first set bit in word * @word: The word to search * * Undefined if no bit exists, so code should check against 0 first. */ static __always_inline unsigned long __ffs(unsigned long word) { asm("rep; bsf %1,%0" : "=r" (word) : "rm" (word)); return word; } > > 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. Yes, really. As 1 bit represents 2M, not all chunks can be represented in the bitmap and they have to be accepted. But the *goal* is to record unaccepted memory into bitmap. Some accepting is a side effect. The early_accept_memory() name is just wrong. > > 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. And some stuff for encrypted host (SME). > 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. Okay, I will do as you want, but I really hate it. With one hand you try to unwind header mess in decompresser code and with another propose to create a kitchen-sink header because topics somewhat related. Looks contradictory.
On Tue, Apr 19, 2022 at 02:50:15AM +0300, Kirill A. Shutemov wrote: > I find it strange that you go after <linux/bitmap.h> which has limited > exposure while <linux/acpi.h> and <linux/efi.h> are there already. Funny you should mention that: https://lore.kernel.org/r/YlCKWhMJEMUgJmjF@zn.tnic I *have* been working towards that but it's a losing whack-a-mole game when you and others keep adding new stuff. So no, we won't take a pile of changes and let the maintainer clean it up afterwards. > What do you want me to do here? I think the stuff coming from the linux/ namespace you can simply copy into a header in compressed/, like I've done with efi.h. > // <asm/bitops.h> The asm/ stuff can be put into a shared/ namespace header like the io stuff you did. > As 1 bit represents 2M, not all chunks can be represented in the bitmap > and they have to be accepted. But the *goal* is to record unaccepted > memory into bitmap. Some accepting is a side effect. > > The early_accept_memory() name is just wrong. Ok, how about process_unaccepted_memory(). It should be generic enough. > Okay, I will do as you want, but I really hate it. I find it really weird that you feel so strongly about it. If I would have been asked to do it, I would've done it without even considering it. But ok, since you feel so strongly about it, I've asked what the other maintainers think.
On Tue, Apr 19, 2022 at 09:39:53AM +0200, Borislav Petkov wrote: > On Tue, Apr 19, 2022 at 02:50:15AM +0300, Kirill A. Shutemov wrote: > > I find it strange that you go after <linux/bitmap.h> which has limited > > exposure while <linux/acpi.h> and <linux/efi.h> are there already. > > Funny you should mention that: > > https://lore.kernel.org/r/YlCKWhMJEMUgJmjF@zn.tnic > > I *have* been working towards that but it's a losing whack-a-mole game > when you and others keep adding new stuff. > > So no, we won't take a pile of changes and let the maintainer clean it > up afterwards. > > > What do you want me to do here? > > I think the stuff coming from the linux/ namespace you can simply copy > into a header in compressed/, like I've done with efi.h. Hm. Dave was worried about having copies of _find_next_bit() and __bitmap_*() inside compressed/. How do we rectify code duplication and making decompresser self-contained? Do we care about multiple copies of the same code in the kernel? Do we care about keeping them in sync? > > // <asm/bitops.h> > > The asm/ stuff can be put into a shared/ namespace header like the io > stuff you did. > > > As 1 bit represents 2M, not all chunks can be represented in the bitmap > > and they have to be accepted. But the *goal* is to record unaccepted > > memory into bitmap. Some accepting is a side effect. > > > > The early_accept_memory() name is just wrong. > > Ok, how about process_unaccepted_memory(). It should be generic enough. Sounds good.
On 4/19/22 08:30, Kirill A. Shutemov wrote: >> I think the stuff coming from the linux/ namespace you can simply copy >> into a header in compressed/, like I've done with efi.h. > Hm. Dave was worried about having copies of _find_next_bit() and > __bitmap_*() inside compressed/. > > How do we rectify code duplication and making decompresser self-contained? > Do we care about multiple copies of the same code in the kernel? > Do we care about keeping them in sync? Would it be feasible to have the common code defined as a 'static inline' in a header that both the main kernel and the decompressor could include? Something like the attached patch. I'd much rather duplicate something like this: int strncasecmp(const char *s1, const char *s2, size_t len) { return __lib_strncasecmp(s1, s1, len); } in the decompressor versus a real full implementation.
On Tue, Apr 19, 2022 at 06:30:02PM +0300, Kirill A. Shutemov wrote: > Hm. Dave was worried about having copies of _find_next_bit() and > __bitmap_*() inside compressed/. That's fine. > How do we rectify code duplication and making decompresser self-contained? Also fine - as long as the decompressor and kernel-proper are independent. > Do we care about multiple copies of the same code in the kernel? The copied versions in the decompressor should be simply sufficient for its use. And there shouldn't be that much of duplication. Note that we're using the same strategy with perf tool - it does copy kernel facilities when it needs them. > Do we care about keeping them in sync? Nope - as long as they're sufficient for the decompressor. My expectation here is that the decompressor won't need too many facilities. Thx.
On Tue, Apr 19, 2022 at 09:39:53AM +0200, Borislav Petkov wrote: > I find it really weird that you feel so strongly about it. If I would > have been asked to do it, I would've done it without even considering > it. But ok, since you feel so strongly about it, I've asked what the > other maintainers think. Ok, Dave thinks separate files are better so let's leave it at that. We can always change things later.
On Tue, Apr 19, 2022 at 09:39:53AM +0200, Borislav Petkov wrote: > On Tue, Apr 19, 2022 at 02:50:15AM +0300, Kirill A. Shutemov wrote: > > I find it strange that you go after <linux/bitmap.h> which has limited > > exposure while <linux/acpi.h> and <linux/efi.h> are there already. > > Funny you should mention that: > > https://lore.kernel.org/r/YlCKWhMJEMUgJmjF@zn.tnic There's still #include <linux/efi.h> in misc.h. You removed one, but there's a second one for some reason. Any plans for <linux/acpi.h>? It includes <linux/bitmap.h>: In file included from ./include/linux/cpumask.h:12, from ./include/linux/smp.h:13, from ./include/linux/lockdep.h:14, from ./include/linux/mutex.h:17, from ./include/linux/kernfs.h:11, from ./include/linux/sysfs.h:16, from ./include/linux/kobject.h:20, from ./include/linux/of.h:17, from ./include/linux/irqdomain.h:35, from ./include/linux/acpi.h:13, from arch/x86/boot/compressed/misc.h:3 We will get name conflicts if we try to copy <linux/bitmap.h> stuff. Hm. I also underesitmated what is required to be copied because of the indirect include. The list was only to compile bitmap.c. mem.c (former unaccepted_memory.c) would require more. BTW, do we have a white list of linux/ includes that allowed? minmax.h? math.h? What is the line. Maybe allow what is included directly or indirectly now? (Yes, it is my poor attempt to slide under closing door.)
On Fri, Apr 22, 2022 at 03:21:24AM +0300, Kirill A. Shutemov wrote: > There's still #include <linux/efi.h> in misc.h. You removed one, but > there's a second one for some reason. I don't know which tree you're looking at but latest tip/master has: $ git grep -E "efi\.h" arch/x86/boot/ arch/x86/boot/compressed/acpi.c:6:#include "efi.h" arch/x86/boot/compressed/kaslr.c:25:#include "efi.h" arch/x86/boot/compressed/misc.h:40:#include "efi.h" arch/x86/boot/compressed/pgtable_64.c:7:#include "efi.h" > Any plans for <linux/acpi.h>? It includes <linux/bitmap.h>: So if misc.h is including linux/bitmap.h indirectly, you can simply include misc.h right? And then you'll slide under the closing door, as you say below. :) > I also underesitmated what is required to be copied because of the > indirect include. The list was only to compile bitmap.c. mem.c (former > unaccepted_memory.c) would require more. More like? Maybe I can help out converting some of the stuff. You could push your current state somewhere - even if it doesn't build - so that I can take a look... > BTW, do we have a white list of linux/ includes that allowed? minmax.h? > math.h? What is the line. Well, that's the thing. Even if those look innocuous now, if they get new includes added to them, that has an influence on the decompressor. So I'm thinking copying the required bits would be the proper way forward. > Maybe allow what is included directly or indirectly now? (Yes, it is my > poor attempt to slide under closing door.) That's basically saying, can I get this done so that I can mark my checkbox that my task is done - you can deal with the crap later yourself. How about we take our time and solve this properly instead of hurrying constantly? Thx.
On Fri, Apr 22, 2022 at 11:30:11AM +0200, Borislav Petkov wrote: > On Fri, Apr 22, 2022 at 03:21:24AM +0300, Kirill A. Shutemov wrote: > > There's still #include <linux/efi.h> in misc.h. You removed one, but > > there's a second one for some reason. > > I don't know which tree you're looking at but latest tip/master has: > > $ git grep -E "efi\.h" arch/x86/boot/ > arch/x86/boot/compressed/acpi.c:6:#include "efi.h" > arch/x86/boot/compressed/kaslr.c:25:#include "efi.h" > arch/x86/boot/compressed/misc.h:40:#include "efi.h" > arch/x86/boot/compressed/pgtable_64.c:7:#include "efi.h" Sorry for the noise. I read 'elf.h' as 'efi.h' :/ But it also includes <linux/bitmap.h> indirectly: In file included from include/linux/elf.h:6: In file included from arch/x86/include/asm/elf.h:8: In file included from include/linux/thread_info.h:60: In file included from arch/x86/include/asm/thread_info.h:53: In file included from arch/x86/include/asm/cpufeature.h:5: In file included from arch/x86/include/asm/processor.h:22: In file included from arch/x86/include/asm/msr.h:11: In file included from arch/x86/include/asm/cpumask.h:5: In file included from include/linux/cpumask.h:12: > > Any plans for <linux/acpi.h>? It includes <linux/bitmap.h>: > > So if misc.h is including linux/bitmap.h indirectly, you can simply > include misc.h right? Yes. > And then you'll slide under the closing door, as you say below. :) Is it sarcasm or clearance to go? > > I also underesitmated what is required to be copied because of the > > indirect include. The list was only to compile bitmap.c. mem.c (former > > unaccepted_memory.c) would require more. > > More like? for_each_clear_bitrange() is pain to unwind. > Maybe I can help out converting some of the stuff. You could push your > current state somewhere - even if it doesn't build - so that I can take > a look... I will push what I have a bit later today. > > BTW, do we have a white list of linux/ includes that allowed? minmax.h? > > math.h? What is the line. > > Well, that's the thing. Even if those look innocuous now, if they get > new includes added to them, that has an influence on the decompressor. > > So I'm thinking copying the required bits would be the proper way > forward. I understand where you comes from. But on my side I face suddenly higher entry bar. Yes, it is bad excuse, I know. > > Maybe allow what is included directly or indirectly now? (Yes, it is my > > poor attempt to slide under closing door.) > > That's basically saying, can I get this done so that I can mark my > checkbox that my task is done - you can deal with the crap later > yourself. > > How about we take our time and solve this properly instead of hurrying > constantly? I'm okay with this. But I lack coherent understating on how you want it to look like. Like, looking on your new "efi.h", I see it still implicitly depends on <linux/types.h> and <linux/uuid.h>. Why is it okay? Is it temporary? What is criteria of what is okay to keep for now? You mentioned having <asm/shared/bitops.h> as we do <asm/shared/io.h>. But <asm/bitops.h> has non-trivial dependencies on its own. Okay, we can move them into asm/shared as well, but how to deal with asm-generic/ things? And linux/ dependencies? Do we create a copy in x86/include?
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) =========== ===== ======================= ================================================= 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> + +void __bitmap_set(unsigned long *map, unsigned int start, int len) +{ + unsigned long *p = map + BIT_WORD(start); + const unsigned int size = start + len; + int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG); + unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start); + + while (len - bits_to_set >= 0) { + *p |= mask_to_set; + len -= bits_to_set; + bits_to_set = BITS_PER_LONG; + mask_to_set = ~0UL; + p++; + } + if (len) { + mask_to_set &= BITMAP_LAST_WORD_MASK(size); + *p |= mask_to_set; + } +} diff --git a/arch/x86/boot/compressed/unaccepted_memory.c b/arch/x86/boot/compressed/unaccepted_memory.c new file mode 100644 index 000000000000..d363acf59c08 --- /dev/null +++ b/arch/x86/boot/compressed/unaccepted_memory.c @@ -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) +{ + /* + * 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. + */ + + /* + * 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: */ + 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 new file mode 100644 index 000000000000..cbc24040b853 --- /dev/null +++ b/arch/x86/include/asm/unaccepted_memory.h @@ -0,0 +1,12 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Copyright (C) 2020 Intel Corporation */ +#ifndef _ASM_X86_UNACCEPTED_MEMORY_H +#define _ASM_X86_UNACCEPTED_MEMORY_H + +#include <linux/types.h> + +struct boot_params; + +void mark_unaccepted(struct boot_params *params, u64 start, u64 num); + +#endif diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h index b25d3f82c2f3..16bc686a198d 100644 --- a/arch/x86/include/uapi/asm/bootparam.h +++ b/arch/x86/include/uapi/asm/bootparam.h @@ -217,7 +217,8 @@ struct boot_params { struct boot_e820_entry e820_table[E820_MAX_ENTRIES_ZEROPAGE]; /* 0x2d0 */ __u8 _pad8[48]; /* 0xcd0 */ struct edd_info eddbuf[EDDMAXNR]; /* 0xd00 */ - __u8 _pad9[276]; /* 0xeec */ + __u64 unaccepted_memory; /* 0xeec */ + __u8 _pad9[268]; /* 0xef4 */ } __attribute__((packed)); /** diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig index 2c3dac5ecb36..b17ceec757d0 100644 --- a/drivers/firmware/efi/Kconfig +++ b/drivers/firmware/efi/Kconfig @@ -243,6 +243,21 @@ config EFI_DISABLE_PCI_DMA options "efi=disable_early_pci_dma" or "efi=no_disable_early_pci_dma" may be used to override this option. +config UNACCEPTED_MEMORY + bool + depends on EFI_STUB + depends on !KEXEC_CORE + help + Some Virtual Machine platforms, such as Intel TDX, require + some memory to be "accepted" by the guest before it can be used. + This mechanism helps prevent malicious hosts from making changes + to guest memory. + + UEFI specification v2.9 introduced EFI_UNACCEPTED_MEMORY memory type. + + This option adds support for unaccepted memory and makes such memory + usable by kernel. + endmenu config EFI_EMBEDDED_FIRMWARE diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c index 5502e176d51b..2c055afb1b11 100644 --- a/drivers/firmware/efi/efi.c +++ b/drivers/firmware/efi/efi.c @@ -747,6 +747,7 @@ static __initdata char memory_type_name[][13] = { "MMIO Port", "PAL Code", "Persistent", + "Unaccepted", }; char * __init efi_md_typeattr_format(char *buf, size_t size, diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c index d18cac8ab436..e7601fd612aa 100644 --- a/drivers/firmware/efi/libstub/x86-stub.c +++ b/drivers/firmware/efi/libstub/x86-stub.c @@ -9,12 +9,14 @@ #include <linux/efi.h> #include <linux/pci.h> #include <linux/stddef.h> +#include <linux/bitmap.h> #include <asm/efi.h> #include <asm/e820/types.h> #include <asm/setup.h> #include <asm/desc.h> #include <asm/boot.h> +#include <asm/unaccepted_memory.h> #include "efistub.h" @@ -504,6 +506,13 @@ setup_e820(struct boot_params *params, struct setup_data *e820ext, u32 e820ext_s e820_type = E820_TYPE_PMEM; break; + case EFI_UNACCEPTED_MEMORY: + if (!IS_ENABLED(CONFIG_UNACCEPTED_MEMORY)) + continue; + e820_type = E820_TYPE_RAM; + mark_unaccepted(params, d->phys_addr, + d->phys_addr + PAGE_SIZE * d->num_pages); + break; default: continue; } @@ -575,6 +584,9 @@ static efi_status_t allocate_e820(struct boot_params *params, { efi_status_t status; __u32 nr_desc; + bool unaccepted_memory_present = false; + 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; } + + 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. + * + * TODO: handle situation if params->unaccepted_memory has already set. + * It's required to deal with kexec. + * + * The bitmap will be populated in setup_e820() according to the memory + * map after efi_exit_boot_services(). + */ + if (unaccepted_memory_present) { + unsigned long *unaccepted_memory = NULL; + u64 size = DIV_ROUND_UP(max_addr, PMD_SIZE * BITS_PER_BYTE); + + status = efi_allocate_pages(size, + (unsigned long *)&unaccepted_memory, + ULONG_MAX); + if (status != EFI_SUCCESS) + goto out; + memset(unaccepted_memory, 0, size); + params->unaccepted_memory = (unsigned long)unaccepted_memory; + } else { + params->unaccepted_memory = 0; + } + out: efi_bs_call(free_pool, *map->map); - return EFI_SUCCESS; + return status; + } struct exit_boot_struct { diff --git a/include/linux/efi.h b/include/linux/efi.h index ccd4d3f91c98..b0240fdcaf5b 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -108,7 +108,8 @@ typedef struct { #define EFI_MEMORY_MAPPED_IO_PORT_SPACE 12 #define EFI_PAL_CODE 13 #define EFI_PERSISTENT_MEMORY 14 -#define EFI_MAX_MEMORY_TYPE 15 +#define EFI_UNACCEPTED_MEMORY 15 +#define EFI_MAX_MEMORY_TYPE 16 /* Attribute values: */ #define EFI_MEMORY_UC ((u64)0x0000000000000001ULL) /* uncached */
UEFI Specification version 2.9 introduces the concept of memory acceptance: Some Virtual Machine platforms, such as Intel TDX or AMD SEV-SNP, requiring memory to be accepted before it can be used by the guest. Accepting happens via a protocol specific for the Virtual Machine platform. Accepting memory is costly and it makes VMM allocate memory for the accepted guest physical address range. It's better to postpone memory acceptance until memory is needed. It lowers boot time and reduces memory overhead. The kernel needs to know what memory has been accepted. Firmware communicates this information via memory map: a new memory type -- EFI_UNACCEPTED_MEMORY -- indicates such memory. Range-based tracking works fine for firmware, but it gets bulky for the kernel: e820 has to be modified on every page acceptance. It leads to table fragmentation, but there's a limited number of entries in the e820 table Another option is to mark such memory as usable in e820 and track if the range has been accepted in a bitmap. One bit in the bitmap represents 2MiB in the address space: one 4k page is enough to track 64GiB or 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. Any unaccepted memory that is not aligned to 2M gets accepted upfront. The bitmap is allocated and constructed in the EFI stub and passed down to the kernel via boot_params. allocate_e820() allocates the bitmap if unaccepted memory is present, according to the maximum address in the memory map. The same boot_params.unaccepted_memory can be used to pass the bitmap between two kernels on kexec, but the use-case is not yet implemented. Make KEXEC and UNACCEPTED_MEMORY mutually exclusive for now. Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> --- Documentation/x86/zero-page.rst | 1 + arch/x86/boot/compressed/Makefile | 1 + arch/x86/boot/compressed/bitmap.c | 24 ++++++++ arch/x86/boot/compressed/unaccepted_memory.c | 53 +++++++++++++++++ arch/x86/include/asm/unaccepted_memory.h | 12 ++++ arch/x86/include/uapi/asm/bootparam.h | 3 +- drivers/firmware/efi/Kconfig | 15 +++++ drivers/firmware/efi/efi.c | 1 + drivers/firmware/efi/libstub/x86-stub.c | 62 +++++++++++++++++++- include/linux/efi.h | 3 +- 10 files changed, 172 insertions(+), 3 deletions(-) create mode 100644 arch/x86/boot/compressed/bitmap.c create mode 100644 arch/x86/boot/compressed/unaccepted_memory.c create mode 100644 arch/x86/include/asm/unaccepted_memory.h