Message-ID: <20211215201734.glq5gsle6crj25sf@amd.com>
Date:   Wed, 15 Dec 2021 14:17:34 -0600
From:   Michael Roth <michael.roth@....com>
To:     Borislav Petkov <bp@...en8.de>
CC:     Venu Busireddy <venu.busireddy@...cle.com>,
        Tom Lendacky <thomas.lendacky@....com>,
        Brijesh Singh <brijesh.singh@....com>, <x86@...nel.org>,
        <linux-kernel@...r.kernel.org>, <kvm@...r.kernel.org>,
        <linux-efi@...r.kernel.org>, <platform-driver-x86@...r.kernel.org>,
        <linux-coco@...ts.linux.dev>, <linux-mm@...ck.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Joerg Roedel <jroedel@...e.de>,
        "H. Peter Anvin" <hpa@...or.com>, Ard Biesheuvel <ardb@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Jim Mattson <jmattson@...gle.com>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Sergio Lopez <slp@...hat.com>, Peter Gonda <pgonda@...gle.com>,
        Peter Zijlstra <peterz@...radead.org>,
        "Srinivas Pandruvada" <srinivas.pandruvada@...ux.intel.com>,
        David Rientjes <rientjes@...gle.com>,
        Dov Murik <dovmurik@...ux.ibm.com>,
        "Tobin Feldman-Fitzthum" <tobin@....com>,
        Vlastimil Babka <vbabka@...e.cz>,
        "Kirill A . Shutemov" <kirill@...temov.name>,
        Andi Kleen <ak@...ux.intel.com>,
        "Dr . David Alan Gilbert" <dgilbert@...hat.com>,
        <tony.luck@...el.com>, <marcorr@...gle.com>,
        <sathyanarayanan.kuppuswamy@...ux.intel.com>
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&amp;data=04%7C01%7Cmichael.roth%40amd.com%7C6a28b961ef1441ed08f908d9bff970ea%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637751900351173552%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=nnCrpsw9%2FYlmhK1Xbx5y5vUScVsEOQeU%2F%2FTCmBMQ3v4%3D&amp;reserved=0

Powered by blists - more mailing lists