Page MenuHomeWildfire Games

Handle unknown APIC IDs in the ACPI SRAT
ClosedPublic

Authored by Itms on Mar 17 2019, 11:28 AM.

Details

Reviewers
Imarok
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP24326: Handle unknown APIC IDs in the ACPI SRAT
Trac Tickets
#5412
Summary

The SRAT table can contain information about processors that 0 A.D. doesn't have access to. Since processors are listed by calling the CPUID instruction on each processor, this list is not necessarily complete (and we really don't care about cores we don't have access to).

One part of the hardware detection code assumes that the processors in the SRAT are supposed to be known, which is not always the case (we have access to the whole SRAT table). This patch removes that assumption, and adds an early return for aesthetics.

Test Plan

Test that the game starts without crashing in this situation, outputting a useful warning for possible future bugs on hardware that has this SRAT/CPUID discrepancy.

Test that the game still starts without change on all other hardware.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Itms created this revision.Mar 17 2019, 11:28 AM
Vulcan added a subscriber: Vulcan.Mar 17 2019, 11:34 AM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1100/display/redirect

Stan added a reviewer: Restricted Owners Package.Mar 17 2019, 12:08 PM
Imarok added a subscriber: Imarok.Mar 20 2019, 11:41 AM

Game works for me. (Intel Xeon with Ubuntu Gnome)

vladislavbelov added inline comments.
source/lib/sysdep/arch/x86_x64/apic.cpp
115 ↗(On Diff #7557)

Don't we need a check like above?

Itms added inline comments.Mar 27 2019, 12:08 PM
source/lib/sysdep/arch/x86_x64/apic.cpp
115 ↗(On Diff #7557)

If apicInitState is negative, it's just that we created false apic ids to describe the processors that we know about, in which case most of the processor-related hardware detection is just skipped.

Adding a check like above wouldn't hurt I suppose, but it's not needed.

Game works for me. (Ryzen 3700X, Windows)
Seems to, that the patch is needed for some AMD Threaripper CPUs.

Imarok accepted this revision.Nov 29 2020, 2:37 PM

Looks legit.
Sadly, we can't check if this fix really fixes the issue because we lack the hardware.

This revision is now accepted and ready to land.Nov 29 2020, 2:37 PM
This revision was automatically updated to reflect the committed changes.