On 2021-12-15 11:49:34 -0600, Michael Roth wrote:

I think in the greater context of consolidating all the SME/SEV setup
and re-using code, this helper stands a high chance of eventually becoming
something more along the lines of sme_sev_parse_cpuid(), since otherwise
we'd end up re-introducing multiple helpers to parse the same 0x8000001F
fields if we ever need to process any of the other fields advertised in
there. Given that, it makes sense to reserve the return value as an
indication that either SEV or SME are enabled, and then have a
pass-by-pointer parameters list to collect the individual feature
bits/encryption mask for cases where SEV/SME are enabled, which are only
treated as valid if sme_sev_parse_cpuid() returns 0.

So Venu's original approach of passing the encryption mask by pointer
seems a little closer toward that end, but I also agree Tom's approach
is cleaner for the current code base, so I'm fine either way, just
figured I'd mention this.

I think needing to pass in the SME/SEV CPUID bits to tell the helper when
to parse encryption bit and when not to is a little bit awkward though.
If there's some agreement that this will ultimately serve the purpose of
handling all (or most) of SME/SEV-related CPUID parsing, then the caller
shouldn't really need to be aware of any individual bit positions.
Maybe a bool could handle that instead, e.g.:

  int get_me_bit(bool sev_only, ...)

  or

  int sme_sev_parse_cpuid(bool sev_only, ...)

where for boot/compressed sev_only=true, for kernel proper sev_only=false.

I can implement it this way too. But I am wondering if having a
boolean argument limits us from handling any future additions to the
bit positions.

Boris & Tom, which implementation would you prefer?

Venu