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: <Zzxuv3FFmCxmTtS-@gmail.com>
Date: Tue, 19 Nov 2024 11:55:59 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Baoquan He <bhe@...hat.com>
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()


* 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.

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.

Thanks,

	Ingo