cpuid crate refactoring - part 2 #956

serban300 · 2019-02-21T13:55:15Z

Signed-off-by: Serban Iorga seriorga@amazon.com

Issue #, if available: #851

Description of changes:
- move leaf handlers into separate functions
- test leaf handlers individually
- move all the templates into a folder

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

andreeaflorescu

I don't think it is a good idea to have one module for each leaf. The code it is too sparse and hard to follow. What is the benefit of that? I think it is good practice to explain the why in PRs for cases where the need for the change is not obvious.

If you create a template module, there is no need to name the individual files c3_template and t2_template. You can name them just t2 and c3 as they are going to be imported as use cpuid::template::{c3, t2}.

andreeaflorescu · 2019-02-21T14:51:28Z

cpuid/src/cpu_leaf/leaf_0x1/engine.rs

+    Ok(max_addressable_lcpu as u8)
+}
+
+pub struct Engine {


What is Engine suppose to be?

LeafEngine.process is supposed to be the handler that configures each leaf. There was a switch-case in filter_cpuid that used to do that.

Engine is just an implementation of LeafEngine

serban300 · 2019-02-22T08:22:14Z

I don't think it is a good idea to have one module for each leaf. The code it is too sparse and hard to follow. What is the benefit of that? I think it is good practice to explain the why in PRs for cases where the need for the change is not obvious.

If you create a template module, there is no need to name the individual files c3_template and t2_template. You can name them just t2 and c3 as they are going to be imported as use cpuid::template::{c3, t2}.

I agree with naming the template files t2 and c3

By splitting the cpu_leaf logic I wanted to avoid having a file that is bigger then 1000 lines and that contains multiple test modules (one per LeafEngine) and uses multiple imbrication layers. I tried to do it like that and I couldn't follow the code. Also in the future I would like to maybe be able to split the leafs into multiple folders by cpu vendor if needed. For example:

common
- leaf_bstr.rs
intel
- leaf_0x4.rs
amd
- leaf_0x8000001d.rs

In general I think that modularity is an asset, not a liability. Anyway, let's discuss this offline.

andreeaflorescu

At a first glance, LGTM. I will do a in-depth review tomorrow to check all the leafs.

andreeaflorescu · 2019-02-27T15:41:46Z

cpuid/src/transformer/common.rs

+        assert!(get_max_cpus_per_package(u8::max_value()).is_err());
+    }
+
+    fn test_update_feature_info_entry(cpu_count: u8, expected_htt: bool) {


nit: you can name this check_update.... because we are prefixing with test on the test functions.
Same for the other test helper functions.

dianpopa · 2019-03-11T16:39:28Z

@serban300 It would be a lot easier to review if the changes got split into multiple commits. I see that in the Desctiption of changes from the conversation section you are enumerating three directions this PR heads towards. Would it be feasible to do a commit for each of them?

serban300 · 2019-03-11T16:56:13Z

@serban300 It would be a lot easier to review if the changes got split into multiple commits. I see that in the Desctiption of changes from the conversation section you are enumerating three directions this PR heads towards. Would it be feasible to do a commit for each of them?

The first 2 points are connected so I don't think it's feasible to split them into separate commits.
I could try to split the 3rd point into a separate commit, but I don't know if it would make the review much easier since that change only involves moving 2 files into a subfolder.

dhrgit · 2019-03-12T11:24:51Z

cpuid/src/cpu_leaf.rs

@@ -81,11 +81,11 @@ pub mod leaf_0x4 {
            msb_index: 7,
            lsb_index: 5,
        };
-        pub const MAX_ADDR_IDS_SHARING_CACHE_BITRANGE: BitRange = BitRange {
+        pub const MAX_CPUS_PER_CORE: BitRange = BitRange {


Nit-ish: this looks a bit cumbersome to me. I mean, it's four lines of code, just to specify a region. What do you think about having a constructor there for BitRange, such that you could slim these definitions down to something like:

pub const MAX_CPUS_PER_CORE: BitRange = BitRange::new(25, 14);

Alternatively, a macro could be used for keeping the definitions short. So that you could do something like:

bit_range!(MAX_CPUS_PER_CORE, 25, 14)

Also nit-ish: the _index suffix seems superfluous to me. The bit part in MSB or LSB has always implied an index, not a value.

BitRange::new(25, 14); Wouldn't work because you can't call functions when initializing a const

I like the macro idea but I would prefer declaring the variable outside of the macro:

pub const MAX_CPUS_PER_CORE_BITRANGE: BitRange = bit_range!(25, 14);

I don't know, I think MSB or LSB is opened to interpretation. It could refer either to the value of the bit or to the index. I would prefer to keep the _index in the name

Oh, right - the const / function thingy flew right past me. Anyway, what I was getting at was a clear / easy / quick way to scan the source and see what those constants meant.

dhrgit · 2019-03-12T14:20:09Z

cpuid/src/transformer/mod.rs

+    if let Ok(host_bstr) = BrandString::from_host_cpuid() {
+        if host_bstr.starts_with(b"Intel") {
+            if let Some(freq) = host_bstr.find_freq() {
+                let mut v3 = vec![];


We shouldn't need an extra Vec here (heap allocated). How about just:

bstr.push_bytes(b" @ ").unwrap(); bstr.push_bytes(freq) .expect("Unexpected frequency information in host CPUID");

Nice. Done.

dhrgit · 2019-03-12T14:21:15Z

cpuid/src/transformer/mod.rs

+///
+/// This is safe because we know DEFAULT_BRAND_STRING to hold valid data
+/// (allowed length and holding only valid ASCII chars).
+pub fn get_brand_string() -> BrandString {


This seems more like a create_ / build_ / make_, rather than a get_.

dhrgit · 2019-03-12T14:38:47Z

cpuid/src/transformer/common.rs

@@ -0,0 +1,112 @@
+// Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.


Why do you use both a common.rs and a mod.rs? The latter seems empty.

common.rs contains the common cpuid transformer functions like update_feature_info_entry
mod.rs contains the basics for the module like the VmSpec struct, the Error enum, etc.

Neither one is empty.

- move leaf handlers into separate functions - test leaf handlers individually - move all the templates into a folder Signed-off-by: Serban Iorga <seriorga@amazon.com>

serban300 · 2019-03-13T13:44:30Z

@dhrgit I addressed all the comments. Please take another look.

serban300 added Codebase: Refactoring Priority: High Indicates than an issue or pull request should be resolved ahead of issues or pull requests labelled labels Feb 21, 2019

serban300 self-assigned this Feb 21, 2019

serban300 requested review from andreeaflorescu and dhrgit February 21, 2019 13:55

serban300 force-pushed the serban300-cpuid-refactoring branch from 82d2d56 to 0aafc1d Compare February 21, 2019 14:13

serban300 added this to the AMD Support milestone Feb 21, 2019

andreeaflorescu reviewed Feb 21, 2019

View reviewed changes

serban300 force-pushed the serban300-cpuid-refactoring branch 3 times, most recently from 20f9be1 to 9851ee9 Compare February 25, 2019 16:47

andreeaflorescu reviewed Feb 27, 2019

View reviewed changes

serban300 force-pushed the serban300-cpuid-refactoring branch from 9851ee9 to 23affd3 Compare March 11, 2019 15:11

dhrgit reviewed Mar 12, 2019

View reviewed changes

serban300 force-pushed the serban300-cpuid-refactoring branch from 23affd3 to e1707c0 Compare March 13, 2019 10:59

cpuid crate refactoring - part 2

d3d13ef

- move leaf handlers into separate functions - test leaf handlers individually - move all the templates into a folder Signed-off-by: Serban Iorga <seriorga@amazon.com>

acatangiu previously approved these changes Mar 13, 2019

View reviewed changes

serban300 dismissed acatangiu’s stale review via d3d13ef March 13, 2019 11:42

serban300 force-pushed the serban300-cpuid-refactoring branch from e1707c0 to d3d13ef Compare March 13, 2019 11:42

dhrgit approved these changes Mar 13, 2019

View reviewed changes

Merge branch 'master' into serban300-cpuid-refactoring

d06809e

acatangiu approved these changes Mar 13, 2019

View reviewed changes

acatangiu merged commit 8e2640a into firecracker-microvm:master Mar 13, 2019

serban300 deleted the serban300-cpuid-refactoring branch May 10, 2021 07:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cpuid crate refactoring - part 2 #956

cpuid crate refactoring - part 2 #956

serban300 commented Feb 21, 2019 •

edited

Loading

andreeaflorescu left a comment

andreeaflorescu Feb 21, 2019

serban300 Feb 22, 2019

serban300 commented Feb 22, 2019

andreeaflorescu left a comment

andreeaflorescu Feb 27, 2019

serban300 Mar 11, 2019

dianpopa commented Mar 11, 2019

serban300 commented Mar 11, 2019

dhrgit Mar 12, 2019

serban300 Mar 13, 2019

dhrgit Mar 13, 2019

dhrgit Mar 12, 2019

serban300 Mar 13, 2019

dhrgit Mar 12, 2019

serban300 Mar 13, 2019

dhrgit Mar 12, 2019

serban300 Mar 13, 2019

serban300 commented Mar 13, 2019

		@@ -0,0 +1,112 @@
		// Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.

cpuid crate refactoring - part 2 #956

cpuid crate refactoring - part 2 #956

Conversation

serban300 commented Feb 21, 2019 • edited Loading

andreeaflorescu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serban300 commented Feb 22, 2019

andreeaflorescu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dianpopa commented Mar 11, 2019

serban300 commented Mar 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serban300 commented Mar 13, 2019

serban300 commented Feb 21, 2019 •

edited

Loading