Date:   Tue, 29 Jun 2021 14:47:59 -0500
From:   Tom Lendacky <thomas.lendacky@....com>
To:     "Kuppuswamy, Sathyanarayanan" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Andy Lutomirski <luto@...nel.org>
Cc:     Peter H Anvin <hpa@...or.com>, Dave Hansen <dave.hansen@...el.com>,
        Tony Luck <tony.luck@...el.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Kirill Shutemov <kirill.shutemov@...ux.intel.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Kuppuswamy Sathyanarayanan <knsathya@...nel.org>,
        x86@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 04/11] x86: Introduce generic protected guest
 abstraction

On 6/28/21 2:14 PM, Kuppuswamy, Sathyanarayanan wrote:
> 
> 
> On 6/28/21 10:52 AM, Tom Lendacky wrote:
>> On 6/18/21 5:57 PM, Kuppuswamy Sathyanarayanan wrote:

>>> +static inline bool prot_guest_has(unsigned long flag)
>>> +{
>>> +    if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
>>> +        return tdx_protected_guest_has(flag);
>>> +    else if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
>>> +        return sev_protected_guest_has(flag);
>>
>> So as I think about this, I don't think this will work if the hypervisor
>> decides to change the vendor name, right?
> 
> For TDX guest, vendor name cannot be changed. It is set by TDX module and
> it is fixed as per TDX module spec.
> 
>>
>> And doesn't TDX supply "IntelTDX    " as a signature. I don't see where
>> the signature is used to set the CPU vendor to X86_VENDOR_INTEL.>
> We don't need to specially handle it for TDX. Generic early_identify_cpu()
> will
> set boot_cpu_data.x86_vendor as X86_VENDOR_INTEL for TDX guest. I think it is
> based on Intel in vendor string.

Hmmm..., I must be missing something then. I thought early_identify_cpu()
will read the signature, which would be "IntelTDX    ", right? Then that
is be compared against the structs that register via cpu_dev_register()
which would contain the x86_vendor value. I don't see anything registering
with the "IndexTDX    " signature so I don't know how you'll get an
x86_vendor value of X86_VENDOR_INTEL.

I'm probably missing something there, but it shouldn't matter for this
routine going forward.

> 
>>
>> The current SEV checks to set sev_status, which is used by sme_active(),
>> sev_active, etc.) are based on the max leaf and CPUID bits, but not a
>> CPUID vendor check.
>>
> 
> You also set x86_vendor id as AMD based on SEV checks?

No, we don't.

> 
>> So maybe we can keep the prot_guest_has() but I think it will have to be a
>> common routine, with a "switch" statement that has supporting case element
>> that check for "sev_active() || static_cpu_has(X86_FEATURE_TDX_GUEST)",
>> etc.
> 
>>>   }
>>> +
>>> +bool sev_protected_guest_has(unsigned long flag)
>>> +{
>>> +    switch (flag) {
>>> +    case PR_GUEST_MEM_ENCRYPT:
>>> +    case PR_GUEST_MEM_ENCRYPT_ACTIVE:
>>> +    case PR_GUEST_UNROLL_STRING_IO:
>>> +    case PR_GUEST_HOST_MEM_ENCRYPT:
>>> +        return true;
>>
>> This will need to be fixed up because this function will be called for
>> baremetal and legacy guests and those properties aren't true for those
>> situations. Something like (although I'm unsure of the difference between
>> PR_GUEST_MEM_ENCRYPT and PR_GUEST_MEM_ENCRYPT_ACTIVE):
> 
> MEM_ENCRYPT_ACTIVE is suggested for mem_encrypt_active() case (I think it
> means some sort of encryption is active).
> 
> PR_GUEST_MEM_ENCRYPT means guest supports memory encryption (sev_active()
> case).

Yeah, this is the problem with the name having guest in everything when
there are host and guest scenarios for AMD.

We have PR_GUEST_HOST_MEM_ENCRYPT but it would look strange to have
PR_GUEST_GUEST_MEM_ENCRYPT.

> 
> Yes, I can include following changes in next version.
> 
>>
>>     case PR_GUEST_MEM_ENCRYPT:
>>     case PR_GUEST_MEM_ENCRYPT_ACTIVE:
>>         return sev_active();
>>     case PR_GUEST_UNROLL_STRING_IO:
>>         return sev_active() && !sev_es_active();
>>     case PR_GUEST_HOST_MEM_ENCRYPT:
>>         return sme_active();
>>
>> But you (or I) would have to audit all of the locations where
>> mem_encrypt_active(), sme_active(), sev_active() and sev_es_active() are
>> used, to be sure the right thing is being done. And for bisectability,
>> that should probably be the first patch if you will be invoking
>> prot_guest_has() in the same location as any of the identified functions.
>>
>> Create the new helper and fixup the locations should be one (or more)
>> patches. Then add the TDX support to the helper function as a follow-on
>> patch.
> 
> Can you submit a patch to replace all existing uses cases of
> mem_encrypt_active()
> ,sme_active(), sev_active() and sev_es_active() with prot_guest_has()
> calls? Since
> I cannot test any of these changes for AMD, it would be better if you
> could do it.
> 
> Once you submit a tested version, I can enable these features for TDX and
> test
> and submit it separately.
> 
> This patch can be split as below:
> 
> 1. x86: Introduce generic protected guest abstraction patch (with below
> changes).
>    - Remove all PR_GUEST flags in sev_protected_guest_has() and
>      tdx_protected_guest_has().
> 2. Patch from you to use prot_guest_has() for AMD code and enable relevant
>    PR_GUEST flags in sev_protected_guest_has().
> 3. Patch from me to us prot_guest_has() for TDX cases and enable relevant
>    PR_GUEST flags in tdx_protected_guest_has().
> 
> Agree?

So I can work on a pre-patch series. It will be purely a replacement for
the current SME/SEV calls. You'll need to add all of the TDX support in a
subsequent patch in the TDX series. Given this is a pre-patch, I will
probably reset the flag values slightly and work on the names to be less
confusing.

Thanks,
Tom

Powered by blists - more mailing lists