Re: [PATCH v2 1/2] x86/ioremap: introduce helper to implement xxx_is_setup_data()

[Baoquan He]

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@xxxxxxxxxx>
> > ---
> >  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.

> 
> >  /*
> >   * Examine the physical address to determine if it is boot data by checking
> >   * it against the boot params setup_data chain.
> >   */
> > -static bool memremap_is_setup_data(resource_size_t phys_addr,
> > -				   unsigned long size)
> > +static bool __ref __memremap_is_setup_data(resource_size_t phys_addr,
> 
> Oh, I see why the __ref is needed now, because this calls an __init
> function based on the early bool.

Exactly, I explained in another thread replying to you, it could be
ignored.

> 
> While this nicely consolidates the checking, I'll let the x86
> maintainers decide whether they like that an __init function is calling
> a non __init function.
...... snip.......
> > -	return false;
> > +static bool early_memremap_is_setup_data(resource_size_t phys_addr,
> 
> This should retain the original __init reference.

OK, so you suggest they are like below, right?

static bool __ref __memremap_is_setup_data(resource_size_t phys_addr,
                                                bool early)
{
	......
}

static bool memremap_is_setup_data(resource_size_t phys_addr)
{
        return __memremap_is_setup_data(phys_addr, false);
}
 
static bool __init early_memremap_is_setup_data(resource_size_t phys_addr)
{
        return __memremap_is_setup_data(phys_addr, true);
}

I can make v3 if we all agree on this.