Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x86: implement CPUID 15h support #143

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

petershh
Copy link
Contributor

This allows us to get TSC and APIC rates without calibration on processors supporting this leaf.

@heatd heatd self-requested a review November 29, 2024 18:31
Copy link
Owner

@heatd heatd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, minus these nits


for (int i = 0; i < CALIBRATION_TRIALS; i++)
{
apic_calibrate(i);
if (calibrate_apic)
{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't add {} around single-statement ifs


for (int i = 0; i < 3; i++)
{
deltas[i] = calib.apic_ticks[i];
}

apic_rate = calculate_frequency(deltas, 1);
if (calibrate_apic)
{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and up there)

{
if (ebx == 0 || eax == 0)
{
INFO("x86cpu", "CPUID 0x15 is useless, reverting to calibration\n");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same single-brace statement (apply to elsewhere in the review)
Don't use INFO but pr_info (maybe pr_info("x86: ..."))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to get rid of INFO in this and other files in a separate PR

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not asking you to get rid of all INFOs, just don't add new ones

/* Some Intel SoCs don't report crystal clock. Though it can be
* highly accurately calculated from CPU base frequency
*/
auto numerator = ebx;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: no auto

@heatd
Copy link
Owner

heatd commented Nov 30, 2024

Also, please rebase on top of master!

@heatd
Copy link
Owner

heatd commented Nov 30, 2024

You have conflicts with master after my latest merge. Also small nit: s/x86_64/x86/

@petershh petershh changed the title x86_64: implement CPUID 15h support x86: implement CPUID 15h support Dec 1, 2024
@heatd
Copy link
Owner

heatd commented Dec 1, 2024

I renamed cpu to bootcpu_info, btw

This allows us to get TSC and APIC rates without calibration on
processors supporting this leaf.

Signed-off-by: Peter Shkenev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants