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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id B4406C433F5 for ; Mon, 7 Feb 2022 21:45:44 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id EE0696B0071; Mon, 7 Feb 2022 16:45:43 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id E69BA6B0075; Mon, 7 Feb 2022 16:45:43 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CE31B6B0078; Mon, 7 Feb 2022 16:45:43 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0081.hostedemail.com [216.40.44.81]) by kanga.kvack.org (Postfix) with ESMTP id BA0146B0071 for ; Mon, 7 Feb 2022 16:45:43 -0500 (EST) Received: from smtpin05.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 6BC8B181D6CE1 for ; Mon, 7 Feb 2022 21:45:43 +0000 (UTC) X-FDA: 79117316166.05.CAE753C Received: from mail-pf1-f171.google.com (mail-pf1-f171.google.com [209.85.210.171]) by imf11.hostedemail.com (Postfix) with ESMTP id 0FC4940003 for ; Mon, 7 Feb 2022 21:45:42 +0000 (UTC) Received: by mail-pf1-f171.google.com with SMTP id y5so14387209pfe.4 for ; Mon, 07 Feb 2022 13:45:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=C54NHAHJe2/RhvlXxtN7Quo9XCOtI81aWyvbTsjYtFo=; b=WYj4mFjAk6xht/r9QypenfKoozQsNt1hZb9UVLupmsvKeRxgH4hAvbq0X32fICFEQL NqqTzy0DS9MAn7DNgVlGKUun4x4nh44sEMZnGxmlL7I28wfcP8KbDOh2SlMDx6Qw6eq3 yFWZvcvI0fMUrZ2pwsgr7NxV55WGpZtirafQU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=C54NHAHJe2/RhvlXxtN7Quo9XCOtI81aWyvbTsjYtFo=; b=2ih60Wm035E/6VpEyjNDqrsTDZVumACl7U1x/NXTw0u+nPfJb9u13jlX2yVUAk1VfV oF+aNjtgIEuV2bqeieLHLZGwbT6EmVSXVMRXuYcrRF+fbnMfCqylJzOADDn+uvblbiHO VSbnNsxcJYgZVClTF9HpQR3LlRUNfYCY29+LTcX3KC7H5R5MZkeiGieVtrwoFow0KeVK b2o7aKuvhoDADEZHmVoSq1Q0jjROZe0RQxUn+/6r5dmUIbYnxJW9zdjI2TbfrhKeA5aU XuKfEMb+yYNAmfD2xwQFKgAaL3bXu3Ov9i2IkWYJptaz7n2Xuyo6Gg4yOEbLYplMv/fx BcLg== X-Gm-Message-State: AOAM533xU/9F7fCLctWBFUGrMDSyXNxMXMhggEgaOuMVRveT2k/xYo8/ ToUH41EuyShN2RBE/+lOxIkwiw== X-Google-Smtp-Source: ABdhPJxNZEPOxw9TppzGQbOpuglsG0ky7x1oEV1Zp16yu8938Adf8G2a+ttz7MKgKhFpMpcMJqYinA== X-Received: by 2002:a63:69c8:: with SMTP id e191mr1077596pgc.412.1644270341903; Mon, 07 Feb 2022 13:45:41 -0800 (PST) Received: from www.outflux.net (smtp.outflux.net. [198.145.64.163]) by smtp.gmail.com with ESMTPSA id d22sm12919245pfl.71.2022.02.07.13.45.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 07 Feb 2022 13:45:41 -0800 (PST) Date: Mon, 7 Feb 2022 13:45:40 -0800 From: Kees Cook To: Martin Fernandez Cc: linux-kernel@vger.kernel.org, linux-efi@vger.kernel.org, platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, tglx@linutronix.de, mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com, ardb@kernel.org, dvhart@infradead.org, andy@infradead.org, gregkh@linuxfoundation.org, rafael@kernel.org, rppt@kernel.org, akpm@linux-foundation.org, daniel.gutson@eclypsium.com, hughsient@gmail.com, alex.bazhaniuk@eclypsium.com, alison.schofield@intel.com Subject: Re: [PATCH v6 3/6] x86/e820: Refactor range_update and range_remove Message-ID: <202202071325.F8450B3B2D@keescook> References: <20220203164328.203629-1-martin.fernandez@eclypsium.com> <20220203164328.203629-4-martin.fernandez@eclypsium.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220203164328.203629-4-martin.fernandez@eclypsium.com> X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 0FC4940003 X-Rspam-User: nil Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=WYj4mFjA; dmarc=pass (policy=none) header.from=chromium.org; spf=pass (imf11.hostedemail.com: domain of keescook@chromium.org designates 209.85.210.171 as permitted sender) smtp.mailfrom=keescook@chromium.org X-Stat-Signature: k6wonabphjg3rpnj73e8ajm1zcsruo45 X-HE-Tag: 1644270342-581323 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 Thu, Feb 03, 2022 at 01:43:25PM -0300, Martin Fernandez wrote: > __e820__range_update and e820__range_remove had a very similar > implementation with a few lines different from each other, the lines > that actually perform the modification over the e820_table. The > similiraties were found in the checks for the different cases on how > each entry intersects with the given range (if it does at all). These > checks were very presice and error prone so it was not a good idea to > have them in both places. Yay removing copy/paste code! :) > > I propose a refactor of those functions, given that I need to create a > similar one for this patchset. The diff here is pretty hard (for me) to review; I'll need more time to check it. What might make review easier (at least for me), is to incrementally change these routines. i.e. separate patches to: - add the new infrastructure - replace e820__range_remove - replace __e820__range_update If that's not actually useful, no worries. I'll just stare at it a bit more. :) > > Add a function to modify a E820 table in a given range. This > modification is done backed up by two helper structs: > e820_entry_updater and e820_*_data. > > The first one, e820_entry_updater, carries 3 callbacks which function > as the actions to take on the table. > > The other one, e820_*_data carries information needed by the > callbacks, for example in the case of range_update it will carry the > type that we are targeting. Something I think would be really amazing here is if you could add KUnit tests here to exercise the corner cases and validate the changes. It should be pretty easy to add. Here's a quick example for the boilerplate and testing a bit of __e820__range_add(): #ifdef CONFIG_E820_KUNIT_TEST #include static void __init test_e820_range_add(struct kunit *context) { struct e820_table table; u32 full; full = ARRAY_SIZE(table.entries); /* Add last entry. */ table->nr_entries = full - 1; __e820__range_add(&table, 0, 15, 0); KUNIT_EXPECT_EQ(table->nr_entries, full) /* Skip new entry when full. */ __e820__range_add(&table, 0, 15, 0); KUNIT_EXPECT_EQ(table->nr_entries, full) } static void __init test_e820_update(struct kunit *context) { ... } static struct kunit_case __refdata e820_test_cases[] = { KUNIT_CASE(test_e820_range_add), KUNIT_CASE(test_e820_update), ... {} }; static struct kunit_suite e820_test_suite = { .name = "e820", .test_cases = e820_test_cases, }; kunit_test_suites(&e820_test_suite); #endif -- Kees Cook