From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lf1-f47.google.com (mail-lf1-f47.google.com [209.85.167.47]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 524F37A for ; Mon, 18 Apr 2022 15:54:12 +0000 (UTC) Received: by mail-lf1-f47.google.com with SMTP id x17so24779152lfa.10 for ; Mon, 18 Apr 2022 08:54:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=shutemov-name.20210112.gappssmtp.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=muYZw33hwSyG5ibVTGDvutUbYkYp8VVI0agZbsao0GQ=; b=ZUD8bIAuwFX0uDQkXwdfxBDO5xGtKspZjQm1+K6KPfJvlgGfThbVpuq4ptwKx70zFC er6LmktkWg8j/tiGOGeYGMnfS3L9McuEHJdOSj7Ni/LPYYLinXUYrOvSC4vagB/w6xiC KwEfmFvMB7e5Nw+3cTSKxkjvto5iaPcfgW/lGtqxobRlA0pi+gQqL+nOLabdR2BjuY2N rTonQad6QmVuRf6hiMwkKp6APv+pT0dGq4+eL7OpflUEZRPcmFHz4oLjEgt7HX7POkHs EmRnEg3fVs/J0SzIIU2rz+fRy4A2T33DZkS/JE9AYmcxfEvm67lU4xq6sBpKtQdL+QXR dpnw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=muYZw33hwSyG5ibVTGDvutUbYkYp8VVI0agZbsao0GQ=; b=SiUIl9yZrkjnpQxpBB+owKX+jMkLwPN3cqdEkiQg7naAyspjBQ4UgsTSvpYnBxoUeq KEx5akl1MPiM7UFHVBUWVpLJ2vzh5ueYj32CREqRhD66uvtt8nBrZG+6JauyCLM9RR8W IDA2fc+C3jKSzOYmdNUeP0lEuGhcpjrKa/x2RojpwtWcuJfD8sMz5rT3ij3dgbo2EA2M 1WLFDuQEF4qlQAx0eU9qhexntWkqMUJEoZRRrXscavYYPlOD7zMuiBRoUcyvWnH1BuEt IHxfXCAEPZwMGTl5tZC9hRzyia+tok7MZ0Yst8l8aF6OXhwmEjTL5nrcI+qAKrhSxrNV nCew== X-Gm-Message-State: AOAM530bqRkFcjTLDhnJTrFTjUIzGVgrLj5e1nRhZMwDC5pcp4BXK8XB pIMzB6TVIZXikTGQroNysBrQ1w== X-Google-Smtp-Source: ABdhPJwZ/jVosT/a9D2Ro5q2t3tBFRd55s7uIBzB7DTojjbaukvUm04gwiUU9s1IkvjceizKbDSUvw== X-Received: by 2002:a05:6512:3da8:b0:44a:73a4:3371 with SMTP id k40-20020a0565123da800b0044a73a43371mr8822829lfv.72.1650297250238; Mon, 18 Apr 2022 08:54:10 -0700 (PDT) Received: from box.localdomain ([86.57.175.117]) by smtp.gmail.com with ESMTPSA id t4-20020a2e7804000000b0024db2efb994sm740202ljc.39.2022.04.18.08.54.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 18 Apr 2022 08:54:09 -0700 (PDT) Received: by box.localdomain (Postfix, from userid 1000) id 0C014103A61; Mon, 18 Apr 2022 18:55:45 +0300 (+03) Date: Mon, 18 Apr 2022 18:55:45 +0300 From: "Kirill A. Shutemov" To: Borislav Petkov Cc: "Kirill A. Shutemov" , Andy Lutomirski , Sean Christopherson , Andrew Morton , Joerg Roedel , Ard Biesheuvel , Andi Kleen , Kuppuswamy Sathyanarayanan , David Rientjes , Vlastimil Babka , Tom Lendacky , Thomas Gleixner , Peter Zijlstra , Paolo Bonzini , Ingo Molnar , Varad Gautam , Dario Faggioli , Dave Hansen , Brijesh Singh , Mike Rapoport , David Hildenbrand , 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 Message-ID: <20220418155545.a567xnxa6elglapl@box.shutemov.name> References: <20220405234343.74045-1-kirill.shutemov@linux.intel.com> <20220405234343.74045-4-kirill.shutemov@linux.intel.com> Precedence: bulk X-Mailing-List: linux-coco@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 > > verify_include_paths: Warning: Kernel-proper include at arch/x86/boot/compressed/bitmap.c:4 [+#include ] > > 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 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 > 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 > + 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. 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