diff mbox series

x86/mm: Print the encryption features correctly when a paravisor is present

Message ID 20231019062030.3206-1-decui@microsoft.com
State New
Headers
Series x86/mm: Print the encryption features correctly when a paravisor is present |

Commit Message

Dexuan Cui Oct. 19, 2023, 6:20 a.m. UTC 
  Hyper-V provides two modes for running a TDX/SNP VM:

1) In TD Partitioning mode (TDX) or vTOM mode (SNP) with a paravisor;
2) In "fully enlightened" mode with the normal TDX shared bit or SNP C-bit
   control over page encryption, and no paravisor.

In the first mode (i.e. paravisor mode), the native TDX/SNP CPUID
capability is hidden from the VM, but cc_platform_has(CC_ATTR_MEM_ENCRYPT)
and cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) are true; as a result,
mem_encrypt_init() incorrectly prints the below message when an Intel TDX
VM with a paravisor runs on Hyper-V:
"Memory Encryption Features active: AMD SEV".

Introduce x86_platform.print_mem_enc_feature_info and allow hv_vtom_init()
to override the function pointer so that the correct message is printed.

BTW, when a VBS (Virtualization-based Security) VM running on Hyper-V
(the physical CPU can be an AMD CPU or an Intel CPU), the VM's memory is
not encrypted, but mem_encrypt_init() also prints the same incorrect
message. The introduction of x86_platform.print_mem_enc_feature_info can
also fix the issue.

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---

Some open questions:

1. Should we refactor the existing print_memory_encrypt_feature_info()
into a TDX-specific function and an SEV-specific function?  The
function pointer in x86_platform_ops would be initialized to a no-op
function, and then early_tdx_init(), sme_enable() and hv_vtom_init()
would fill it in accordingly.

2. Should we rename "print_mem_enc_feature_info()" to
"print_coco_feature_info()"?  CC_ATTR_GUEST_STATE_ENCRYPT (and
CC_ATTR_GUEST_SEV_SNP?) may not look like *memory* encryption to me?

 arch/x86/hyperv/ivm.c              | 11 +++++++++++
 arch/x86/include/asm/mem_encrypt.h |  1 +
 arch/x86/include/asm/x86_init.h    |  2 ++
 arch/x86/kernel/x86_init.c         |  1 +
 arch/x86/mm/mem_encrypt.c          |  4 ++--
 5 files changed, 17 insertions(+), 2 deletions(-)

Comments

Dave Hansen Oct. 19, 2023, 3:54 p.m. UTC | #1 
> --- a/arch/x86/hyperv/ivm.c
> +++ b/arch/x86/hyperv/ivm.c
> @@ -450,6 +450,16 @@ static bool hv_is_private_mmio(u64 addr)
>  	return false;
>  }
>  
> +static void hv_print_mem_enc_feature_info(void)
> +{
> +	enum hv_isolation_type type = hv_get_isolation_type();
> +
> +	if (type == HV_ISOLATION_TYPE_SNP)
> +		pr_info("Memory Encryption Features active: AMD SEV\n");
> +	else if (type == HV_ISOLATION_TYPE_TDX)
> +		pr_info("Memory Encryption Features active: Intel TDX\n");
> +}

If we draw this to its logical conclusion, every paravisor will need a
pr_info() for every hardware CoCo implementation.  That M*N pr_info()s.
That seems nuts.
Dexuan Cui Oct. 20, 2023, 6:01 a.m. UTC | #2 
> From: Dave Hansen <dave.hansen@intel.com>
> Sent: Thursday, October 19, 2023 8:54 AM
> To: Dexuan Cui <decui@microsoft.com>; KY Srinivasan
> [...]
> > --- a/arch/x86/hyperv/ivm.c
> > +++ b/arch/x86/hyperv/ivm.c
> > @@ -450,6 +450,16 @@ static bool hv_is_private_mmio(u64 addr)
> >  	return false;
> >  }
> >
> > +static void hv_print_mem_enc_feature_info(void)
> > +{
> > +	enum hv_isolation_type type = hv_get_isolation_type();
> > +
> > +	if (type == HV_ISOLATION_TYPE_SNP)
> > +		pr_info("Memory Encryption Features active: AMD
> SEV\n");
> > +	else if (type == HV_ISOLATION_TYPE_TDX)
> > +		pr_info("Memory Encryption Features active: Intel
> > TDX\n");
> > +}
> 
> If we draw this to its logical conclusion, every paravisor will need a
> pr_info() for every hardware CoCo implementation.  That M*N pr_info()s.
> That seems nuts.

This patch only modifies x86 related files. I think it's unlikely to see
a third hardware Coco implementation for x86 in the foreseeable feature (?)
When we have a third implementation, I suppose more code, e.g., the existing
print_mem_encrypt_feature_info(),  will have to be changed as well.

Currently it looks like there is only 1 paravisor implementation. 
I think we'll know if some code can be shared only when a second paravisor
implementation appears.

I can use the below version if you think it's better:

static const char *hv_mem_enc_features[] = {
        [ HV_ISOLATION_TYPE_SNP ] = "AMD SEV",
        [ HV_ISOLATION_TYPE_TDX ] = "Intel TDX",
};

static void hv_print_mem_enc_feature_info(void)
{
        enum hv_isolation_type type = hv_get_isolation_type();

        if (type < HV_ISOLATION_TYPE_SNP || type > HV_ISOLATION_TYPE_TDX)
                return;

        pr_info("Memory Encryption Features active:: %s\n",
                hv_mem_enc_features[type]);
}

Thanks,
Dexuan
Dave Hansen Oct. 20, 2023, 6:39 p.m. UTC | #3 
On 10/19/23 23:01, Dexuan Cui wrote:
> This patch only modifies x86 related files. I think it's unlikely to see
> a third hardware Coco implementation for x86 in the foreseeable feature (?)

OK, then what good is this patch in the first place?  If you are right,
then this would give equivalent information:

cat /proc/cpuinfo | grep -q Intel && echo 'TDX'
cat /proc/cpuinfo | grep -q AMD   && echo 'SEV'

No kernel patching needed, right?
Dexuan Cui Oct. 20, 2023, 8 p.m. UTC | #4 
> From: Dave Hansen <dave.hansen@intel.com>
> Sent: Friday, October 20, 2023 11:40 AM
> To: Dexuan Cui <decui@microsoft.com>; KY Srinivasan
> [...]
> On 10/19/23 23:01, Dexuan Cui wrote:
> > This patch only modifies x86 related files. I think it's unlikely to see
> > a third hardware Coco implementation for x86 in the foreseeable feature
> (?)
> 
> OK, then what good is this patch in the first place?  If you are right,
> then this would give equivalent information:
> 
> cat /proc/cpuinfo | grep -q Intel && echo 'TDX'
> cat /proc/cpuinfo | grep -q AMD   && echo 'SEV'
> 
> No kernel patching needed, right?

Currently arch/x86/mm/mem_encrypt.c: print_mem_encrypt_feature_info()
prints an incorrect and confusing message 
"Memory Encryption Features active: AMD SEV".
when an Intel TDX VM with a paravisor runs on Hyper-V.

So I think a kernel patch is needed.
Dave Hansen Oct. 20, 2023, 8:13 p.m. UTC | #5 
On 10/20/23 13:00, Dexuan Cui wrote:
>> OK, then what good is this patch in the first place?  If you are right,
>> then this would give equivalent information:
>>
>> cat /proc/cpuinfo | grep -q Intel && echo 'TDX'
>> cat /proc/cpuinfo | grep -q AMD   && echo 'SEV'
>>
>> No kernel patching needed, right?
> Currently arch/x86/mm/mem_encrypt.c: print_mem_encrypt_feature_info()
> prints an incorrect and confusing message 
> "Memory Encryption Features active: AMD SEV".
> when an Intel TDX VM with a paravisor runs on Hyper-V.
> 
> So I think a kernel patch is needed.

How about either removing the message entirely or removing the ": AMD
SEV" part?
Dexuan Cui Oct. 20, 2023, 8:20 p.m. UTC | #6 
> From: Dave Hansen <dave.hansen@intel.com>
> Sent: Friday, October 20, 2023 1:14 PM
> To: Dexuan Cui <decui@microsoft.com>; KY Srinivasan
> [...]
> On 10/20/23 13:00, Dexuan Cui wrote:
> >> OK, then what good is this patch in the first place?  If you are right,
> >> then this would give equivalent information:
> >>
> >> cat /proc/cpuinfo | grep -q Intel && echo 'TDX'
> >> cat /proc/cpuinfo | grep -q AMD   && echo 'SEV'
> >>
> >> No kernel patching needed, right?
> > Currently arch/x86/mm/mem_encrypt.c:
> print_mem_encrypt_feature_info()
> > prints an incorrect and confusing message
> > "Memory Encryption Features active: AMD SEV".
> > when an Intel TDX VM with a paravisor runs on Hyper-V.
> >
> > So I think a kernel patch is needed.
> 
> How about either removing the message entirely or removing the ": AMD
> SEV" part?

It looks good to me if we can update/remove the message in
print_mem_encrypt_feature_info(), but I guess AMD folks might want to
keep the ": AMD SEV" part?
Borislav Petkov Oct. 20, 2023, 8:21 p.m. UTC | #7 
On Fri, Oct 20, 2023 at 08:00:13PM +0000, Dexuan Cui wrote:
> Currently arch/x86/mm/mem_encrypt.c: print_mem_encrypt_feature_info()
> prints an incorrect and confusing message 
> "Memory Encryption Features active: AMD SEV".
> when an Intel TDX VM with a paravisor runs on Hyper-V.
> 
> So I think a kernel patch is needed.

So I'm trying to parse this:

"Hyper-V provides two modes for running a TDX/SNP VM:

1) In TD Partitioning mode (TDX) or vTOM mode (SNP) with a paravisor;
2) In "fully enlightened" mode with the normal TDX shared bit or SNP C-bit
   control over page encryption, and no paravisor."

and it all sounds like word salad to me.

The fact that you've managed to advertize a salad of CPUID bits to the
guest to lead to such confusing statement, sounds like a major insanity.

> the native TDX/SNP CPUID capability is hidden from the VM

Why do you wonder then that it detects wrong?! You're hiding it!

> but cc_platform_has(CC_ATTR_MEM_ENCRYPT) and
> cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) are true;

I guess you need to go to talk to Michael:

812b0597fb40 ("x86/hyperv: Change vTOM handling to use standard coco mechanisms")
diff mbox series

Patch

diff --git a/arch/x86/hyperv/ivm.c b/arch/x86/hyperv/ivm.c
index e68051eba25a..fdc2fab0415e 100644
--- a/arch/x86/hyperv/ivm.c
+++ b/arch/x86/hyperv/ivm.c
@@ -450,6 +450,16 @@  static bool hv_is_private_mmio(u64 addr)
 	return false;
 }
 
+static void hv_print_mem_enc_feature_info(void)
+{
+	enum hv_isolation_type type = hv_get_isolation_type();
+
+	if (type == HV_ISOLATION_TYPE_SNP)
+		pr_info("Memory Encryption Features active: AMD SEV\n");
+	else if (type == HV_ISOLATION_TYPE_TDX)
+		pr_info("Memory Encryption Features active: Intel TDX\n");
+}
+
 void __init hv_vtom_init(void)
 {
 	enum hv_isolation_type type = hv_get_isolation_type();
@@ -479,6 +489,7 @@  void __init hv_vtom_init(void)
 	cc_set_mask(ms_hyperv.shared_gpa_boundary);
 	physical_mask &= ms_hyperv.shared_gpa_boundary - 1;
 
+	x86_platform.print_mem_enc_feature_info = hv_print_mem_enc_feature_info;
 	x86_platform.hyper.is_private_mmio = hv_is_private_mmio;
 	x86_platform.guest.enc_cache_flush_required = hv_vtom_cache_flush_required;
 	x86_platform.guest.enc_tlb_flush_required = hv_vtom_tlb_flush_required;
diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h
index 7f97a8a97e24..6e8050a9138e 100644
--- a/arch/x86/include/asm/mem_encrypt.h
+++ b/arch/x86/include/asm/mem_encrypt.h
@@ -19,6 +19,7 @@ 
 
 #ifdef CONFIG_X86_MEM_ENCRYPT
 void __init mem_encrypt_init(void);
+void print_mem_encrypt_feature_info(void);
 #else
 static inline void mem_encrypt_init(void) { }
 #endif
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 14b0562c1d8b..7798174d4b8d 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -291,6 +291,7 @@  struct x86_hyper_runtime {
  * 				semantics.
  * @realmode_reserve:		reserve memory for realmode trampoline
  * @realmode_init:		initialize realmode trampoline
+ * @print_mem_enc_feature_info:	print the supported memory encryption features
  * @hyper:			x86 hypervisor specific runtime callbacks
  */
 struct x86_platform_ops {
@@ -309,6 +310,7 @@  struct x86_platform_ops {
 	void (*set_legacy_features)(void);
 	void (*realmode_reserve)(void);
 	void (*realmode_init)(void);
+	void (*print_mem_enc_feature_info)(void);
 	struct x86_hyper_runtime hyper;
 	struct x86_guest guest;
 };
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index 6117662ae4e6..ccb53db1b51e 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -149,6 +149,7 @@  struct x86_platform_ops x86_platform __ro_after_init = {
 	.restore_sched_clock_state	= tsc_restore_sched_clock_state,
 	.realmode_reserve		= reserve_real_mode,
 	.realmode_init			= init_real_mode,
+	.print_mem_enc_feature_info	= print_mem_encrypt_feature_info,
 	.hyper.pin_vcpu			= x86_op_int_noop,
 	.hyper.is_private_mmio		= is_private_mmio_noop,
 
diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index 9f27e14e185f..8d37048bc1df 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -39,7 +39,7 @@  bool force_dma_unencrypted(struct device *dev)
 	return false;
 }
 
-static void print_mem_encrypt_feature_info(void)
+void print_mem_encrypt_feature_info(void)
 {
 	pr_info("Memory Encryption Features active:");
 
@@ -84,5 +84,5 @@  void __init mem_encrypt_init(void)
 	/* Call into SWIOTLB to update the SWIOTLB DMA buffers */
 	swiotlb_update_mem_attributes();
 
-	print_mem_encrypt_feature_info();
+	x86_platform.print_mem_enc_feature_info();
 }