Messages in this thread | | | Date | Wed, 15 Dec 2021 14:17:34 -0600 | From | Michael Roth <> | Subject | Re: [PATCH v8 01/40] x86/compressed/64: detect/setup SEV/SME features earlier in boot |
| |
On Wed, Dec 15, 2021 at 07:33:47PM +0100, Borislav Petkov wrote: > On Wed, Dec 15, 2021 at 12:17:44PM -0600, Venu Busireddy wrote: > > Boris & Tom, which implementation would you prefer? > > I'd like to see how that sme_sev_parse_cpuid() would look like. And that > function should be called sev_parse_cpuid(), btw. > > Because if that function turns out to be a subset of your suggestion, > functionality-wise, then we should save us the churn and simply do one > generic helper.
I was actually thinking this proposed sev_parse_cpuid() helper would be a superset of what Venu currently has implemented. E.g. Venu's most recent patch does:
sev_enable(): unsigned int me_bit_pos;
me_bit_pos = get_me_bit(AMD_SEV_BIT) if (!me_bit_pos) return;
...
Let's say in the future there's need to also grab say, the VTE bit. We could introduce a new helper, get_vte_bit() that re-does all the 0x80000000-0x8000001F range checks, some sanity checks that SEV is set if VTE bit is set, and then now have a nice single-purpose helper that duplicates similar checks in get_me_bit(), or we could avoid the duplication by expanding get_me_bit() so it could be used something like:
me_bit_pos = get_me_bit(AMD_SEV_BIT, &vte_enabled)
at which point it makes more sense to just have it be a more generic helper, called via:
ret = sev_parse_cpuid(AMD_SEV_BIT, &me_bit_pos, &vte_enabled)
i.e. Venu's original patch basically, but with the helper function renamed.
and if fields are added in the future:
sev_parse_cpuid(AMD_SEV_BIT, &me_bit_pos, &vte_enabled, &new_feature_enabled, etc..)
or if that eventually becomes unwieldly it could later be changed to return a feature mask.
> > Btw 2, that helper should be in arch/x86/kernel/sev-shared.c so that it > gets shared by both kernel stages instead having an inline function in > some random header. > > Btw 3, I'm not crazy about the feature testing with the @features param > either. Maybe that function should return the eYx register directly, > like the cpuid_eYx() variants in the kernel do, where Y in { a, b, c, d > }. > > The caller can than do its own testing: > > eax = sev_parse_cpuid(RET_EAX, ...) > if (eax > 0) { > if (eax & BIT(1)) > ... > > Something along those lines, for example.
I think having sev_parse_cpuid() using a more "human-readable" format for reporting features/fields will make it easier to abstract away the nitty-gritty details and reduce that chances for more duplication between boot/compressed and kernel proper in the future. That "human-readable" format could be in the form of a boolean/int parameter list that gets expanded over time as needed (like the above examples), or a higher-level construct like a struct/bitmask/etc. But either way it would be nice to only have to think about specific CPUID bits when looking at sev_parse_cpuid(), and have callers instead rely purely on the sev_parse_cpuid() function prototype/documentation to know what's going on.
> > But I'd have to see a concrete diff from Michael to get a better idea > how that CPUID parsing from the CPUID page is going to look like.
It should look the same with/without CPUID page, since the CPUID page will have already been set up early in sev_enable()/sme_enable() based on the presence of the CC blob via snp_init(), introduced in:
[PATCH v8 31/40] x86/compressed: add SEV-SNP feature detection/setup
Thanks,
Mike
> > Thx. > > -- > Regards/Gruss, > Boris. > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&data=04%7C01%7Cmichael.roth%40amd.com%7C6a28b961ef1441ed08f908d9bff970ea%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637751900351173552%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=nnCrpsw9%2FYlmhK1Xbx5y5vUScVsEOQeU%2F%2FTCmBMQ3v4%3D&reserved=0
| |