ring icon indicating copy to clipboard operation
ring copied to clipboard

cpu: Move some of the CPUID logic to Rust.

Open briansmith opened this issue 4 years ago • 3 comments

Separate the generic CPUID logic from the OpenSSL-specific logic for constructing OPENSSL_ia32cap_P. Rewrite the generic logic in Rust.

briansmith avatar Dec 14 '21 01:12 briansmith

@ShadowJonathan PTAL. This demonstrates some of the non-technical difficulties (license) with the process of replacing the remaining C code with Rust.

briansmith avatar Dec 14 '21 01:12 briansmith

Codecov Report

Merging #1437 (cfabb61) into main (08fcf4a) will increase coverage by 0.00%. The diff coverage is 91.89%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1437   +/-   ##
=======================================
  Coverage   93.02%   93.02%           
=======================================
  Files         127      127           
  Lines       18227    18214   -13     
  Branches      195      190    -5     
=======================================
- Hits        16956    16944   -12     
- Misses       1237     1241    +4     
+ Partials       34       29    -5     
Impacted Files Coverage Δ
src/cpu/intel.rs 93.33% <89.65%> (-6.67%) :arrow_down:
crypto/cpu-intel.c 51.85% <100.00%> (-18.64%) :arrow_down:
src/cpu.rs 100.00% <100.00%> (ø)
src/io/der_writer.rs 97.82% <0.00%> (-2.18%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 08fcf4a...cfabb61. Read the comment docs.

codecov[bot] avatar Dec 14 '21 02:12 codecov[bot]

This demonstrates some of the non-technical difficulties (license) with the process of replacing the remaining C code with Rust.

I'm not seeing those difficulties in action in this PR necessarily, could you explain them to me?

Edit: Ah nevermind, i see some of it, what is the idea behind transliterating that bit of code while preserving the license? I don't have a good idea of how such a process would happen, so that's why im asking.

ShadowJonathan avatar Dec 14 '21 12:12 ShadowJonathan

Closing this. The original idea was to separate the "obvious" and non-OpenSSL/BoringSSL-specific bits from the OpenSSL/BoringSSL-specific bits. But on its own it doesn't really accomplish much. Also testing the logic of is_intel and similar is tricky when we don't all reliably have access to Intel (or AMD) CPUs.

I think the right way to do this is to have a safe abstraction for the CPUID instruction. But then using our safe abstraction would differ from what BoringSSL does because it unconditionally queries CPUID leaf 0 and leaf 1. So then we have to choose between the safe abstraction or doing exactly what BoringSSL does. In order to get away from doing exactly what BoringSSL does we need to have more (all?) of the CPU feature detection logic in Rust and none of it in assembly. Then we'll know enough about exactly what the assembly code is doing to determine how to deal with situations where we can't safely query leaf 0 and/or 1.

briansmith avatar Oct 09 '23 19:10 briansmith