Re: [PATCH] x86/mm: Introduce helper for checking direct map 1G page support

[Dave Hansen <dave.hansen@intel.com>]

On 9/2/26 17:34, Sohil Mehta wrote:
> On 9/2/2026 12:47 PM, Dave Hansen wrote:
>>  extern int direct_gbpages;
>> +static inline bool direct_gbpages_enabled(void)
>> +{
>> +	/* Check the direct map config option: */
>> +	if (!IS_ENABLED(CONFIG_X86_DIRECT_GBPAGES))
>> +		return false;
>> +
>> +	/* Check the CPU feature: */
>> +	if (!boot_cpu_has(X86_FEATURE_GBPAGES))
>> +		return false;
>> +
> 
> The first two comments don't add much beyond the code.
The "CPU feature" one is arguable. But even when writing this, I was
forgetful about what CONFIG_X86_DIRECT_GBPAGES actually did. I _think_
it was the fact that these:

	CONFIG_X86_DIRECT_GBPAGES
	X86_FEATURE_GBPAGES

kinda read similarly if you're reading fast. The config option also
doesn't have the most enlightening name.

The comments are more there to get the reader to slow down than anything
else.

> Would it be useful to say why boot_cpu_has() instead of
> static_cpu_has() over here (mainly to avoid accidental cleanup)?
It's a pretty minor thing. To me, it's changelog material, not comment
material.

>> +	/* Check the command-line and early setup variable: */
>> +	return direct_gbpages;
>> +}
>> +
>>  void init_mem_mapping(void);
>>  void early_alloc_pgt_buf(void);
>>  void __init poking_init(void);
>> diff -puN arch/x86/kernel/machine_kexec_64.c~direct_gbpages-compiletime arch/x86/kernel/machine_kexec_64.c
>> --- a/arch/x86/kernel/machine_kexec_64.c~direct_gbpages-compiletime	2026-09-02 10:09:00.004169798 -0700
>> +++ b/arch/x86/kernel/machine_kexec_64.c	2026-09-02 10:09:00.012170479 -0700
>> @@ -257,7 +257,7 @@ static int init_pgtable(struct kimage *i
>>  		info.kernpg_flag |= _PAGE_ENC;
>>  	}
>>  
>> -	if (direct_gbpages)
>> +	if (direct_gbpages_enabled())
>>  		info.direct_gbpages = true;
> 
> How about:
> 
> 	info.direct_gbpages = direct_gbpages_enabled()

First and foremost, in a refactoring patch, you must resist the urge to
do this. Refactoring patches' job is to show -- in the most plain way
possible -- that they are not hurting things. The easiest way to do that
is to make it as stupidly obvious as possible to the reader that nothing
is changing.

The moment you start making changes like the suggestion, you make
reviewers' lives harder.