Message ID | 20230625140931.1266216-2-songshuaishuai@tinylab.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp6939516vqr; Sun, 25 Jun 2023 07:18:18 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ691i5qAWb1wh/5fOfDWQKIyPeKaDoF/B6wDmQkOB5e1zhtHISRQoSniPWKebST96l5dFAH X-Received: by 2002:a05:6a00:17a6:b0:676:ad06:29d7 with SMTP id s38-20020a056a0017a600b00676ad0629d7mr1272909pfg.15.1687702698330; Sun, 25 Jun 2023 07:18:18 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1687702698; cv=none; d=google.com; s=arc-20160816; b=onhrr8nidMg6AWv0LUt62lurA8X2glFJlPtBDXtRE/3okZeQA6Y1E23Ch9KLQhZpJH DGcQSU4W6vZBDAOmUWhiOOC8kuFeZ8S7JWczS/Qoi5J3uvyxpRWTseyBbMw/M3mByxTq /wxdD90H6npvd7Lb0UArWa7iMqpf9chpcxGZudK4/C1tdmASqbg59ZhErbEbA6hgBILR HWOBA0DWfdv/ZMx5UEj/B40X3cCbW9xLTAUDo5JDjWUxGIdjdXB80Lz5qJs+DScA9mn7 I7amhB4SH/xe5zbMnqm3oSjuIALG3Gc+0mqPKNiYd/KAkGurwzRDCjIzrTOrEMvdmcyN /4sQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:feedback-id:content-transfer-encoding :mime-version:references:in-reply-to:message-id:date:subject:cc:to :from; bh=aO4bL7wh8dIgxc1UXdk85xO6Ne+sPAuUMDbTWR3f9NQ=; fh=2/EbjHRE7EKafzAkxWdKfWLNh6seQ7/0UQ7OzUYTgxI=; b=iqEOOLlFyrPRVMM/LrYwcvH2ff1xiS6KhevgZ1ir8WJDwAKwv5sNMHOV4BXuMLXpc1 oplddCTWJqu+PADbT/78Av3bQucjiPsto2axDp0TR4Fct+pdsMGHf6VtgwxRDaiCKnYb uLkxg3X8kK6Uz3dYv7UFCPpdGNjk3uT6xdnfB2FoXLCib598+o9G5Wb/ez5zCx8ji9wQ ThiEEfoCNkzj8yeU5GsreWX50wAfOaskf40hhetg5OESr74tukZgKAKLHTVm6q39Z5S1 LsGYoQ65dVIwWc72u3YIo0jeNWAE+ji0TJrfVR04z/MF/uh4HpE39WlfePtBzhkLvCrc bYkQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id br6-20020a056a00440600b006686ec2f62esi3151241pfb.187.2023.06.25.07.18.05; Sun, 25 Jun 2023 07:18:18 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230026AbjFYOLA (ORCPT <rfc822;duw91626@gmail.com> + 99 others); Sun, 25 Jun 2023 10:11:00 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47694 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229964AbjFYOKz (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sun, 25 Jun 2023 10:10:55 -0400 Received: from bg4.exmail.qq.com (bg4.exmail.qq.com [43.154.54.12]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9AB7B1B1 for <linux-kernel@vger.kernel.org>; Sun, 25 Jun 2023 07:10:53 -0700 (PDT) X-QQ-mid: bizesmtp73t1687702208t869bq9s Received: from localhost.localdomain ( [112.2.230.41]) by bizesmtp.qq.com (ESMTP) with id ; Sun, 25 Jun 2023 22:10:04 +0800 (CST) X-QQ-SSF: 01200000000000B0B000000A0000000 X-QQ-FEAT: TVZM0Uoyj00c0bZeOtQsMG0DaWKF8k5IP29sSaNKu2IqHl0SZUO1pXN9aQB5m So+nB8VGz5FuKu2vXt+QJgjvfdapKB5QFP/nycPWDjuq5n+govmjlnnfC3eg5zAOuPs3RLJ tidw5jfJcMk/f3odNds/Y35dsaNOgrDwb/1JqzDV+lozOChS18Qcsa48sKJp4BW5GTiQIy+ uENrBlNboO9pUCSceB6gdweAS2JHCzmhrhNjhlZNzbmvK9cEFaJe9JopuWLI/HG/hHN1hbp YC5V/dL5eDx0XIYB3rpFxJsYuPt+2YzAPJHuZUvik2ApM8SgDnrqQS532EawfgLqH9EsUe7 JIjbNrfBR84Y/3K7AbHzIySaufKumCejpKOrEBPytWTAKTadhs= X-QQ-GoodBg: 0 X-BIZMAIL-ID: 11597191393074232432 From: Song Shuai <songshuaishuai@tinylab.org> To: paul.walmsley@sifive.com, palmer@dabbelt.com, aou@eecs.berkeley.edu, robh+dt@kernel.org, frowand.list@gmail.com, ajones@ventanamicro.com, alexghiti@rivosinc.com, mpe@ellerman.id.au, arnd@arndb.de, songshuaishuai@tinylab.org, rppt@kernel.org, samuel@sholland.org, panqinglin2020@iscas.ac.cn, conor.dooley@microchip.com, anup@brainfault.org, xianting.tian@linux.alibaba.com, anshuman.khandual@arm.com, heiko@sntech.de Cc: linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org Subject: [PATCH V1 1/3] Revert "RISC-V: mark hibernation as nonportable" Date: Sun, 25 Jun 2023 22:09:29 +0800 Message-Id: <20230625140931.1266216-2-songshuaishuai@tinylab.org> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20230625140931.1266216-1-songshuaishuai@tinylab.org> References: <20230625140931.1266216-1-songshuaishuai@tinylab.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-QQ-SENDSIZE: 520 Feedback-ID: bizesmtp:tinylab.org:qybglogicsvrgz:qybglogicsvrgz5a-3 X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1769684544467580743?= X-GMAIL-MSGID: =?utf-8?q?1769684544467580743?= |
Series |
Revert huge-paged linear mapping and its related fixups
|
|
Commit Message
Song Shuai
June 25, 2023, 2:09 p.m. UTC
This reverts commit ed309ce522185583b163bd0c74f0d9f299fe1826.
With the commit 3335068f8721 ("riscv: Use PUD/P4D/PGD pages for the
linear mapping") reverted, the MIN_MEMBLOCK_ADDR points the kernel
load address which was placed at a PMD boundary. And firmware always
correctly mark resident memory, or memory protected with PMP as
per the devicetree specification and/or the UEFI specification.
So those regions will not be mapped in the linear mapping and they
can be safely saved/restored by hibernation.
Signed-off-by: Song Shuai <songshuaishuai@tinylab.org>
---
arch/riscv/Kconfig | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
Comments
On Sun, Jun 25, 2023 at 10:09:29PM +0800, Song Shuai wrote: > This reverts commit ed309ce522185583b163bd0c74f0d9f299fe1826. > > With the commit 3335068f8721 ("riscv: Use PUD/P4D/PGD pages for the > linear mapping") reverted, the MIN_MEMBLOCK_ADDR points the kernel > load address which was placed at a PMD boundary. > And firmware always > correctly mark resident memory, or memory protected with PMP as > per the devicetree specification and/or the UEFI specification. But this is not true? The versions of OpenSBI that you mention in your cover letter do not do this. Please explain. > So those regions will not be mapped in the linear mapping and they > can be safely saved/restored by hibernation. > > Signed-off-by: Song Shuai <songshuaishuai@tinylab.org> > --- > arch/riscv/Kconfig | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > index 5966ad97c30c..17b5fc7f54d4 100644 > --- a/arch/riscv/Kconfig > +++ b/arch/riscv/Kconfig > @@ -800,11 +800,8 @@ menu "Power management options" > > source "kernel/power/Kconfig" > > -# Hibernation is only possible on systems where the SBI implementation has > -# marked its reserved memory as not accessible from, or does not run > -# from the same memory as, Linux > config ARCH_HIBERNATION_POSSIBLE > - def_bool NONPORTABLE > + def_bool y > > config ARCH_HIBERNATION_HEADER > def_bool HIBERNATION > -- > 2.20.1 >
Sorry for the delayed reply, My tinylab email went something wrong, I'll use gmail in this thread. 在 2023/6/25 22:18, Conor Dooley 写道: > On Sun, Jun 25, 2023 at 10:09:29PM +0800, Song Shuai wrote: >> This reverts commit ed309ce522185583b163bd0c74f0d9f299fe1826. >> >> With the commit 3335068f8721 ("riscv: Use PUD/P4D/PGD pages for the >> linear mapping") reverted, the MIN_MEMBLOCK_ADDR points the kernel >> load address which was placed at a PMD boundary. > >> And firmware always >> correctly mark resident memory, or memory protected with PMP as >> per the devicetree specification and/or the UEFI specification. > > But this is not true? The versions of OpenSBI that you mention in your > cover letter do not do this. > Please explain. > At this time, OpenSbi [v0.8,v1.3) and edk2(RiscVVirt) indeed don't obey the DT/UEFI spec. This statement is excerpted from "Reserved memory for resident firmware" part from the upcoming riscv/boot.rst. It isn't accurate for now. How about deleting this one? Actually with 3335068f8721 reverted, the change of MIN_MEMBLOCK_ADDR can avoid the mapping of firmware memory, I will make it clear in the next version. >> So those regions will not be mapped in the linear mapping and they >> can be safely saved/restored by hibernation. >> >> Signed-off-by: Song Shuai <songshuaishuai@tinylab.org> >> --- >> arch/riscv/Kconfig | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig >> index 5966ad97c30c..17b5fc7f54d4 100644 >> --- a/arch/riscv/Kconfig >> +++ b/arch/riscv/Kconfig >> @@ -800,11 +800,8 @@ menu "Power management options" >> >> source "kernel/power/Kconfig" >> >> -# Hibernation is only possible on systems where the SBI implementation has >> -# marked its reserved memory as not accessible from, or does not run >> -# from the same memory as, Linux >> config ARCH_HIBERNATION_POSSIBLE >> - def_bool NONPORTABLE >> + def_bool y >> >> config ARCH_HIBERNATION_HEADER >> def_bool HIBERNATION >> -- >> 2.20.1 >>
Hey, On Sun, Jun 25, 2023 at 11:09:21PM +0800, Song Shuai wrote: > Sorry for the delayed reply, It wasn't really delayed at all actually, you replied within an hour or so, AFAICT. > My tinylab email went something wrong, I'll use gmail in this thread. > > 在 2023/6/25 22:18, Conor Dooley 写道: > > On Sun, Jun 25, 2023 at 10:09:29PM +0800, Song Shuai wrote: > > > This reverts commit ed309ce522185583b163bd0c74f0d9f299fe1826. > > > > > > With the commit 3335068f8721 ("riscv: Use PUD/P4D/PGD pages for the > > > linear mapping") reverted, the MIN_MEMBLOCK_ADDR points the kernel > > > load address which was placed at a PMD boundary. > > > > > And firmware always > > > correctly mark resident memory, or memory protected with PMP as > > > per the devicetree specification and/or the UEFI specification. > > > > But this is not true? The versions of OpenSBI that you mention in your > > cover letter do not do this. > > Please explain. > > > > At this time, OpenSbi [v0.8,v1.3) and edk2(RiscVVirt) indeed don't obey the > DT/UEFI spec. This statement is excerpted from "Reserved memory for resident > firmware" part from the upcoming riscv/boot.rst. It isn't accurate for now. > How about deleting this one? It is incorrect, so it will need to be removed, yes. Unfortunately writing a doc does not fix the existing implementations :( > Actually with 3335068f8721 reverted, the change of MIN_MEMBLOCK_ADDR can > avoid the mapping of firmware memory, I will make it clear in the next > version. To be honest, I'd like to see this revert as the final commit in a series that deals with the problem by actually reserving the regions, rather than a set of reverts that go back to how we were. I was hoping that someone who cares about hibernation support would be interested in working on that - *cough* starfive *cough*, although maybe they just fixed their OpenSBI and moved on. If there were no volunteers, my intention was to add a firmware erratum that would probe the SBI implementation & version IDs, and add a firmware erratum that'd parse the DT for the offending regions and reserve them. Cheers, Conor. > > > So those regions will not be mapped in the linear mapping and they > > > can be safely saved/restored by hibernation. > > > > > > Signed-off-by: Song Shuai <songshuaishuai@tinylab.org> > > > --- > > > arch/riscv/Kconfig | 5 +---- > > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > > > index 5966ad97c30c..17b5fc7f54d4 100644 > > > --- a/arch/riscv/Kconfig > > > +++ b/arch/riscv/Kconfig > > > @@ -800,11 +800,8 @@ menu "Power management options" > > > source "kernel/power/Kconfig" > > > -# Hibernation is only possible on systems where the SBI implementation has > > > -# marked its reserved memory as not accessible from, or does not run > > > -# from the same memory as, Linux > > > config ARCH_HIBERNATION_POSSIBLE > > > - def_bool NONPORTABLE > > > + def_bool y > > > config ARCH_HIBERNATION_HEADER > > > def_bool HIBERNATION > > > -- > > > 2.20.1 >
On Sun, 25 Jun 2023 15:15:14 PDT (-0700), Conor Dooley wrote: > Hey, > > On Sun, Jun 25, 2023 at 11:09:21PM +0800, Song Shuai wrote: > >> Sorry for the delayed reply, > > It wasn't really delayed at all actually, you replied within an hour or > so, AFAICT. > >> My tinylab email went something wrong, I'll use gmail in this thread. >> >> 在 2023/6/25 22:18, Conor Dooley 写道: >> > On Sun, Jun 25, 2023 at 10:09:29PM +0800, Song Shuai wrote: >> > > This reverts commit ed309ce522185583b163bd0c74f0d9f299fe1826. >> > > >> > > With the commit 3335068f8721 ("riscv: Use PUD/P4D/PGD pages for the >> > > linear mapping") reverted, the MIN_MEMBLOCK_ADDR points the kernel >> > > load address which was placed at a PMD boundary. >> > >> > > And firmware always >> > > correctly mark resident memory, or memory protected with PMP as >> > > per the devicetree specification and/or the UEFI specification. >> > >> > But this is not true? The versions of OpenSBI that you mention in your >> > cover letter do not do this. >> > Please explain. >> > >> >> At this time, OpenSbi [v0.8,v1.3) and edk2(RiscVVirt) indeed don't obey the >> DT/UEFI spec. This statement is excerpted from "Reserved memory for resident >> firmware" part from the upcoming riscv/boot.rst. It isn't accurate for now. >> How about deleting this one? > > It is incorrect, so it will need to be removed, yes. > Unfortunately writing a doc does not fix the existing implementations :( > >> Actually with 3335068f8721 reverted, the change of MIN_MEMBLOCK_ADDR can >> avoid the mapping of firmware memory, I will make it clear in the next >> version. > > To be honest, I'd like to see this revert as the final commit in a > series that deals with the problem by actually reserving the regions, > rather than a set of reverts that go back to how we were. > I was hoping that someone who cares about hibernation support would be > interested in working on that - *cough* starfive *cough*, although maybe > they just fixed their OpenSBI and moved on. > If there were no volunteers, my intention was to add a firmware erratum > that would probe the SBI implementation & version IDs, and add a firmware > erratum that'd parse the DT for the offending regions and reserve them. Is there any actual use case for hibernation on these boards? Maybe it's simpler to just add a "reserved regions actually work" sort of property and then have new firmware set it -- that way we can avoid sorting through all the old stuff nobody cares about and just get on with fixing the stuff people use. > > Cheers, > Conor. > >> > > So those regions will not be mapped in the linear mapping and they >> > > can be safely saved/restored by hibernation. >> > > >> > > Signed-off-by: Song Shuai <songshuaishuai@tinylab.org> >> > > --- >> > > arch/riscv/Kconfig | 5 +---- >> > > 1 file changed, 1 insertion(+), 4 deletions(-) >> > > >> > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig >> > > index 5966ad97c30c..17b5fc7f54d4 100644 >> > > --- a/arch/riscv/Kconfig >> > > +++ b/arch/riscv/Kconfig >> > > @@ -800,11 +800,8 @@ menu "Power management options" >> > > source "kernel/power/Kconfig" >> > > -# Hibernation is only possible on systems where the SBI implementation has >> > > -# marked its reserved memory as not accessible from, or does not run >> > > -# from the same memory as, Linux >> > > config ARCH_HIBERNATION_POSSIBLE >> > > - def_bool NONPORTABLE >> > > + def_bool y >> > > config ARCH_HIBERNATION_HEADER >> > > def_bool HIBERNATION >> > > -- >> > > 2.20.1 >>
On Sun, Jun 25, 2023 at 03:36:06PM -0700, Palmer Dabbelt wrote: > On Sun, 25 Jun 2023 15:15:14 PDT (-0700), Conor Dooley wrote: > > On Sun, Jun 25, 2023 at 11:09:21PM +0800, Song Shuai wrote: > > > 在 2023/6/25 22:18, Conor Dooley 写道: > > > > On Sun, Jun 25, 2023 at 10:09:29PM +0800, Song Shuai wrote: > > > > > This reverts commit ed309ce522185583b163bd0c74f0d9f299fe1826. > > > > > > > With the commit 3335068f8721 ("riscv: Use PUD/P4D/PGD pages > > > for the > > > > > linear mapping") reverted, the MIN_MEMBLOCK_ADDR points the kernel > > > > > load address which was placed at a PMD boundary. > > > > > > And firmware always > > > > > correctly mark resident memory, or memory protected with PMP as > > > > > per the devicetree specification and/or the UEFI specification. > > > > > But this is not true? The versions of OpenSBI that you mention > > > in your > > > > cover letter do not do this. > > > > Please explain. > > > > > > > > > > At this time, OpenSbi [v0.8,v1.3) and edk2(RiscVVirt) indeed don't obey the > > > DT/UEFI spec. This statement is excerpted from "Reserved memory for resident > > > firmware" part from the upcoming riscv/boot.rst. It isn't accurate for now. > > > How about deleting this one? > > > > It is incorrect, so it will need to be removed, yes. > > Unfortunately writing a doc does not fix the existing implementations :( > > > > > Actually with 3335068f8721 reverted, the change of MIN_MEMBLOCK_ADDR can > > > avoid the mapping of firmware memory, I will make it clear in the next > > > version. > > > > To be honest, I'd like to see this revert as the final commit in a > > series that deals with the problem by actually reserving the regions, > > rather than a set of reverts that go back to how we were. > > I was hoping that someone who cares about hibernation support would be > > interested in working on that - *cough* starfive *cough*, although maybe > > they just fixed their OpenSBI and moved on. > > If there were no volunteers, my intention was to add a firmware erratum > > that would probe the SBI implementation & version IDs, and add a firmware > > erratum that'd parse the DT for the offending regions and reserve them. > > Is there any actual use case for hibernation on these boards? Maybe it's > simpler to just add a "reserved regions actually work" sort of property and > then have new firmware set it -- that way we can avoid sorting through all > the old stuff nobody cares about and just get on with fixing the stuff > people use. What is "old stuff nobody cares about"? The first version of OpenSBI with the fix shipped only the other day, so effectively all current stuff has this problem. Certainly everything shipping from vendors at the moment has the problem, and probably whatever downstream, custom versions of OpenSBI also have it. Also, the problem isn't just limited to hibernation apparently. I think it was mentioned in the cover letter that according to Rob, without being marked as no-map we could also see speculative access & potentially some of the memory debugging stuff walking these regions. I'm not sure how you'd intend communicating "reserved regions actually work", I figure you mean via DT? I don't really see the benefit of adding a property for those who are behaving, if we can detect the versions of the one relevant SBI implementation that are broken at runtime. DT hat on, even less so. Perhaps I am missing your point, and there's another angle (like trying to per firmware code)?
On Mon, 26 Jun 2023 06:34:43 PDT (-0700), Conor Dooley wrote: > On Sun, Jun 25, 2023 at 03:36:06PM -0700, Palmer Dabbelt wrote: >> On Sun, 25 Jun 2023 15:15:14 PDT (-0700), Conor Dooley wrote: >> > On Sun, Jun 25, 2023 at 11:09:21PM +0800, Song Shuai wrote: >> > > 在 2023/6/25 22:18, Conor Dooley 写道: >> > > > On Sun, Jun 25, 2023 at 10:09:29PM +0800, Song Shuai wrote: >> > > > > This reverts commit ed309ce522185583b163bd0c74f0d9f299fe1826. >> > > > > > > With the commit 3335068f8721 ("riscv: Use PUD/P4D/PGD pages >> > > for the >> > > > > linear mapping") reverted, the MIN_MEMBLOCK_ADDR points the kernel >> > > > > load address which was placed at a PMD boundary. >> > > > > > And firmware always >> > > > > correctly mark resident memory, or memory protected with PMP as >> > > > > per the devicetree specification and/or the UEFI specification. >> > > > > But this is not true? The versions of OpenSBI that you mention >> > > in your >> > > > cover letter do not do this. >> > > > Please explain. >> > > > >> > > >> > > At this time, OpenSbi [v0.8,v1.3) and edk2(RiscVVirt) indeed don't obey the >> > > DT/UEFI spec. This statement is excerpted from "Reserved memory for resident >> > > firmware" part from the upcoming riscv/boot.rst. It isn't accurate for now. >> > > How about deleting this one? >> > >> > It is incorrect, so it will need to be removed, yes. >> > Unfortunately writing a doc does not fix the existing implementations :( >> > >> > > Actually with 3335068f8721 reverted, the change of MIN_MEMBLOCK_ADDR can >> > > avoid the mapping of firmware memory, I will make it clear in the next >> > > version. >> > >> > To be honest, I'd like to see this revert as the final commit in a >> > series that deals with the problem by actually reserving the regions, >> > rather than a set of reverts that go back to how we were. >> > I was hoping that someone who cares about hibernation support would be >> > interested in working on that - *cough* starfive *cough*, although maybe >> > they just fixed their OpenSBI and moved on. >> > If there were no volunteers, my intention was to add a firmware erratum >> > that would probe the SBI implementation & version IDs, and add a firmware >> > erratum that'd parse the DT for the offending regions and reserve them. >> >> Is there any actual use case for hibernation on these boards? Maybe it's >> simpler to just add a "reserved regions actually work" sort of property and >> then have new firmware set it -- that way we can avoid sorting through all >> the old stuff nobody cares about and just get on with fixing the stuff >> people use. > > What is "old stuff nobody cares about"? The first version of OpenSBI with > the fix shipped only the other day, so effectively all current stuff has > this problem. Certainly everything shipping from vendors at the moment > has the problem, and probably whatever downstream, custom versions of > OpenSBI also have it. Ya, so "old stuff" is everything -- but that's all already broken, so nothing we can do about it. IIUC there's nothing shipping that functions correctly here, so it's just a matter of detecting everything before the bug. > Also, the problem isn't just limited to hibernation apparently. I > think it was mentioned in the cover letter that according to Rob, > without being marked as no-map we could also see speculative access & > potentially some of the memory debugging stuff walking these regions. We've got a bunch of other problems around speculative accesses to these regions in M-mode, so we'll have to deal with it at some point anyway. > I'm not sure how you'd intend communicating "reserved regions actually > work", I figure you mean via DT? Somewhere in DT. I hadn't thought about it a ton, just adding some property that says "this doesn't have the bug" was roughly the idea. > I don't really see the benefit of adding a property for those who are > behaving, if we can detect the versions of the one relevant SBI > implementation that are broken at runtime. DT hat on, even less so. > Perhaps I am missing your point, and there's another angle (like trying > to per firmware code)? If it's easy to figure out which versions are broken that seems fine to me. My worry was just that's hard to do (folks forking OpenSBI) and it might be easier to just
On Mon, Jun 26, 2023 at 07:43:42AM -0700, Palmer Dabbelt wrote: > On Mon, 26 Jun 2023 06:34:43 PDT (-0700), Conor Dooley wrote: > > On Sun, Jun 25, 2023 at 03:36:06PM -0700, Palmer Dabbelt wrote: > > > On Sun, 25 Jun 2023 15:15:14 PDT (-0700), Conor Dooley wrote: > > > > On Sun, Jun 25, 2023 at 11:09:21PM +0800, Song Shuai wrote: > > > > > 在 2023/6/25 22:18, Conor Dooley 写道: > > > > > > On Sun, Jun 25, 2023 at 10:09:29PM +0800, Song Shuai wrote: > > > > > > > This reverts commit ed309ce522185583b163bd0c74f0d9f299fe1826. > > > > > > > > > With the commit 3335068f8721 ("riscv: Use PUD/P4D/PGD pages > > > > > for the > > > > > > > linear mapping") reverted, the MIN_MEMBLOCK_ADDR points the kernel > > > > > > > load address which was placed at a PMD boundary. > > > > > > > > And firmware always > > > > > > > correctly mark resident memory, or memory protected with PMP as > > > > > > > per the devicetree specification and/or the UEFI specification. > > > > > > > But this is not true? The versions of OpenSBI that you mention > > > > > in your > > > > > > cover letter do not do this. > > > > > > Please explain. > > > > > > > > > > > > > At this time, OpenSbi [v0.8,v1.3) and edk2(RiscVVirt) indeed > > > don't obey the > > > > > DT/UEFI spec. This statement is excerpted from "Reserved memory for resident > > > > > firmware" part from the upcoming riscv/boot.rst. It isn't accurate for now. > > > > > How about deleting this one? > > > > > It is incorrect, so it will need to be removed, yes. > > > > Unfortunately writing a doc does not fix the existing implementations :( > > > > > > Actually with 3335068f8721 reverted, the change of > > > MIN_MEMBLOCK_ADDR can > > > > > avoid the mapping of firmware memory, I will make it clear in the next > > > > > version. > > > > > To be honest, I'd like to see this revert as the final commit in > > > a > > > > series that deals with the problem by actually reserving the regions, > > > > rather than a set of reverts that go back to how we were. > > > > I was hoping that someone who cares about hibernation support would be > > > > interested in working on that - *cough* starfive *cough*, although maybe > > > > they just fixed their OpenSBI and moved on. > > > > If there were no volunteers, my intention was to add a firmware erratum > > > > that would probe the SBI implementation & version IDs, and add a firmware > > > > erratum that'd parse the DT for the offending regions and reserve them. > > > > > > Is there any actual use case for hibernation on these boards? Maybe it's > > > simpler to just add a "reserved regions actually work" sort of property and > > > then have new firmware set it -- that way we can avoid sorting through all > > > the old stuff nobody cares about and just get on with fixing the stuff > > > people use. > > > > What is "old stuff nobody cares about"? The first version of OpenSBI with > > the fix shipped only the other day, so effectively all current stuff has > > this problem. Certainly everything shipping from vendors at the moment > > has the problem, and probably whatever downstream, custom versions of > > OpenSBI also have it. > > Ya, so "old stuff" is everything -- but that's all already broken, so > nothing we can do about it. IIUC there's nothing shipping that functions > correctly here, so it's just a matter of detecting everything before the > bug. g5soc functions correctly, because our HSS (which a small portion of OpenSBI is built into) doesn't run out of system memory. > > Also, the problem isn't just limited to hibernation apparently. I > > think it was mentioned in the cover letter that according to Rob, > > without being marked as no-map we could also see speculative access & > > potentially some of the memory debugging stuff walking these regions. > > We've got a bunch of other problems around speculative accesses to these > regions in M-mode, so we'll have to deal with it at some point anyway. > > > I'm not sure how you'd intend communicating "reserved regions actually > > work", I figure you mean via DT? > > Somewhere in DT. I hadn't thought about it a ton, just adding some property > that says "this doesn't have the bug" was roughly the idea. > > > I don't really see the benefit of adding a property for those who are > > behaving, if we can detect the versions of the one relevant SBI > > implementation that are broken at runtime. DT hat on, even less so. > > Perhaps I am missing your point, and there's another angle (like trying > > to per firmware code)? > > If it's easy to figure out which versions are broken that seems fine to me. > My worry was just that's hard to do (folks forking OpenSBI) and it might be > easier to just You cut yourself off here :/ If people are forking OpenSBI, but reusing the version/implementation info, and changing behaviours, I am not really sure what we are supposed to do in general. I'd vote for apply the erratum to those forks, based on the version info, until they make changes That's pretty much the same thing that'd happen under your proposal anyway? I went and sent a PR last week to get a custom ID for the HSS on g5soc so that we can be differentiated, everyone else that doesn't want to be tarred with the same brush probably should too, if they are sufficiently different.
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 5966ad97c30c..17b5fc7f54d4 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -800,11 +800,8 @@ menu "Power management options" source "kernel/power/Kconfig" -# Hibernation is only possible on systems where the SBI implementation has -# marked its reserved memory as not accessible from, or does not run -# from the same memory as, Linux config ARCH_HIBERNATION_POSSIBLE - def_bool NONPORTABLE + def_bool y config ARCH_HIBERNATION_HEADER def_bool HIBERNATION