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

From: Ingo Molnar
Date: Mon Nov 25 2024 - 04:07:38 EST



* 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