Remove RawCpuid

[zulinx86]

Changes

• Add a bidirectional direct conversion between kvm_bindings::CpuId and Cpuid.
• Remove RawCpuid.

Reason

RawCpuid type was relayed as an intermediate type when converting between kvm_bindings::CpuId and Cpuid. By deleting RawCpuid, we can reduce a bunch of lines and unsafe codes.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following
Developer Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

• [ ] If a specific issue led to this PR, this PR closes the issue.
• The description of changes is clear and encompassing.
• [ ] Any required documentation changes (code and docs) are included in this PR.
• [ ] API changes follow the Runbook for Firecracker API changes.
• [ ] User-facing changes are mentioned in CHANGELOG.md.
• All added/changed functionality is tested.
• [ ] New TODOs link to an issue.
• Commits meet contribution quality standards.

---

• [ ] This functionality cannot be added in rust-vmm.

[JonathanWoollett-Light]

I do not like including the intel and amd in the function names, this is something I feel is more elegantly implemented by including this information in a type, hence the IntelCpuid and AmdCpuid types. This makes sense to me as these types have separate distinct functionality. In the proposed implementation here it would be possible to call normalize_intel on an AMD CPUID.

To avoid this would require something like:

impl Cpuid {
    pub fn normalize(&self) -> Result<(),NormalizeCpuidError> {
        match self.manufacturer() {
            CpuidManufacturer::Intel => self.normalize_intel()?,
            CpuidManufacturer::Amd => self.normalize_amd()?
        }
    }
    fn normalize_intel(&self) -> Result<(),IntelNormalizeCpuidError> { /* .. */ }
    fn normalize_amd(&self) -> Result<(),AmdNormalizeCpuidError> { /* .. */ }
}

but this is much worse in my opinion, it embeds a check inside normalize, a user may already be doing duplicating work (this check require matching the manufacturer id string from within cpuid, and itself can error, so we also duplicate the error type variants in our returned error type, although only 1 of them is ever likely to occur) e.g.

self.normalize()?;
match self.manufacturer() {
    CpuidManufacturer::Intel => {
        /* some intel specific work */
    }
    CpuidManufacturer::Amd => {
        /* some amd specific work */  
    }
}

and it force all functions which only work for 1 manufacturer to have to themselves check the manufacturer of CPUID (which leads to quite a bit of duplicated work), and it forces these functions to handle a larger combined NormalizeCpuidError that has both the error types of AMD and Intel when they may only care about Intel or AMD e.g.

fn my_function(cpuid: &Cpuid) -> Result<(),MyFunctionError> {
    match cpuid {
        // Here `MyFunctionError` is defined as bigger more complex type
        // with variants from `AmdNormalizeError` when in practicality it can
        // never be constructed with these variants being selected.
        Cpuid::Intel(intel_cpuid) => intel_cpuid.normalize().map_err(MyFunctionError),
        Cpuid::Amd(amd_cpuid) => Err(MyFunctionError::Amd)
    }
}

I do not think it makes sense to remove IntelCpuid or AmdCpuid as they allow for a better implementation.

My concerns here are only exaggerated further when we consider implementing compatibility checking and serialization/de-serialization with named keys and fields.

[zulinx86]

I do not like including the intel and amd in the function names, this is something I feel is more elegantly implemented by including this information in a type, hence the IntelCpuid and AmdCpuid types. This makes sense to me as these types have separate distinct functionality. In the proposed implementation here it would be possible to call normalize_intel on an AMD CPUID.

To avoid this would require something like:

impl Cpuid {
    pub fn normalize(&self) -> Result<(),NormalizeCpuidError> {
        match self.manufacturer() {
            CpuidManufacturer::Intel => self.normalize_intel()?,
            CpuidManufacturer::Amd => self.normalize_amd()?
        }
    }
    fn normalize_intel(&self) -> Result<(),IntelNormalizeCpuidError> { /* .. */ }
    fn normalize_amd(&self) -> Result<(),AmdNormalizeCpuidError> { /* .. */ }
}

but this is much worse in my opinion, it embeds a check inside normalize, a user may already be doing duplicating work (this check require matching the manufacturer id string from within cpuid, and itself can error, so we also duplicate the error type variants in our returned error type, although only 1 of them is ever likely to occur) e.g.

self.normalize()?;
match self.manufacturer() {
    CpuidManufacturer::Intel => {
        /* some intel specific work */
    }
    CpuidManufacturer::Amd => {
        /* some amd specific work */  
    }
}

and it force all functions which only work for 1 manufacturer to have to themselves check the manufacturer of CPUID (which leads to quite a bit of duplicated work), and it forces these functions to handle a larger combined NormalizeCpuidError that has both the error types of AMD and Intel when they may only care about Intel or AMD e.g.

fn my_function(cpuid: &Cpuid) -> Result<(),MyFunctionError> {
    match cpuid {
        // Here `MyFunctionError` is defined as bigger more complex type
        // with variants from `AmdNormalizeError` when in practicality it can
        // never be constructed with these variants being selected.
        Cpuid::Intel(intel_cpuid) => intel_cpuid.normalize().map_err(MyFunctionError),
        Cpuid::Amd(amd_cpuid) => Err(MyFunctionError::Amd)
    }
}

I do not think it makes sense to remove IntelCpuid or AmdCpuid as they allow for a better implementation.

My concerns here are only exaggerated further when we consider implementing compatibility checking and serialization/de-serialization with named keys and fields.

Thanks for sharing your good opinion! Let's make this PR focus on the removal of RawCpuid here, and discuss and consider the removal of IntelCpuid and AmdCpuid in a follow-up PR.