Re: [PATCH] x86/mm: Check cc_vendor when printing memory encryption info

[Jeremi Piotrowski]

On 09/11/2023 17:50, Dave Hansen wrote:
> On 11/9/23 08:35, Jeremi Piotrowski wrote:
>> On 09/11/2023 17:25, Dave Hansen wrote:
>>> On 11/9/23 08:14, Jeremi Piotrowski wrote:
>>> ...
>>>>  	pr_info("Memory Encryption Features active:");
>>>>  
>>>> -	if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) {
>>>> +	if (cc_vendor == CC_VENDOR_INTEL) {
>>>>  		pr_cont(" Intel TDX\n");
>>>>  		return;
>>>>  	}
>>>
>>> Why aren't these guests setting X86_FEATURE_TDX_GUEST?
>>
>> They could if we can confirm that the code gated behind
>> cpu_feature_enabled(X86_FEATURE_TDX_GUEST) is correct when running with TD partitioning.
> 
> Let me elaborate a bit on my question.
> 
> X86_FEATURE_TDX_GUEST is set based on some specific gunk that shows up
> in CPUID in TDX guests.  I *believe* it's in one of the CPUID leaves
> that the VMM has no control over.> 

Right - you're talking about TDX_CPUID_LEAF_ID 0x21.

> So, first, what in practice is keeping tdx_early_init() from running on
> these "TD partitioning" systems?  Because it's either not running, or
> I'm misreading the code horribly.
> 

You're right, it is not running in this case. Not super familiar with TD partitioning,
but as I understand it in TD partitioning Linux runs as a kind of L2 TD guest with the
paravisor acting as the L1 TD VM(M). tdx_early_init() changes kernel behavior with the
assumption that it can talk directly to the TD module or change page visibility in a
certain way, instead of talking to a paravisor. So that CPUID is hidden to prevent this.
Otherwise tdx_early_init() would need to be modified to check "am I running with TD
partitioning and if so - switch to other implementations".

>> It still makes sense to have these prints based on the cc_xxx abstractions.
> 
> I'm not sure I'm following.
> 
> For instance, let's say that someone came up with a nutty reason to do
> MKTME in Linux.  We'd need a host-side contraption that sets
> CC_ATTR_MEM_ENCRYPT and so forth.  It would also, obviously, set
> cc_vendor=CC_VENDOR_INTEL just like host-side SME sets
> cc_vendor=CC_VENDOR_AMD.
> 
> Then we'd have to go back and disentangle all the places where we
> assumed CC_VENDOR_INTEL==TDX.

We're not baking that generalization into any other place, but when printing
coco features here it made sense to me to make the shortcut. I can also post
a v2 that is more accurate:

    if (cc_vendor == CC_VENDOR_INTEL) {
            pr_cont(" Intel");
            if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
                    pr_cont(" TDX\n");
            return;
    }

Dexuan and Michael would like to see callbacks added that allow kernel code
to more accurately report coco features when a paravisor is involved.

> 
> That, combined with this patch's apparent disregard for why
> X86_FEATURE_TDX_GUEST isn't getting set makes me think that the big
> picture was not considered here and this patch represents the quickest
> hack to get the right spew out to dmesg that is desired.

It's not disregard, the way the kernel behaves in this case is correct except
for the error in reporting CPU vendor. Users care about seeing the correct
information in dmesg.

Setting X86_FEATURE_TDX_GUEST might be interesting for a different reason:
so that we also see TDX in /proc/cpuinfo. But then we would want to consider
whether we should add a feature for SNP_GUEST, or else every platform reports
things differently. The cc_platform stuff in the kernel was introduced to avoid
this kind of differentiation.

Jeremi