Re: [PATCH v5 01/51] x86/apic: Provide helpers to set local APIC timer period in hz and khz

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The `apic_set_timer_period_khz()` and `apic_set_timer_period_hz()` helper functions accept a `const char *source` argument which is never used, resulting in dead code and a silent loss of boot diagnostic logging on platforms where logging was removed.
--

commit 4fb576b0d909663c9b3e6628dbed45d329c08a5d
Author: Sean Christopherson <[email protected]>

x86/apic: Provide helpers to set local APIC timer period in hz and khz

This commit adds APIs to set the local APIC timer period, abstracting the
HZ math previously open-coded by external callers. It also changes the
lapic_timer_period variable to be local to apic.c.

> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index aa1e19979aa8e..8d3d930576fd7 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c

[ ... ]

> @@ -796,6 +796,16 @@ bool __init apic_needs_pit(void)
>  	return lapic_timer_period == 0;
>  }
>  
> +void apic_set_timer_period_khz(u64 period_khz, const char *source)
> +{
> +	lapic_timer_period = mul_u64_u32_div(period_khz, 1000, HZ);
> +}
> +
> +void apic_set_timer_period_hz(u64 period_hz, const char *source)
> +{
> +	lapic_timer_period = div_u64(period_hz, HZ);
> +}

[Severity: Medium]
It looks like the source parameter is accepted by apic_set_timer_period_khz()
and apic_set_timer_period_hz() in arch/x86/kernel/apic/apic.c but is never
used. Was it intended to log the frequency using this source string?

> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index 185d4f677ec0f..87beecec76f04 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -646,10 +646,7 @@ static void __init ms_hyperv_init_platform(void)
>  		u64	hv_lapic_frequency;
>  
>  		rdmsrq(HV_X64_MSR_APIC_FREQUENCY, hv_lapic_frequency);
> -		hv_lapic_frequency = div_u64(hv_lapic_frequency, HZ);
> -		lapic_timer_period = hv_lapic_frequency;
> -		pr_info("Hyper-V: LAPIC Timer Frequency: %#x\n",
> -			lapic_timer_period);
> +		apic_set_timer_period_hz(hv_lapic_frequency, "Hyper-V hypervisor");
>  	}
>  

[ ... ]

> diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
> index 34b73573b108d..36f779dd311dc 100644
> --- a/arch/x86/kernel/cpu/vmware.c
> +++ b/arch/x86/kernel/cpu/vmware.c
> @@ -424,9 +424,7 @@ static void __init vmware_platform_setup(void)
>  
>  #ifdef CONFIG_X86_LOCAL_APIC
>  		/* Skip lapic calibration since we know the bus frequency. */
> -		lapic_timer_period = ecx / HZ;
> -		pr_info("Host bus clock speed read from hypervisor : %u Hz\n",
> -			ecx);
> +		apic_set_timer_period_hz(ecx, "VMware hypervisor");
>  #endif

[Severity: Medium]
These changes remove the pr_info() logging of the APIC frequency during early
boot APIC initialization in ms_hyperv_init_platform()
(arch/x86/kernel/cpu/mshyperv.c) and vmware_platform_setup()
(arch/x86/kernel/cpu/vmware.c).

Since the new apic_set_timer_period_hz() helper does not log this information,
does this result in an unintended loss of boot diagnostic logging on these
platforms?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1