linux-kernel - Re: [PATCH v2 1/2] x86/ioremap: introduce helper to implement xxx_is_setup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zz2ODGBXmOLDY4mv@MiWiFi-R3L-srv>
Date: Wed, 20 Nov 2024 15:21:48 +0800
From: Baoquan He <bhe@...hat.com>
To: Ingo Molnar <mingo@...nel.org>
Cc: Tom Lendacky <thomas.lendacky@....com>, linux-kernel@...r.kernel.org,
	bp@...en8.de, x86@...nel.org
Subject: Re: [PATCH v2 1/2] x86/ioremap: introduce helper to implement
 xxx_is_setup_data()

On 11/19/24 at 11:55am, Ingo Molnar wrote:
> 
> * Baoquan He <bhe@...hat.com> wrote:
> 
> > On 11/18/24 at 09:19am, Tom Lendacky wrote:
> > > On 11/17/24 19:08, Baoquan He wrote:
> > > > Functions memremap_is_setup_data() and early_memremap_is_setup_data()
> > > > share completely the same process and handling, except of the
> > > > different memremap/unmap invocations.
> > > > 
> > > > So add helper __memremap_is_setup_data() to extract the common part,
> > > > parameter 'early' is used to decide what kind of memremap/unmap
> > > > APIs are called. This simplifies codes a lot by removing the duplicated
> > > > codes, and also removes the similar code comment above them.
> > > > 
> > > > And '__ref' is added to __memremap_is_setup_data() to suppress below
> > > > section mismatch warning:
> > > > 
> > > > ARNING: modpost: vmlinux: section mismatch in reference: __memremap_is_setup_data+0x5f (section: .text) ->
> > > > early_memunmap (section: .init.text)
> > > > 
> > > > Signed-off-by: Baoquan He <bhe@...hat.com>
> > > > ---
> > > >  arch/x86/mm/ioremap.c | 108 +++++++++++++++---------------------------
> > > >  1 file changed, 38 insertions(+), 70 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> > > > index 8d29163568a7..68d78e2b1203 100644
> > > > --- a/arch/x86/mm/ioremap.c
> > > > +++ b/arch/x86/mm/ioremap.c
> > > > @@ -628,12 +628,13 @@ static bool memremap_is_efi_data(resource_size_t phys_addr,
> > > >  	return false;
> > > >  }
> > > >  
> > > > +#define SD_SIZE sizeof(struct setup_data)
> > > 
> > > Nit, I still think you should use "sizeof(*data)" in the code instead of
> > > creating a #define.
> > 
> > Thanks for reviewing, Tom.
> > 
> > Boris suggested this. Both is fine to me. If there is indeed a tiny
> > preference, I would choose SD_SIZE. It's going a bit far, but not too
> > far.
> 
> Yeah, I'd prefer Boris's SD_SIZE suggestion too: while *normally* we'd 
> use the 'sizeof(*data)' pattern, this particular size repeats a number 
> of times and not all contexts are obvious - so abstracting it out into 
> a trivial define looks like the proper cleanup.

Totally agree.

> 
> Maybe such material changes should be done in a separate patch though:
> 
>    x86/ioremap: Introduce helper to implement xxx_is_setup_data()
>    x86/ioremap: Clean up size calculations in xxx_is_setup_data()
> 
> ... or so, where the first patch is a trivial refactoring that keeps 
> the existing patterns - which would make the series easier to review.

OK, will do like this if Tom doesn't oppose it.

During v1, Tom suggested squashing the helper introducing patch and
helper using patch into one patch. I personally prefer splitting code
changes into multiple independent units so that reviewing is focused on
the controversial change, but not the whole big patch including many
controversial points is posted again and again. 

Thanks a lot for careful reviewing and great suggestions.