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

No CPUInfo for all ARM #291

Closed
bluecmd opened this issue May 12, 2020 · 10 comments · Fixed by #292
Closed

No CPUInfo for all ARM #291

bluecmd opened this issue May 12, 2020 · 10 comments · Fixed by #292

Comments

@bluecmd
Copy link
Contributor

bluecmd commented May 12, 2020

Hi,

There has been changes in the CpuInfo part in the recent past, and e.g. #290 adds some support for some ARM CPUs. However, that's only for v6.

Could we maybe enable parseCPUInfoARM for all ARM versions?

@discordianfish
Copy link
Member

@bluecmd Makes sense, feel free to submit a PR to add all other ARM versions (assuming they have the same cpuinfo format)

@bluecmd
Copy link
Contributor Author

bluecmd commented May 12, 2020

Well, I can make sure to test it for a v5 ARM - but it seems unlikely to me that the format would be a function of the version of the instruction set. If there are ARM CPUs that have a separate format, likely they need to be handled by some other means than filtering by ARM version.

@discordianfish
Copy link
Member

Yeah I think it's reasonable to enable this on all arm versions. If the format should be different, it will just return an error which isn't worse than have a compile error I guess.

@bluecmd
Copy link
Contributor Author

bluecmd commented May 12, 2020

So, it seems like what you have currently in tree is something quite special, ARM-wise.

Right now parseCPUInfoARM works for only a format that was used 8 years ago and has since v3.8-rc1 been replaced with a format that looks like this:

/# cat /proc/cpuinfo 
processor	: 0
model name	: ARM926EJ-S rev 5 (v5l)
BogoMIPS	: 48.00
Features	: swp half thumb fastmult edsp java 
CPU implementer	: 0x41
CPU architecture: 5TEJ
CPU variant	: 0x0
CPU part	: 0x926
CPU revision	: 5

Hardware	: Generic DT based system
Revision	: 0000
Serial		: 0000000000000000

Problematic code is here:

procfs/cpuinfo.go

Lines 193 to 195 in 7a44272

if !strings.HasPrefix(firstLine, "Processor") || !strings.Contains(firstLine, ":") {
return nil, errors.New("invalid cpuinfo file: " + firstLine)
}

This "new" format is quite close to the x86 format, FWIW.

I can help fix this by looking at the kernel version and either error out, or parse the older format. And I will of course improve the tests.

Thoughts?

@discordianfish
Copy link
Member

Uh.. hrm, that's unfortunate. @pgier thoughts?

@bluecmd
Copy link
Contributor Author

bluecmd commented May 12, 2020

My apologizes, I got the commit wrong - it seems this was changed 8 years ago in this commit. So from v3.8-rc1 and later. I have updated my post above accordingly.

@discordianfish
Copy link
Member

It's close but not identical to x86 I assume?

@bluecmd
Copy link
Contributor Author

bluecmd commented May 14, 2020

Right, looking at x86 you're extracting many x86 specific attributes that ARM does not have, and likewise ARM has some things that x86 does not have - like "CPU architecture".

Do you wish to re-use this issue, or should we move to a new issue for upgrading the formatting for ARM?

@discordianfish
Copy link
Member

New issue would make sense I think

@bluecmd
Copy link
Contributor Author

bluecmd commented May 15, 2020

Done, #294

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants