From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1mFDGB-00081K-3N for mharc-grub-devel@gnu.org; Sun, 15 Aug 2021 06:20:55 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:34282) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mFDG9-00080Z-3c for grub-devel@gnu.org; Sun, 15 Aug 2021 06:20:53 -0400 Received: from out4-smtp.messagingengine.com ([66.111.4.28]:39405) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mFDG4-0005IH-3n for grub-devel@gnu.org; Sun, 15 Aug 2021 06:20:52 -0400 Received: from compute6.internal (compute6.nyi.internal [10.202.2.46]) by mailout.nyi.internal (Postfix) with ESMTP id 0E8FE5C00A7; Sun, 15 Aug 2021 06:20:45 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute6.internal (MEProxy); Sun, 15 Aug 2021 06:20:45 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm1; bh=R+iDD20LIcy2VwfoSOuSAwNEEa7 O+1z/u98tajVhkwI=; b=QroQ1lVjd/AgeHISOXio651vqOf3Om6UuPZHJf+rdDG LL7GUeCYsSfTg6KeR910Hf3b8jMwdXGFWzeRIqEiBKk1yecDa3BYonou92iu3jJZ elRQKHkF+bejJhzY75OYhgbB3sHvs1Gdf/KR+CDfG5eKjZ9Za39oKth80NraMHvc 11iOW+k4X4ywcpwiDRrz9mYBXqSRPG25U6U6Rxq1l09ysdhm47op6o05ecX9AqkP 6VbsfqE2M3+OYHfPz7hh4Pfy1oh5EfmO8jV4WnhO/GdNWyn8akcASQFoAja4loz4 wbkHyE+lJ38vnueLgpzrQuC8q2UTQZNzrUgHGXao4aA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm3; bh=R+iDD2 0LIcy2VwfoSOuSAwNEEa7O+1z/u98tajVhkwI=; b=TsbiN810mzfDMjn2R0s1g9 RAw/PkjvDFj3Y2krafkHNuZfvF4wb0cu60ydZCEzVSaW1LADdRPEiuO55NGAZPt/ i9tu4si08zUhC7TDrTNiq5YlfflrsGptMoERr5NqbjNlUdrkhlWQMFrU7t7zOzmS Ulhhx1Cj10IyFtnGW5A2K6hbCAnzdUqpL0vPUJPs0wjl/4IEyMQujlpUIUbMbCnB TNP+2N75EFVvYi0rmJLqLPTkIhaX2vFFmjcMJlXSLofYbJaSb4q7AQi3Hz4o11KR bCTw8QEf6rGS6d4Ck7kfkPEZmMu1SjoUak3+ggraXJKEYtANERG1k4v7OGO2f+hg == X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrkeelgddvjecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvuffkfhggtggujgesghdtreertddtvdenucfhrhhomheprfgrthhrihgt khcuufhtvghinhhhrghrughtuceophhssehpkhhsrdhimheqnecuggftrfgrthhtvghrnh epheeghfdtfeeuffehkefgffduleffjedthfdvjeektdfhhedvlefgtefgvdettdfhnecu vehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepphhssehpkh hsrdhimh X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Sun, 15 Aug 2021 06:20:43 -0400 (EDT) Received: from localhost (ncase [10.192.0.11]) by vm-mail.pks.im (OpenSMTPD) with ESMTPSA id eb873dc4 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Sun, 15 Aug 2021 10:20:39 +0000 (UTC) Date: Sun, 15 Aug 2021 12:20:37 +0200 From: Patrick Steinhardt To: The development of GNU GRUB Cc: Leif Lindholm , Stefan Berger Subject: Re: [PATCH v2 3/4] efi: mm: Pass up errors from `add_memory_regions ()` Message-ID: References: <20210809161001.vpbmayiu635tmw3j@tomti.i.net-space.pl> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="dTvVi+4F54bJYePH" Content-Disposition: inline In-Reply-To: <20210809161001.vpbmayiu635tmw3j@tomti.i.net-space.pl> Received-SPF: pass client-ip=66.111.4.28; envelope-from=ps@pks.im; helo=out4-smtp.messagingengine.com X-Spam_score_int: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 15 Aug 2021 10:20:53 -0000 --dTvVi+4F54bJYePH Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Aug 09, 2021 at 06:10:01PM +0200, Daniel Kiper wrote: > On Sun, Aug 08, 2021 at 03:31:49PM +0200, Patrick Steinhardt wrote: > > The function `add_memory_regions ()` is currently only called on system > > initialization to allocate a fixed amount of pages. As such, it didn't > > need to return any errors: in case it failed, we cannot proceed anyway. > > This will change with the upcoming support for requesting more memory > > from the firmware at runtime, where it doesn't make sense anymore to > > fail hard. > > > > Refactor the function to return an error to prepare for this. > > > > Signed-off-by: Patrick Steinhardt > > --- > > grub-core/kern/efi/mm.c | 21 +++++++++++++-------- > > 1 file changed, 13 insertions(+), 8 deletions(-) > > > > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c > > index ec64c08c0..376af10af 100644 > > --- a/grub-core/kern/efi/mm.c > > +++ b/grub-core/kern/efi/mm.c > > @@ -478,7 +478,7 @@ filter_memory_map (grub_efi_memory_descriptor_t *me= mory_map, > > } > > > > /* Add memory regions. */ > > -static void > > +static grub_err_t > > add_memory_regions (grub_efi_memory_descriptor_t *memory_map, > > grub_efi_uintn_t desc_size, > > grub_efi_memory_descriptor_t *memory_map_end, > > @@ -506,9 +506,9 @@ add_memory_regions (grub_efi_memory_descriptor_t *m= emory_map, > > GRUB_EFI_ALLOCATE_ADDRESS, > > GRUB_EFI_LOADER_CODE); > > if (! addr) > > - grub_fatal ("cannot allocate conventional memory %p with %u pages", > > - (void *) ((grub_addr_t) start), > > - (unsigned) pages); > > + return grub_error (GRUB_ERR_OUT_OF_MEMORY, > > + "cannot allocate conventional memory %p with %u pages", > > + (void *) ((grub_addr_t) start), (unsigned) pages); > > > > grub_mm_init_region (addr, PAGES_TO_BYTES (pages)); > > > > @@ -518,7 +518,9 @@ add_memory_regions (grub_efi_memory_descriptor_t *m= emory_map, > > } > > > > if (required_pages > 0) > > - grub_fatal ("too little memory"); > > + return grub_error (GRUB_ERR_OUT_OF_MEMORY, "too little memory"); > > + > > + return GRUB_ERR_NONE; > > } > > > > void > > @@ -565,6 +567,7 @@ grub_efi_mm_add_regions (grub_efi_uint64_t required= _bytes) > > grub_efi_memory_descriptor_t *filtered_memory_map_end; > > grub_efi_uintn_t map_size; > > grub_efi_uintn_t desc_size; > > + grub_err_t err; > > int mm_status; > > > > /* Prepare a memory region to store two memory maps. */ > > @@ -609,9 +612,11 @@ grub_efi_mm_add_regions (grub_efi_uint64_t require= d_bytes) > > sort_memory_map (filtered_memory_map, desc_size, filtered_memory_map= _end); > > > > /* Allocate memory regions for GRUB's memory management. */ > > - add_memory_regions (filtered_memory_map, desc_size, > > - filtered_memory_map_end, > > - BYTES_TO_PAGES (required_bytes)); > > + err =3D add_memory_regions (filtered_memory_map, desc_size, > > + filtered_memory_map_end, > > + BYTES_TO_PAGES (required_bytes)); > > + if (err !=3D GRUB_ERR_NONE) > > + return err; >=20 > Should not you call grub_fatal() here? Otherwise you change memory init > behavior in this patch. I suppose this is not what we want... >=20 > Daniel We already call `grub_fatal ()` in `grub_efi_mm_init ()` if `grub_efi_mm_add_regions ()` fails. So while it does move the callsite where we fail, the end result should be the same in the init path. And not failing fatally here is required so we can gracefully request additional pages at runtime without a panic in case it fails. Patrick --dTvVi+4F54bJYePH Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEF9hrgiFbCdvenl/rVbJhu7ckPpQFAmEY6nQACgkQVbJhu7ck PpRniw//Us4Yf8UWZZ3gguPkXY85oLqn0BG/MZ2Ie4lE5zz0Wm1FFXl/455B2CKD ikTW7U9dTXqfeM8mosocGvOeBZkRPzftvedQmbYly32XoAgdNIF+WCIzK8KNKNpb ykX5+IXOBSwKEIlxJF+DB7zhT5Ygdu1DsrZ+kDeh+5OvY6uSyT5tvGqbWLgQWXON ourYcefVbTK90x81+X96fmqoNlPTGHhXn8118CFFWmkF2jhtbVcFTkS2pPsDXQ77 yvhipy0ZN/p1S32Wpuu6oL5o5Ii+YD5aP6RlkHz/wUQYegtIfoRTyYWFlf8DuWbK 4psF+TRKPTkqy0rO2NU7xA1Wx8rE8YVAKk0f4Rr6kUHUIKG7jfWxm+DbaWHUP8vq 1ey9qbtRO6Jiam28l4tXOuhTfmkvCCD1oZybnSsu/tdGfgoVPJldL1vqYIqds0tL GkXLEXrdX9edC2h3WgbpwyLS+BKCP9nwqxEtXx5l8/z0ne1mefyAZ9hupblFSb0N fj3bPUAIGxSZCwUitVcpNIUuUC/PBCxqZWuAKECD2vvLhHo0RJm6tptwsgMiuDLg chcdqQch9ZyPjDN9mDeOjeyoPvRa9m/1PVxPdWPLKvRdzYL9u5w/yFY3e8ywqR73 rgcEBugrW1ubYyHMFlR3vYlPxbkGrp1LOq49jry1hpWulRraxxk= =0kA9 -----END PGP SIGNATURE----- --dTvVi+4F54bJYePH--