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

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

[Jeremi Piotrowski]

Check the value of cc_vendor to see if we're in an Intel TDX protected VM
instead of checking for the TDX_GUEST CPU feature. The rest of the function
already uses the abstractions available in cc_platform.h to check for
confidential computing features. For Intel, cc_vendor is set from
tdx_early_init() or hv_vtom_init(), so the new code correctly handles both
cases. The previous check relied on the Linux-controlled TDX_GUEST CPU feature
which is only set in tdx_early_init().

Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
---
 arch/x86/mm/mem_encrypt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index c290c55b632b..d3bd39aad8b6 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -44,7 +44,7 @@ static void print_mem_encrypt_feature_info(void)
 {
 	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;
 	}
-- 
2.39.2

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

[Dave Hansen]

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?

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

[Jeremi Piotrowski]

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.

It still makes sense to have these prints based on the cc_xxx abstractions.

Jeremi

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

[Dave Hansen]

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.

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.

> 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.

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.

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

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

[kirill.shutemov]

On Thu, Nov 09, 2023 at 07:41:33PM +0100, Jeremi Piotrowski wrote:
> 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.

I think it is wrong place to advertise CoCo features. It better fits into
Intel/AMD-specific code that knows better what it is running on. Inferring
it from generic code has been proved problematic as you showed.

Maybe just remove incorrect info and that's it?

diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index c290c55b632b..f573a97c0524 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -40,42 +40,6 @@ bool force_dma_unencrypted(struct device *dev)
 	return false;
 }
 
-static void print_mem_encrypt_feature_info(void)
-{
-	pr_info("Memory Encryption Features active:");
-
-	if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) {
-		pr_cont(" Intel TDX\n");
-		return;
-	}
-
-	pr_cont(" AMD");
-
-	/* Secure Memory Encryption */
-	if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)) {
-		/*
-		 * SME is mutually exclusive with any of the SEV
-		 * features below.
-		 */
-		pr_cont(" SME\n");
-		return;
-	}
-
-	/* Secure Encrypted Virtualization */
-	if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
-		pr_cont(" SEV");
-
-	/* Encrypted Register State */
-	if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
-		pr_cont(" SEV-ES");
-
-	/* Secure Nested Paging */
-	if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
-		pr_cont(" SEV-SNP");
-
-	pr_cont("\n");
-}
-
 /* Architecture __weak replacement functions */
 void __init mem_encrypt_init(void)
 {
@@ -85,7 +49,7 @@ void __init mem_encrypt_init(void)
 	/* Call into SWIOTLB to update the SWIOTLB DMA buffers */
 	swiotlb_update_mem_attributes();
 
-	print_mem_encrypt_feature_info();
+	pr_info("Memory Encryption is active\n");
 }
 
 void __init mem_encrypt_setup_arch(void)
-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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

[Jeremi Piotrowski]

On 10/11/2023 13:06, kirill.shutemov@linux.intel.com wrote:
> On Thu, Nov 09, 2023 at 07:41:33PM +0100, Jeremi Piotrowski wrote:
>> 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.
> 
> I think it is wrong place to advertise CoCo features. It better fits into
> Intel/AMD-specific code that knows better what it is running on. Inferring
> it from generic code has been proved problematic as you showed.
> 

Let's not make this into a bigger issue than it is. There is a check that does
not cover all cases, and needs to be changed into a different check that covers
all cases. I'd wouldn't call that problematic.

I would be open to moving the function to arch/x86/coco/core.c or providing a
mechanism for cpu/paravisor-specific code to do the reporting (which is what
Dexuan originally proposed [1]).

[1]: https://lore.kernel.org/lkml/20231019062030.3206-1-decui@microsoft.com/

> Maybe just remove incorrect info and that's it?
> 

I disagree, other users and I find the print very useful to see which coco
platform the kernel is running on and which confidential computing features
the kernel detected. I'm willing to fix the code to report correct info.

> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index c290c55b632b..f573a97c0524 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -40,42 +40,6 @@ bool force_dma_unencrypted(struct device *dev)
>  	return false;
>  }
>  
> -static void print_mem_encrypt_feature_info(void)
> -{
> -	pr_info("Memory Encryption Features active:");
> -
> -	if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) {
> -		pr_cont(" Intel TDX\n");
> -		return;
> -	}
> -
> -	pr_cont(" AMD");
> -
> -	/* Secure Memory Encryption */
> -	if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)) {
> -		/*
> -		 * SME is mutually exclusive with any of the SEV
> -		 * features below.
> -		 */
> -		pr_cont(" SME\n");
> -		return;
> -	}
> -
> -	/* Secure Encrypted Virtualization */
> -	if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
> -		pr_cont(" SEV");
> -
> -	/* Encrypted Register State */
> -	if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
> -		pr_cont(" SEV-ES");
> -
> -	/* Secure Nested Paging */
> -	if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
> -		pr_cont(" SEV-SNP");
> -
> -	pr_cont("\n");
> -}
> -
>  /* Architecture __weak replacement functions */
>  void __init mem_encrypt_init(void)
>  {
> @@ -85,7 +49,7 @@ void __init mem_encrypt_init(void)
>  	/* Call into SWIOTLB to update the SWIOTLB DMA buffers */
>  	swiotlb_update_mem_attributes();
>  
> -	print_mem_encrypt_feature_info();
> +	pr_info("Memory Encryption is active\n");
>  }
>  
>  void __init mem_encrypt_setup_arch(void)

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

[kirill.shutemov]

On Fri, Nov 10, 2023 at 01:27:08PM +0100, Jeremi Piotrowski wrote:
> > Maybe just remove incorrect info and that's it?
> > 
> 
> I disagree, other users and I find the print very useful to see which coco
> platform the kernel is running on and which confidential computing features
> the kernel detected. I'm willing to fix the code to report correct info.

For TDX, we already have "tdx: Guest detected" in dmesg. sme_early_init()
can do something similar.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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

[Jeremi Piotrowski]

On 10/11/2023 13:46, kirill.shutemov@linux.intel.com wrote:
> On Fri, Nov 10, 2023 at 01:27:08PM +0100, Jeremi Piotrowski wrote:
>>> Maybe just remove incorrect info and that's it?
>>>
>>
>> I disagree, other users and I find the print very useful to see which coco
>> platform the kernel is running on and which confidential computing features
>> the kernel detected. I'm willing to fix the code to report correct info.
> 
> For TDX, we already have "tdx: Guest detected" in dmesg. sme_early_init()
> can do something similar.
> 

That doesn't cover TDX guests with TD partitioning on Hyper-V.

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

[kirill.shutemov]

On Fri, Nov 10, 2023 at 02:42:31PM +0100, Jeremi Piotrowski wrote:
> On 10/11/2023 13:46, kirill.shutemov@linux.intel.com wrote:
> > On Fri, Nov 10, 2023 at 01:27:08PM +0100, Jeremi Piotrowski wrote:
> >>> Maybe just remove incorrect info and that's it?
> >>>
> >>
> >> I disagree, other users and I find the print very useful to see which coco
> >> platform the kernel is running on and which confidential computing features
> >> the kernel detected. I'm willing to fix the code to report correct info.
> > 
> > For TDX, we already have "tdx: Guest detected" in dmesg. sme_early_init()
> > can do something similar.
> > 
> 
> That doesn't cover TDX guests with TD partitioning on Hyper-V.

I am sure Hyper-V can report what mode it runs.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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

[Jeremi Piotrowski]

On 10/11/2023 19:57, kirill.shutemov@linux.intel.com wrote:
> On Fri, Nov 10, 2023 at 02:42:31PM +0100, Jeremi Piotrowski wrote:
>> On 10/11/2023 13:46, kirill.shutemov@linux.intel.com wrote:
>>> On Fri, Nov 10, 2023 at 01:27:08PM +0100, Jeremi Piotrowski wrote:
>>>>> Maybe just remove incorrect info and that's it?
>>>>>
>>>>
>>>> I disagree, other users and I find the print very useful to see which coco
>>>> platform the kernel is running on and which confidential computing features
>>>> the kernel detected. I'm willing to fix the code to report correct info.
>>>
>>> For TDX, we already have "tdx: Guest detected" in dmesg. sme_early_init()
>>> can do something similar.
>>>
>>
>> That doesn't cover TDX guests with TD partitioning on Hyper-V.
> 
> I am sure Hyper-V can report what mode it runs.
> 

You're right, it could. In that case I would move the AMD print and keep only
"Memory Encryption is active" here.

Lets see whether we find agreement on my new submission first.

Thanks,
Jeremi

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

[Borislav Petkov]

On Thu, Nov 09, 2023 at 07:41:33PM +0100, Jeremi Piotrowski wrote:
> 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".

Here we go with the virt zoo again. If you hide TDX_CPUID_LEAF_ID from
it, then it of course doesn't know that it is a TDX guest. This is the
same thing as the SNP vTom thing: the only viable way going forward is
for the guest kernel to detect correctly what it runs on and act
accordingly.

You can't just do some semi-correct tests for vendor - correct only
if you squint hard enough - and hope that it works because it'll break
apart eventually, when that second-level TDX fun needs to add more
hackery to the guest kernel.

So, instead, think about how the paravisor tells the guest it is running
on one - a special CPUID leaf or an MSR in the AMD case - and use that
to detect it properly.

Everything else is a mess waiting to happen.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Jeremi Piotrowski]

On 10/11/2023 14:17, Borislav Petkov wrote:
> On Thu, Nov 09, 2023 at 07:41:33PM +0100, Jeremi Piotrowski wrote:
>> 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".
> 
> Here we go with the virt zoo again. If you hide TDX_CPUID_LEAF_ID from
> it, then it of course doesn't know that it is a TDX guest. This is the
> same thing as the SNP vTom thing: the only viable way going forward is
> for the guest kernel to detect correctly what it runs on and act
> accordingly.

That part already works correctly. The kernel knows very well that it is a
TDX guest because TD partitioning (same as SNP vTOM) support uses the standard
coco mechanisms to indicate that to the kernel. The kernel is well aware of
how to operate in this case: use bounce buffers, flip page visibility by calling
the correct hoosk, etc. Same flow as for every other flavor of confidential guest.

> 
> You can't just do some semi-correct tests for vendor - correct only
> if you squint hard enough - and hope that it works because it'll break
> apart eventually, when that second-level TDX fun needs to add more
> hackery to the guest kernel.
> 

What's semi-correct about checking for CC_VENDOR_INTEL and then printing Intel?
I can post a v2 that checks CC_ATTR_GUEST_MEM_ENCRYPT before printing "TDX".
Feature printing needs to evolve as new technologies come along.

> So, instead, think about how the paravisor tells the guest it is running
> on one - a special CPUID leaf or an MSR in the AMD case - and use that> to detect it properly.

The paravisor *is* telling the guest it is running on one - using a CPUID leaf
(HYPERV_CPUID_ISOLATION_CONFIG). A paravisor is a hypervisor for a confidential
guest, that's why paravisor detection shares logic with hypervisor detection.

tdx_early_init() runs extremely early, way before hypervisor(/paravisor) detection.
If the TDX_CPUID_LEAF_ID leaf were present it would require duplicating hypervisor/paravisor
logic in that function (and in sme_early_init()). As soon as we'd detect the
paravisor we'd need to avoid performing tdx_module_calls() (because they're not
allowed) so the function would return without doing anything useful:

void __init tdx_early_init(void)
{
        u64 cc_mask;
        u32 eax, sig[3];

        cpuid_count(TDX_CPUID_LEAF_ID, 0, &eax, &sig[0], &sig[2],  &sig[1]);

        if (memcmp(TDX_IDENT, sig, sizeof(sig)))
                return;

        setup_force_cpu_cap(X86_FEATURE_TDX_GUEST);

        cc_vendor = CC_VENDOR_INTEL;

        /* Can't perform tdx_module_calls when a paravisor is present */
        if (early_detect_paravisor())
                goto exit;

        ....
exit:
        pr_info("Guest detected\n");
}

Additionally we'd need to sprinkle paravisor checks along with existing X86_FEATURE_TDX_GUEST
checks. And any time someone adds a new feature that depends solely on X86_FEATURE_TDX_GUEST
we'd run the chance of it breaking things.

That would be a mess.

Jeremi


> 
> Everything else is a mess waiting to happen.
> 
> Thx.
> 

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

[Borislav Petkov]

On Fri, Nov 10, 2023 at 04:51:43PM +0100, Jeremi Piotrowski wrote:
> What's semi-correct about checking for CC_VENDOR_INTEL and then
> printing Intel?  I can post a v2 that checks CC_ATTR_GUEST_MEM_ENCRYPT
> before printing "TDX".

How is it that you're not seeing the conflict:

Your TD partitioning guest *is* a TDX guest so X86_FEATURE_TDX_GUEST
should be set there. But it isn't. Which means, that is already wrong.
Or insufficient.

	 if (cc_vendor == CC_VENDOR_INTEL)

just *happens* to work for your case.

What the detection code should do, rather, is:

	if (guest type == TD partioning)
		set bla;
	else if (TDX_CPUID_LEAF_ID)
		"normal" TDX guest;

and those rules need to be spelled out so that everyone is on the same
page as to how a TD partitioning guest is detected, how a normal TDX
guest is detected, a SEV-ES, a SNP one, yadda yadda.

> The paravisor *is* telling the guest it is running on one - using a CPUID leaf
> (HYPERV_CPUID_ISOLATION_CONFIG). A paravisor is a hypervisor for a confidential
> guest, that's why paravisor detection shares logic with hypervisor detection.
> 
> tdx_early_init() runs extremely early, way before hypervisor(/paravisor) detection.

What?

Why can't tdx_early_init() run CPUID(HYPERV_CPUID_ISOLATION_CONFIG) if
it can't find a valid TDX_CPUID_LEAF_ID and set X86_FEATURE_TDX_GUEST
then?

> Additionally we'd need to sprinkle paravisor checks along with
> existing X86_FEATURE_TDX_GUEST checks. And any time someone adds a new
> feature that depends solely on X86_FEATURE_TDX_GUEST we'd run the
> chance of it breaking things.

Well, before anything, you'd need to define what exactly the guest kernel
needs to do when running as a TD partitioning guest and how exactly that
is going to be detected and checked using the current cc_* and
cpufeatures infra. If it doesn't work with the current scheme, then the
current scheme should be extended.

Then, that should be properly written out:

"if bit X is set, then that is a guest type Y"
"if feature foo present, then so and so are given"

If the current guest type detection is insufficient, then that should be
extended/amended.

That's the only viable way where the kernel would support properly and
reliably a given guest type. There'll be no sprinkling of checks
anywhere.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Jeremi Piotrowski]

On 10/11/2023 17:45, Borislav Petkov wrote:
> On Fri, Nov 10, 2023 at 04:51:43PM +0100, Jeremi Piotrowski wrote:
>> What's semi-correct about checking for CC_VENDOR_INTEL and then
>> printing Intel?  I can post a v2 that checks CC_ATTR_GUEST_MEM_ENCRYPT
>> before printing "TDX".
> 
> How is it that you're not seeing the conflict:
> 
> Your TD partitioning guest *is* a TDX guest so X86_FEATURE_TDX_GUEST
> should be set there. But it isn't. Which means, that is already wrong.
> Or insufficient.
> 
> 	 if (cc_vendor == CC_VENDOR_INTEL)
> 
> just *happens* to work for your case.
> 
> What the detection code should do, rather, is:
> 
> 	if (guest type == TD partioning)
> 		set bla;
> 	else if (TDX_CPUID_LEAF_ID)
> 		"normal" TDX guest;
> 
> and those rules need to be spelled out so that everyone is on the same
> page as to how a TD partitioning guest is detected, how a normal TDX
> guest is detected, a SEV-ES, a SNP one, yadda yadda.
> 
>> The paravisor *is* telling the guest it is running on one - using a CPUID leaf
>> (HYPERV_CPUID_ISOLATION_CONFIG). A paravisor is a hypervisor for a confidential
>> guest, that's why paravisor detection shares logic with hypervisor detection.
>>
>> tdx_early_init() runs extremely early, way before hypervisor(/paravisor) detection.
> 
> What?
> 
> Why can't tdx_early_init() run CPUID(HYPERV_CPUID_ISOLATION_CONFIG) if
> it can't find a valid TDX_CPUID_LEAF_ID and set X86_FEATURE_TDX_GUEST
> then?

I guess it can if no one has an issue with it.

Thank you for the review, I've posted a patchset that implements this idea here:
https://lore.kernel.org/lkml/20231122170106.270266-1-jpiotrowski@linux.microsoft.com/T/#u

> 
>> Additionally we'd need to sprinkle paravisor checks along with
>> existing X86_FEATURE_TDX_GUEST checks. And any time someone adds a new
>> feature that depends solely on X86_FEATURE_TDX_GUEST we'd run the
>> chance of it breaking things.
> 
> Well, before anything, you'd need to define what exactly the guest kernel
> needs to do when running as a TD partitioning guest and how exactly that
> is going to be detected and checked using the current cc_* and
> cpufeatures infra. If it doesn't work with the current scheme, then the
> current scheme should be extended.
> 
> Then, that should be properly written out:
> 
> "if bit X is set, then that is a guest type Y"
> "if feature foo present, then so and so are given"
> 
> If the current guest type detection is insufficient, then that should be
> extended/amended.
> 
> That's the only viable way where the kernel would support properly and
> reliably a given guest type. There'll be no sprinkling of checks
> anywhere.
> 
> Thx.
> 

OK. In the new submission I've added CC_ATTR_TDX_MODULE_CALLS because that
is what we really need to guard against, and these guests can then have
X86_FEATURE_TDX_GUEST set normally.

Jeremi