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;
Date:   Tue, 8 Feb 2022 10:40:24 +0200
From:   Mike Rapoport <rppt@kernel.org>
To:     Kees Cook <keescook@chromium.org>
Cc:     Martin Fernandez <martin.fernandez@eclypsium.com>,
        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,
        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: <YgIseIEMotD2jg83@kernel.org>
References: <20220203164328.203629-1-martin.fernandez@eclypsium.com>
 <20220203164328.203629-4-martin.fernandez@eclypsium.com>
 <202202071325.F8450B3B2D@keescook>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <202202071325.F8450B3B2D@keescook>
Precedence: bulk

On Mon, Feb 07, 2022 at 01:45:40PM -0800, Kees Cook wrote:
> 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! :)

Removing copy/paste is nice but diffstat of

 arch/x86/kernel/e820.c | 383 ++++++++++++++++++++++++++++++-----------
 1 file changed, 283 insertions(+), 100 deletions(-)

does not look nice even accounting for lots of comments :(

I didn't look closely, but diffstat clues that the refactoring making
things much more complex.
 
> > 
> > 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. :)

-- 
Sincerely yours,
Mike.