On Wed, Dec 07, 2022 at 04:49:25AM +0300, Kirill A. Shutemov wrote:
> The implementation requires some basic helpers in boot stub. They
> provided by linux/ includes in the main kernel image, but is not present
> in boot stub. Create copy of required functionality in the boot stub.
Leftover paragraph from a previous version. Can be removed.
...
> +/*
> + * The accepted memory bitmap only works at PMD_SIZE granularity. This
> + * function takes unaligned start/end addresses and either:
s/This function takes/Take/
> + * 1. Accepts the memory immediately and in its entirety
> + * 2. Accepts unaligned parts, and marks *some* aligned part unaccepted
> + *
> + * The function will never reach the bitmap_set() with zero bits to set.
> + */
> +void process_unaccepted_memory(struct boot_params *params, u64 start, u64 end)
> +{
> + /*
> + * Ensure that at least one bit will be set in the bitmap by
> + * immediately accepting all regions under 2*PMD_SIZE. This is
> + * imprecise and may immediately accept some areas that could
> + * have been represented in the bitmap. But, results in simpler
> + * code below
> + *
> + * Consider case like this:
> + *
> + * | 4k | 2044k | 2048k |
> + * ^ 0x0 ^ 2MB ^ 4MB
> + *
> + * Only the first 4k has been accepted. The 0MB->2MB region can not be
> + * represented in the bitmap. The 2MB->4MB region can be represented in
> + * the bitmap. But, the 0MB->4MB region is <2*PMD_SIZE and will be
> + * immediately accepted in its entirety.
> + */
> + 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 to be marked in the bitmap.
> + */
> +
> + /* 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/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index 6787ed8dfacf..8aa8adf0bcb5 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -314,6 +314,20 @@ config EFI_COCO_SECRET
> virt/coco/efi_secret module to access the secrets, which in turn
> allows userspace programs to access the injected secrets.
>
> +config UNACCEPTED_MEMORY
> + bool
> + depends on EFI_STUB
This still doesn't make a whole lotta sense. If I do "make menuconfig" I don't
see the help text because that bool doesn't have a string prompt. So who is that
help text for?
Then, in the last patch you have
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -888,6 +888,8 @@ config INTEL_TDX_GUEST
select ARCH_HAS_CC_PLATFORM
select X86_MEM_ENCRYPT
select X86_MCE
+ select UNACCEPTED_MEMORY
+ select EFI_STUB
I guess you want to select UNACCEPTED_MEMORY only.
And I've already mentioned this whole mess:
https://lore.kernel.org/r/Yt%2BnOeLMqRxjObbx@xxxxxxx
Please incorporate all review comments before sending a new version of
your patch.
Ignoring review feedback is a very unfriendly thing to do:
- if you agree with the feedback, you work it in in the next revision
- if you don't agree, you *say* *why* you don't
> + 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 the kernel.
...
> +static efi_status_t allocate_unaccepted_bitmap(struct boot_params *params,
> + __u32 nr_desc,
> + struct efi_boot_memmap *map)
> +{
> + unsigned long *mem = NULL;
> + u64 size, max_addr = 0;
> + efi_status_t status;
> + bool found = false;
> + int i;
> +
> + /* Check if there's any unaccepted memory and find the max address */
> + for (i = 0; i < nr_desc; i++) {
> + efi_memory_desc_t *d;
> + unsigned long m = (unsigned long)map->map;
> +
> + d = efi_early_memdesc_ptr(m, map->desc_size, i);
> + if (d->type == EFI_UNACCEPTED_MEMORY)
> + found = true;
> + if (d->phys_addr + d->num_pages * PAGE_SIZE > max_addr)
> + max_addr = d->phys_addr + d->num_pages * PAGE_SIZE;
> + }
> +
> + if (!found) {
> + params->unaccepted_memory = 0;
> + return EFI_SUCCESS;
> + }
> +
> + /*
> + * 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 is already set.
> + * It's required to deal with kexec.
A TODO in a patch basically says this patch is not ready to go anywhere.
IOW, you need to handle that kexec case here gracefully. Even if you refuse to
boot a kexec-ed kernel because it cannot support handing in the bitmap from the
first kernel, yadda yadda...
> + *
> + * The bitmap will be populated in setup_e820() according to the memory
> + * map after efi_exit_boot_services().
> + */
> + size = DIV_ROUND_UP(max_addr, PMD_SIZE * BITS_PER_BYTE);
> + status = efi_allocate_pages(size, (unsigned long *)&mem, ULONG_MAX);
> + if (status == EFI_SUCCESS) {
> + memset(mem, 0, size);
> + params->unaccepted_memory = (unsigned long)mem;
> + }
> +
> + return status;
> +}
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
