From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Subject: Re: [PATCH v7 1/8] efi: Export boot-services code and data as debugfs-blobs Date: Wed, 9 Oct 2019 15:18:17 +0200 Message-ID: References: <20191004145056.43267-1-hdegoede@redhat.com> <20191004145056.43267-2-hdegoede@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Ard Biesheuvel Cc: Darren Hart , Andy Shevchenko , Luis Chamberlain , Greg Kroah-Hartman , "Rafael J . Wysocki" , Thomas Gleixner , Ingo Molnar , Borislav Petkov , "H . Peter Anvin" , Jonathan Corbet , Dmitry Torokhov , Peter Jones , Dave Olsthoorn , the arch/x86 maintainers , platform-driver-x86@vger.kernel.org, linux-efi , Linux Kernel Mailing List , Linux Doc Mailing List , linux-input@vger.kernel.org List-Id: linux-input@vger.kernel.org Hi, On 09-10-2019 15:07, Ard Biesheuvel wrote: > On Fri, 4 Oct 2019 at 16:51, Hans de Goede wrote: >> >> Sometimes it is useful to be able to dump the efi boot-services code and >> data. This commit adds these as debugfs-blobs to /sys/kernel/debug/efi, >> but only if efi=debug is passed on the kernel-commandline as this requires >> not freeing those memory-regions, which costs 20+ MB of RAM. >> >> Reviewed-by: Greg Kroah-Hartman >> Acked-by: Ard Biesheuvel >> Signed-off-by: Hans de Goede >> --- >> Changes in v5: >> -Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS >> >> Changes in v4: >> -Add new EFI_BOOT_SERVICES flag and use it to determine if the boot-services >> memory segments are available (and thus if it makes sense to register the >> debugfs bits for them) >> >> Changes in v2: >> -Do not call pr_err on debugfs call failures >> --- >> arch/x86/platform/efi/efi.c | 1 + >> arch/x86/platform/efi/quirks.c | 4 +++ >> drivers/firmware/efi/efi.c | 53 ++++++++++++++++++++++++++++++++++ >> include/linux/efi.h | 1 + >> 4 files changed, 59 insertions(+) >> >> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c >> index c202e1b07e29..847730f7e74b 100644 >> --- a/arch/x86/platform/efi/efi.c >> +++ b/arch/x86/platform/efi/efi.c >> @@ -232,6 +232,7 @@ int __init efi_memblock_x86_reserve_range(void) >> efi.memmap.desc_version); >> >> memblock_reserve(pmap, efi.memmap.nr_map * efi.memmap.desc_size); >> + set_bit(EFI_PRESERVE_BS_REGIONS, &efi.flags); > > Should we add a Kconfig symbol to opt into this behavior [set by the > driver in question], instead of always preserving all boot services > regions on all x86 systems? This bit does not control anything, it merely signals that the arch early boot EFI code keeps the boot-services code around, which is something which the x86 code already does. Where as e.g. on arm / aarch64 this is freed early on, this ties in with the other bits: > >> >> return 0; >> } >> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c >> index 3b9fd679cea9..fab12ebf0ada 100644 >> --- a/arch/x86/platform/efi/quirks.c >> +++ b/arch/x86/platform/efi/quirks.c >> @@ -411,6 +411,10 @@ void __init efi_free_boot_services(void) >> int num_entries = 0; >> void *new, *new_md; >> >> + /* Keep all regions for /sys/kernel/debug/efi */ >> + if (efi_enabled(EFI_DBG)) >> + return; >> + This is the point where normally on x86 we do actually free the boot-services code which is a lot later then on other arches. And this new code actually does change things to keep the boot-services code *forever* but only if EFI debugging is enabled on the kernel commandline. This ties in with the next bit: >> for_each_efi_memory_desc(md) { >> unsigned long long start = md->phys_addr; >> unsigned long long size = md->num_pages << EFI_PAGE_SHIFT; >> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c >> index 8d3e778e988b..abba49c4c46d 100644 >> --- a/drivers/firmware/efi/efi.c >> +++ b/drivers/firmware/efi/efi.c >> @@ -370,6 +420,9 @@ static int __init efisubsys_init(void) >> goto err_remove_group; >> } >> >> + if (efi_enabled(EFI_DBG) && efi_enabled(EFI_PRESERVE_BS_REGIONS)) >> + efi_debugfs_init(); >> + >> return 0; >> >> err_remove_group: Here we register the debugfs dir + files, but only when the boot services code has been kept around, so only if the EFI_PRESERVE_BS_REGIONS arch feature flag has been set and EFI debugging has been requested on the kernel commandline. IOW this patch already offers to configurability you ask for, but instead of through a Kconfig option (which IMHO would be cumbersome) the decision is made runtime based on the presence of efi=debug on the kernel commandline. Regards, Hans >> diff --git a/include/linux/efi.h b/include/linux/efi.h >> index bd3837022307..2a30a1bd8bdf 100644 >> --- a/include/linux/efi.h >> +++ b/include/linux/efi.h >> @@ -1202,6 +1202,7 @@ extern int __init efi_setup_pcdp_console(char *); >> #define EFI_DBG 8 /* Print additional debug info at runtime */ >> #define EFI_NX_PE_DATA 9 /* Can runtime data regions be mapped non-executable? */ >> #define EFI_MEM_ATTR 10 /* Did firmware publish an EFI_MEMORY_ATTRIBUTES table? */ >> +#define EFI_PRESERVE_BS_REGIONS 11 /* Are EFI boot-services memory segments available? */ >> >> #ifdef CONFIG_EFI >> /* >> -- >> 2.23.0 >> 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=-8.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable 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 0F3E1ECE58D for ; Wed, 9 Oct 2019 13:18:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id D9AA5206B6 for ; Wed, 9 Oct 2019 13:18:32 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731005AbfJINSc (ORCPT ); Wed, 9 Oct 2019 09:18:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50848 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731300AbfJINSW (ORCPT ); Wed, 9 Oct 2019 09:18:22 -0400 Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C26BBC010923 for ; Wed, 9 Oct 2019 13:18:21 +0000 (UTC) Received: by mail-wr1-f71.google.com with SMTP id m14so1101372wru.17 for ; Wed, 09 Oct 2019 06:18:21 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=QSGtDck56uyZ7bQZy6YNbrPNRWeC06OlNw7fuz6920g=; b=iGLYTTMZfJn2ZjZScPMBvKM9iULGew4kCX+sjKISNbJzmBKc5PPSdQ1ungjd36BKOZ xRdRKZTTf53iPC9RVqASWLdk6M6r+p2goqNSmMSHJ5PBteo9vb80u6ZTx/WylSyHVb/6 qLCrIjbgGnikAt0tUfzpg2oe6EWg+nKd+BhvRGX0K3yMEuteh2CiJ6E2/a3QXYbhyDRF DCtfMsHmSIdP0AcDeuck9/q+LAVMJ/82SQAFwAM46wBMKthB0GKzUW++RP9BrnhAF1BI FSQXqGyDE6dGlpoUaYtVuZJciSnRoENae9vwxtKtULtKLPFfNlItYzQ64E3nb0zznk9+ jjgg== X-Gm-Message-State: APjAAAUGyNEAgjJBAbwz53y/C9KCKgBXuD3n/kur6gSeYsLo/TmPSZbn ZrVe31RK4HywZDz+3GEZjQpLONtxmE3ZKenXFEIybudpDjoCVPs37LNwbssUEqHaCrM7jrQX8q0 GPahK0xvq8beak9AvAkpRt1E= X-Received: by 2002:a5d:654a:: with SMTP id z10mr2870825wrv.159.1570627100021; Wed, 09 Oct 2019 06:18:20 -0700 (PDT) X-Google-Smtp-Source: APXvYqyEhsE6YKwEHTEE/HZcg+u2Ek0JtIlYFKf+FQD7oObjzI3Nse2MJ3NOgl7JFiN+/rZpZf7MuA== X-Received: by 2002:a5d:654a:: with SMTP id z10mr2870794wrv.159.1570627099787; Wed, 09 Oct 2019 06:18:19 -0700 (PDT) Received: from shalem.localdomain (2001-1c00-0c14-2800-ec23-a060-24d5-2453.cable.dynamic.v6.ziggo.nl. [2001:1c00:c14:2800:ec23:a060:24d5:2453]) by smtp.gmail.com with ESMTPSA id v6sm4176570wma.24.2019.10.09.06.18.18 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 09 Oct 2019 06:18:18 -0700 (PDT) Subject: Re: [PATCH v7 1/8] efi: Export boot-services code and data as debugfs-blobs To: Ard Biesheuvel Cc: Darren Hart , Andy Shevchenko , Luis Chamberlain , Greg Kroah-Hartman , "Rafael J . Wysocki" , Thomas Gleixner , Ingo Molnar , Borislav Petkov , "H . Peter Anvin" , Jonathan Corbet , Dmitry Torokhov , Peter Jones , Dave Olsthoorn , the arch/x86 maintainers , platform-driver-x86@vger.kernel.org, linux-efi , Linux Kernel Mailing List , Linux Doc Mailing List , linux-input@vger.kernel.org References: <20191004145056.43267-1-hdegoede@redhat.com> <20191004145056.43267-2-hdegoede@redhat.com> From: Hans de Goede Message-ID: Date: Wed, 9 Oct 2019 15:18:17 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-input-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org Message-ID: <20191009131817.2OoD3orJMvUc4OSP25xKCHr67UoUxpG0d1VvNIPI9lc@z> Hi, On 09-10-2019 15:07, Ard Biesheuvel wrote: > On Fri, 4 Oct 2019 at 16:51, Hans de Goede wrote: >> >> Sometimes it is useful to be able to dump the efi boot-services code and >> data. This commit adds these as debugfs-blobs to /sys/kernel/debug/efi, >> but only if efi=debug is passed on the kernel-commandline as this requires >> not freeing those memory-regions, which costs 20+ MB of RAM. >> >> Reviewed-by: Greg Kroah-Hartman >> Acked-by: Ard Biesheuvel >> Signed-off-by: Hans de Goede >> --- >> Changes in v5: >> -Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS >> >> Changes in v4: >> -Add new EFI_BOOT_SERVICES flag and use it to determine if the boot-services >> memory segments are available (and thus if it makes sense to register the >> debugfs bits for them) >> >> Changes in v2: >> -Do not call pr_err on debugfs call failures >> --- >> arch/x86/platform/efi/efi.c | 1 + >> arch/x86/platform/efi/quirks.c | 4 +++ >> drivers/firmware/efi/efi.c | 53 ++++++++++++++++++++++++++++++++++ >> include/linux/efi.h | 1 + >> 4 files changed, 59 insertions(+) >> >> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c >> index c202e1b07e29..847730f7e74b 100644 >> --- a/arch/x86/platform/efi/efi.c >> +++ b/arch/x86/platform/efi/efi.c >> @@ -232,6 +232,7 @@ int __init efi_memblock_x86_reserve_range(void) >> efi.memmap.desc_version); >> >> memblock_reserve(pmap, efi.memmap.nr_map * efi.memmap.desc_size); >> + set_bit(EFI_PRESERVE_BS_REGIONS, &efi.flags); > > Should we add a Kconfig symbol to opt into this behavior [set by the > driver in question], instead of always preserving all boot services > regions on all x86 systems? This bit does not control anything, it merely signals that the arch early boot EFI code keeps the boot-services code around, which is something which the x86 code already does. Where as e.g. on arm / aarch64 this is freed early on, this ties in with the other bits: > >> >> return 0; >> } >> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c >> index 3b9fd679cea9..fab12ebf0ada 100644 >> --- a/arch/x86/platform/efi/quirks.c >> +++ b/arch/x86/platform/efi/quirks.c >> @@ -411,6 +411,10 @@ void __init efi_free_boot_services(void) >> int num_entries = 0; >> void *new, *new_md; >> >> + /* Keep all regions for /sys/kernel/debug/efi */ >> + if (efi_enabled(EFI_DBG)) >> + return; >> + This is the point where normally on x86 we do actually free the boot-services code which is a lot later then on other arches. And this new code actually does change things to keep the boot-services code *forever* but only if EFI debugging is enabled on the kernel commandline. This ties in with the next bit: >> for_each_efi_memory_desc(md) { >> unsigned long long start = md->phys_addr; >> unsigned long long size = md->num_pages << EFI_PAGE_SHIFT; >> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c >> index 8d3e778e988b..abba49c4c46d 100644 >> --- a/drivers/firmware/efi/efi.c >> +++ b/drivers/firmware/efi/efi.c >> @@ -370,6 +420,9 @@ static int __init efisubsys_init(void) >> goto err_remove_group; >> } >> >> + if (efi_enabled(EFI_DBG) && efi_enabled(EFI_PRESERVE_BS_REGIONS)) >> + efi_debugfs_init(); >> + >> return 0; >> >> err_remove_group: Here we register the debugfs dir + files, but only when the boot services code has been kept around, so only if the EFI_PRESERVE_BS_REGIONS arch feature flag has been set and EFI debugging has been requested on the kernel commandline. IOW this patch already offers to configurability you ask for, but instead of through a Kconfig option (which IMHO would be cumbersome) the decision is made runtime based on the presence of efi=debug on the kernel commandline. Regards, Hans >> diff --git a/include/linux/efi.h b/include/linux/efi.h >> index bd3837022307..2a30a1bd8bdf 100644 >> --- a/include/linux/efi.h >> +++ b/include/linux/efi.h >> @@ -1202,6 +1202,7 @@ extern int __init efi_setup_pcdp_console(char *); >> #define EFI_DBG 8 /* Print additional debug info at runtime */ >> #define EFI_NX_PE_DATA 9 /* Can runtime data regions be mapped non-executable? */ >> #define EFI_MEM_ATTR 10 /* Did firmware publish an EFI_MEMORY_ATTRIBUTES table? */ >> +#define EFI_PRESERVE_BS_REGIONS 11 /* Are EFI boot-services memory segments available? */ >> >> #ifdef CONFIG_EFI >> /* >> -- >> 2.23.0 >>