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

[Ingo Molnar]

* Tom Lendacky <thomas.lendacky@xxxxxxx> wrote:

> On 11/20/24 02:25, Ingo Molnar wrote:
> > 
> > * Tom Lendacky <thomas.lendacky@xxxxxxx> 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?

> But, I don't anticipate that this helper will be called by anything 
> else than what is currently calling it and the proper boolean values 
> are set on those calls.
> 
> I just wanted to raise awareness. I'm ok with using __ref, just 
> wanted to make sure everyone else is, too.

It's a fair argument I misunderstood :-)

Thanks,

	Ingo