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

[Baoquan He <bhe@redhat.com>]

On 11/25/24 at 10:07am, Ingo Molnar wrote:
> 
> * Tom Lendacky <thomas.lendacky@amd.com> wrote:
> 
> > On 11/20/24 02:25, Ingo Molnar wrote:
> > > 
> > > * Tom Lendacky <thomas.lendacky@amd.com> wrote:
> > > 
> > >>>  /*
> > >>>   * 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.
> > >>
> > >> 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.
> > > 
> > > So why would it be a problem? Only non-__init calling __init is a bug, 
> > > because __init functions cease to exist after early bootup. Also, 
> > > calling certain kernel subsystems too early, before they are 
> > > initialized, is a bug as well.
> > 
> > I brought it up because that is what could happen if the wrong boolean
> > value is supplied to the helper function. The helper function is marked
> > non-__init but calls a __init function if the boolean value is true, hence
> > the need for the __ref tagging.
> 
> Oh, so I misunderstood your point, because you typoed the direction:
> 
>   > >> 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.
> 
> The problem is the inverse: that a non-__init generic facility may be 
> calling an __init function if the wrong flag is supplied. As you wrote 
> a sentence earlier, but I only responded to this paragraph :-/
> 
> So yeah, that's a fragility indeed - which happens sometimes when 
> generic MM facilities share code (I think 
> mm/sparse.c::section_deactivate() is similar), but I tend to agree that 
> this pattern could perhaps be improved:
> 
> +                       if (early)
> +                               early_memunmap(data, SD_SIZE);
> +                       else
> +                               memunmap(data);
> 
> Could we perhaps un-__init early_memunmap(), and call memunmap() if 
> it's in a late context? (Also early_memremap_decrypted().)
> 
> That way this code could just use early_memunmap() and 
> early_memremap_decrypted() and skip the boolean complication?

If trying to skip the bool 'early' checking, we can possibly do it as
below:

                       if (system_state < SYSTEM_FREEING_INITMEM)
                               early_memunmap(data, SD_SIZE);
                       else
                               memunmap(data);

As for un-__init early_memunmap(), do you mean we just remove the __init
from early_memunmap()/early_memremap_decrypted() which are located in
kernel/early_ioremap.c and arch/x86/mm/ioremap.c? This doesn't cost much
memory, while it may make people complain if we only want to optimize
code when doing reducing code duplication, my humble opinion. Not sure
if I got your suggestion correctly.