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.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED 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 76BC8C433E2 for ; Thu, 17 Sep 2020 11:17:55 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 0B36320770 for ; Thu, 17 Sep 2020 11:17:55 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=citrix.com header.i=@citrix.com header.b="JzD6D6NR" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0B36320770 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=citrix.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1kIruy-0003OE-N7; Thu, 17 Sep 2020 11:17:36 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1kIruy-0003O9-5x for xen-devel@lists.xenproject.org; Thu, 17 Sep 2020 11:17:36 +0000 X-Inumbo-ID: 7c6eb5cb-de2d-4315-ba45-c563e0840c4e Received: from esa1.hc3370-68.iphmx.com (unknown [216.71.145.142]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id 7c6eb5cb-de2d-4315-ba45-c563e0840c4e; Thu, 17 Sep 2020 11:17:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=citrix.com; s=securemail; t=1600341455; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=1sNczSkvYKyBI6al9cR8IYNBAQmFBabuTyrgk5HaxSA=; b=JzD6D6NRj8gK1Y7LrXR0V33ehheH/5jod+6wXUws3SGR1sDznRrlDMop gGfvTGkSp5+HppfxjuYoZpvKupil1tPx3aRx7sBuMFpgmvcFKln7vb/cr SWctMOB7WYiymVVkuwFjQnfeC8zMrLTVBIi2rO7CjVu4vOvT8mwbOM9BZ g=; Authentication-Results: esa1.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none IronPort-SDR: uAhjUlCX1yIBGRHTV8x0MUV/vDhRpjHp9ggIOeOHraQXEzC+Fd1IDfINddd2l0MYiPAbEQTpYF dZf6DTl31HkS4o2gM8EI3Ksx3xd1b75wi9+dWjF9kQ9iCrDPDxuRw4g8bcw/QEYNW9DuiFbU0B WUhR4AzRBbYuRNLTeK48dmWHh+coE03WZdrU6ItH3aMnTQbInQ3jzogLmgq8JLZrO1XdXNkgHY VtA5WkBaZap6eHp58BrxVYJOfKjvZX0lUXDgjmXJH+XTUa/1GJ+WJEnQtpdU3WiEVClO0fnV/P G+U= X-SBRS: 2.7 X-MesageID: 27252875 X-Ironport-Server: esa1.hc3370-68.iphmx.com X-Remote-IP: 162.221.158.21 X-Policy: $RELAYED X-IronPort-AV: E=Sophos;i="5.76,436,1592884800"; d="scan'208";a="27252875" Date: Thu, 17 Sep 2020 13:17:12 +0200 From: Roger Pau =?utf-8?B?TW9ubsOp?= To: Jan Beulich CC: "xen-devel@lists.xenproject.org" , "Andrew Cooper" , George Dunlap , Ian Jackson , "Julien Grall" , Wei Liu , Stefano Stabellini , Lukasz Hawrylko Subject: Re: [PATCH v4] EFI: free unused boot mem in at least some cases Message-ID: <20200917111712.GC19254@Air-de-Roger> References: <5dd2fcea-d8ec-1c20-6514-c7733b59047f@suse.com> <20200917104516.GB19254@Air-de-Roger> <37547ef8-7381-7a8e-b735-1633eab978a2@suse.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <37547ef8-7381-7a8e-b735-1633eab978a2@suse.com> X-ClientProxiedBy: AMSPEX02CAS01.citrite.net (10.69.22.112) To FTLPEX02CL06.citrite.net (10.13.108.179) X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" On Thu, Sep 17, 2020 at 12:56:41PM +0200, Jan Beulich wrote: > On 17.09.2020 12:45, Roger Pau Monné wrote: > > On Wed, Sep 16, 2020 at 02:20:54PM +0200, Jan Beulich wrote: > >> --- a/xen/arch/x86/efi/stub.c > >> +++ b/xen/arch/x86/efi/stub.c > >> @@ -52,6 +52,13 @@ bool efi_enabled(unsigned int feature) > >> > >> void __init efi_init_memory(void) { } > >> > >> +bool efi_boot_mem_unused(unsigned long *start, unsigned long *end) > >> +{ > >> + if ( start || end ) > > > > Shouldn't this be start && end? > > This is consistent with "if ( !start && !end )" in the non-stub > function, which was there in v3 already. Right, I certainly didn't catch that passing one as NULL would cause a deref there also. I would be more comfortable with adding an ASSERT, but I'm not going to insist. IIRC there was a time when Xen running as a PVH guest (like in shim mode) would cause it to have a valid mapping at 0. > > Or else you might be de-referencing a NULL pointer? > > Intentionally so: I'd view it as worse if we didn't fill *start > or *end if just one gets passed as NULL. The way it's done now > it'll be a reliable crash, as the v3 issue with the shim has > shown (where the if() here was missing). > > >> @@ -1417,8 +1419,18 @@ void __init noreturn __start_xen(unsigne > >> if ( !xen_phys_start ) > >> panic("Not enough memory to relocate Xen\n"); > >> > >> + /* FIXME: Putting a hole in .bss would shatter the large page mapping. */ > >> + if ( using_2M_mapping() ) > >> + efi_boot_mem_unused(NULL, NULL); > > > > This seems really weird IMO... > > What I didn't like about earlier versions was the exposure of > using_2M_mapping() outside of this CU. The way it works is > somewhat fragile, and hence I think limiting its exposure is a > win. This way there's also no x86-specific code in ebmalloc.c > anymore. > > I'm also slightly puzzled that you ask now when you had acked > this same construct in v3. It's really just the stub function > which has changed in v4. Would you mind also adding a FIXME comment in efi_boot_mem_unused that setting ebmalloc_allocated to sizeof(ebmalloc_mem) will be removed once we can properly free the region regardless of whether 2M are being used? Seems like an abuse of that the function should be doing by passing NULL pointers to it, or maybe I'm just being dense. With that: Acked-by: Roger Pau Monné Thanks.