From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 753DEC4338F for ; Thu, 12 Aug 2021 21:14:40 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id E5CB961038 for ; Thu, 12 Aug 2021 21:14:39 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org E5CB961038 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=shutemov.name Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 3C6036B006C; Thu, 12 Aug 2021 17:14:39 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 376516B0071; Thu, 12 Aug 2021 17:14:39 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2654F8D0001; Thu, 12 Aug 2021 17:14:39 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0201.hostedemail.com [216.40.44.201]) by kanga.kvack.org (Postfix) with ESMTP id 0D94C6B006C for ; Thu, 12 Aug 2021 17:14:39 -0400 (EDT) Received: from smtpin22.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id A2322183459F6 for ; Thu, 12 Aug 2021 21:14:38 +0000 (UTC) X-FDA: 78467682636.22.E070C7B Received: from mail-lf1-f50.google.com (mail-lf1-f50.google.com [209.85.167.50]) by imf06.hostedemail.com (Postfix) with ESMTP id 6504C8022764 for ; Thu, 12 Aug 2021 21:14:38 +0000 (UTC) Received: by mail-lf1-f50.google.com with SMTP id w20so15952377lfu.7 for ; Thu, 12 Aug 2021 14:14:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=shutemov-name.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=O9Ui4Q6PbF0T/rKVbRolnp389K3oQYeJwugtlRVK+Bo=; b=nt7t6noInmS46fO73mQssKdC9yvBRGkXwZCI0IBinN70xxUtKdg7mZT6iZmKZWQQmq EL1PYad5hI1ZbyEBEWTfZdmrWe/N5mQ4U3zNUL4lfDFG1bVGVg2MsK31R4mZJzCrczMN Mk+dfziQ1XP8urPfCtCmbdrQyXqDNzdXR+CmKp2JshpW92JEtRU9AO0NiA9uyCGuQe4L uZpC1EeRbCURCLiHHRDc40Y8mrlfJ3vz/+LG6YalsIN0TFaDWib7qVaL9+ZB45PcyB35 /LO0Cdy5YA6xyLSNYTTMle5uJtufletrPU3xEr5wJnRRwAdBCDw8OA02yiK+m5Hfq5K4 maOw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=O9Ui4Q6PbF0T/rKVbRolnp389K3oQYeJwugtlRVK+Bo=; b=O0tHtchj3jUm5osDIGsu1iI6Ur2stZ1KeDEBWUrOp+ESUE5qwZ8TG10AP5Zvy5TOUo Yov62oSkqb/GJVBowc7Apleq6yD6F/OYjLlktIs5fgMDhDPe/v330tiUkoL7eZs0Rbq/ BSdb2Ql5JeHf93zCPXu8YMqs/b35f/bGjmhWTBjhoBjOJkd0pw1jnMsUpbe8MdRXuurT FTNIJKlORnLaQvvMC74mUW0jsAPThbQLysOyQBB3Db8tT4ywq8vRBPHDEC036INSrIyu Z94l10d9zaH23XDAfo8w2s8B9sbPVFQ3AdBIX6TJLypq25tny3esRMg2i+jWOhE9bUCf jv7A== X-Gm-Message-State: AOAM5320/wXTjieo3tO1oZMy+Td6ETpnQpYPKLYyhwUNwfO8nOirKKrR jzGBjBLMyxEbcptBPvLpudDhKw== X-Google-Smtp-Source: ABdhPJxCnDou6x8BHSKfwHgrInMCW0EOGw+eNpa6BoJHZhlZVjwD62MZzkIh4Yo37zkF5U4ACBw85Q== X-Received: by 2002:ac2:5fcf:: with SMTP id q15mr3904047lfg.597.1628802876962; Thu, 12 Aug 2021 14:14:36 -0700 (PDT) Received: from box.localdomain ([86.57.175.117]) by smtp.gmail.com with ESMTPSA id f16sm371871lfv.115.2021.08.12.14.14.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 12 Aug 2021 14:14:36 -0700 (PDT) Received: by box.localdomain (Postfix, from userid 1000) id E31FB102BEE; Fri, 13 Aug 2021 00:14:49 +0300 (+03) Date: Fri, 13 Aug 2021 00:14:49 +0300 From: "Kirill A. Shutemov" To: Dave Hansen Cc: Borislav Petkov , Andy Lutomirski , Sean Christopherson , Andrew Morton , Joerg Roedel , Andi Kleen , Kuppuswamy Sathyanarayanan , David Rientjes , Vlastimil Babka , Tom Lendacky , Thomas Gleixner , Peter Zijlstra , Paolo Bonzini , Ingo Molnar , Varad Gautam , Dario Faggioli , x86@kernel.org, linux-mm@kvack.org, linux-coco@lists.linux.dev, linux-kernel@vger.kernel.org, "Kirill A. Shutemov" Subject: Re: [PATCH 2/5] efi/x86: Implement support for unaccepted memory Message-ID: <20210812211449.5bsblj6lphtu7zsd@box.shutemov.name> References: <20210810062626.1012-1-kirill.shutemov@linux.intel.com> <20210810062626.1012-3-kirill.shutemov@linux.intel.com> <142bccc6-0e67-dfc1-9069-b773c2bad585@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <142bccc6-0e67-dfc1-9069-b773c2bad585@intel.com> Authentication-Results: imf06.hostedemail.com; dkim=pass header.d=shutemov-name.20150623.gappssmtp.com header.s=20150623 header.b=nt7t6noI; spf=none (imf06.hostedemail.com: domain of kirill@shutemov.name has no SPF policy when checking 209.85.167.50) smtp.mailfrom=kirill@shutemov.name; dmarc=none X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 6504C8022764 X-Stat-Signature: 913aimkfw91wat3n7knjqh77zcgjdogc X-HE-Tag: 1628802878-999722 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, Aug 10, 2021 at 10:50:33AM -0700, Dave Hansen wrote: > ... > > +void mark_unaccepted(struct boot_params *params, u64 start, u64 num) > > +{ > > Some of these interfaces like accept_memory() take a start/end physical > address. Having this take a "num pages" is bound to cause confusion. > Could you make these all consistently take start/end physical addresses? Okay. > > > + u64 end = start + num * PAGE_SIZE; > > + unsigned int npages; > > > Could you comment those, please? > > /* > * 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. > */ > > Then go on to comment the three cases: > > /* Check for ranges which do not span a whole PMD_SIZE area: */ Okay. > > + if ((start & PMD_MASK) == (end & PMD_MASK)) { > > + npages = (end - start) / PAGE_SIZE; > > + __accept_memory(start, start + npages * PAGE_SIZE); > > + return; > > + } > > Hmm, is it possible to have this case hit, but neither of the two below > cases? This seems to be looking for a case where the range is somehow > entirely contained in one PMD_SIZE area, but where it doesn't consume a > whole area. > > Wouldn't that mean that 'start' or 'end' must be unaligned? The problem is that if both of them unaligned round_up() and round_down() in the cases below would step outside the requested range. > > + if (start & ~PMD_MASK) { > > + npages = (round_up(start, PMD_SIZE) - start) / PAGE_SIZE; > > + __accept_memory(start, start + npages * PAGE_SIZE); > > + start = round_up(start, PMD_SIZE); > > + } > > + > > + if (end & ~PMD_MASK) { > > + npages = (end - round_down(end, PMD_SIZE)) / PAGE_SIZE; > > + end = round_down(end, PMD_SIZE); > > + __accept_memory(end, end + npages * PAGE_SIZE); > > + } > > + npages = (end - start) / PMD_SIZE; > > + bitmap_set((unsigned long *)params->unaccepted_memory, > > + start / PMD_SIZE, npages); > > +} > > Even though it's changed right there, it's a bit cruel to change the > units of 'npages' right in the middle of a function. It's just asking > for bugs. > > It would only take a single extra variable declaration to make this > unambiguous: > > u64 nr_unaccepted_bits; > > or something, then you can do: > > nr_unaccepted_bits = (end - start) / PMD_SIZE; > bitmap_set((unsigned long *)params->unaccepted_memory, > start / PMD_SIZE, nr_unaccepted_bits); Okay. > > ... > > static efi_status_t allocate_e820(struct boot_params *params, > > + struct efi_boot_memmap *map, > > struct setup_data **e820ext, > > u32 *e820ext_size) > > { > > - unsigned long map_size, desc_size, map_key; > > efi_status_t status; > > - __u32 nr_desc, desc_version; > > - > > - /* Only need the size of the mem map and size of each mem descriptor */ > > - map_size = 0; > > - status = efi_bs_call(get_memory_map, &map_size, NULL, &map_key, > > - &desc_size, &desc_version); > > - if (status != EFI_BUFFER_TOO_SMALL) > > - return (status != EFI_SUCCESS) ? status : EFI_UNSUPPORTED; > > I noticed that there's no reference to EFI_BUFFER_TOO_SMALL in the hunks > you added back. That makes me a bit nervous that this is going to > unintentionally change behavior. > > It might be worth having a preparatory reorganization patch for > allocate_e820() before this new feature is added to make this more clear. Okay. Will do. > > > + __u32 nr_desc; > > + bool unaccepted_memory_present = false; > > + u64 max_addr = 0; > > + int i; > > > > - nr_desc = map_size / desc_size + EFI_MMAP_NR_SLACK_SLOTS; > > + status = efi_get_memory_map(map); > > + if (status != EFI_SUCCESS) > > + return status; > > > > - if (nr_desc > ARRAY_SIZE(params->e820_table)) { > > - u32 nr_e820ext = nr_desc - ARRAY_SIZE(params->e820_table); > > + nr_desc = *map->map_size / *map->desc_size; > > + if (nr_desc > ARRAY_SIZE(params->e820_table) - EFI_MMAP_NR_SLACK_SLOTS) { > > + u32 nr_e820ext = nr_desc - ARRAY_SIZE(params->e820_table) - > > + EFI_MMAP_NR_SLACK_SLOTS; > > > > status = alloc_e820ext(nr_e820ext, e820ext, e820ext_size); > > if (status != EFI_SUCCESS) > > return status; > > } > > > > + if (!IS_ENABLED(CONFIG_UNACCEPTED_MEMORY)) > > + return EFI_SUCCESS; > > + > > + /* 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; > > + } > > This 'max_addr' variable looks a bit funky. > > It *seems* like it's related only to EFI_UNACCEPTED_MEMORY, but it's not > underneath the EFI_UNACCEPTED_MEMORY check. Is this somehow assuming > that once unaccepted memory as been found that *all* memory found in > later descriptors at higher addresses is also going to be unaccepted? You got it right below :P > > + /* > > + * If unaccepted memory 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: 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 -- we would need 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. > > + */ > > + if (unaccepted_memory_present) { > > + unsigned long *unaccepted_memory = NULL; > > + u64 size = DIV_ROUND_UP(max_addr, PMD_SIZE * BITS_PER_BYTE); > > Oh, so the bitmap has to be present for *all* memory, not just > unaccepted memory. So, we really do need to know the 'max_addr' so that > we can allocate the bitmap for so that can be marked in the bitmap has > having been accepted. Right we need a bit for every 2M. Accepted or not. > > + status = efi_allocate_pages(size, > > + (unsigned long *)&unaccepted_memory, > > + ULONG_MAX); > > + if (status != EFI_SUCCESS) > > + return status; > > + memset(unaccepted_memory, 0, size); > > + params->unaccepted_memory = (u64)unaccepted_memory; > > + } > > It might be nice to refer to setup_e820() here to mention that it is the > thing that actually fills out the bitmap. Okay. -- Kirill A. Shutemov