Skip to contentSkip to conversation

Limit memory encryption cpuid pass through

PATCH In review Peter Gonda 5 participants 15 messages Nov 21, 2019 – Nov 27, 2019

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Peter Gonda

author
#0 lore raw

cover Limit memory encryption cpuid pass through

KVM_GET_SUPPORTED_CPUID for 0x8000001F currently passes through all data if X86_FEATURE_SEV is enabled. Guests only need the SEV bit and Cbit location to work correctly. This series moves handing of this cpuid function out of svm.c to the general x86 function and masks out host data.

Peter Gonda (2):
KVM x86: Move kvm cpuid support out of svm
KVM x86: Mask memory encryption guest cpuid

 arch/x86/kvm/cpuid.c | 11 +++++++++++
 arch/x86/kvm/svm.c   |  7 -------
 2 files changed, 11 insertions(+), 7 deletions(-)
Signature · 1 line
2.24.0.432.g9d3f5f5b63-goog
author
#1 lore raw

PATCH 1/2 KVM x86: Move kvm cpuid support out of svm

Memory encryption support does not have module parameter dependencies and can be moved into the general x86 cpuid __do_cpuid_ent function. This changes maintains current behavior of passing through all of CPUID.8000001F.

Suggested-by Jim Mattson <[email protected]>
Signed-off-by Peter Gonda <[email protected]>
Reviewed-by Jim Mattson <[email protected]>
---
 arch/x86/kvm/cpuid.c | 5 +++++
 arch/x86/kvm/svm.c   | 7 -------
 2 files changed, 5 insertions(+), 7 deletions(-)
arch/x86/kvm/cpuid.c +5 -0
index f68c0c753c38..946fa9cb9dd6 100644
@@ -778,6 +778,11 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 	case 0x8000001a:
 	case 0x8000001e:
 		break;
+	/* Support memory encryption cpuid if host supports it */
+	case 0x8000001F:
+		if (!boot_cpu_has(X86_FEATURE_SEV))
+			entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
+		break;
 	/*Add support for Centaur's CPUID instruction*/
 	case 0xC0000000:
 		/*Just support up to 0xC0000004 now*/
arch/x86/kvm/svm.c +0 -7
index c5673bda4b66..79842329ebcd 100644
@@ -5936,13 +5936,6 @@ static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
 		if (npt_enabled)
 			entry->edx |= F(NPT);
 
-		break;
-	case 0x8000001F:
-		/* Support memory encryption cpuid if host supports it */
-		if (boot_cpu_has(X86_FEATURE_SEV))
-			cpuid(0x8000001f, &entry->eax, &entry->ebx,
-				&entry->ecx, &entry->edx);
-
 	}
 }
Signature · 1 line
2.24.0.432.g9d3f5f5b63-goog
Reviewed-by on v1 1/2
#2 lore raw

Re: [PATCH 1/2] KVM x86: Move kvm cpuid support out of svm

Quoted 37 lines · Peter Gonda

Memory encryption support does not have module parameter dependencies and can be moved into the general x86 cpuid __do_cpuid_ent function. This changes maintains current behavior of passing through all of CPUID.8000001F.

Suggested-by Jim Mattson <[email protected]>
Signed-off-by Peter Gonda <[email protected]>
Reviewed-by Jim Mattson <[email protected]>
---
  arch/x86/kvm/cpuid.c | 5 +++++
  arch/x86/kvm/svm.c   | 7 -------
  2 files changed, 5 insertions(+), 7 deletions(-)
arch/x86/kvm/cpuid.c +5 -0
index f68c0c753c38..946fa9cb9dd6 100644
@@ -778,6 +778,11 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
  	case 0x8000001a:
  	case 0x8000001e:
  		break;
+	/* Support memory encryption cpuid if host supports it */
+	case 0x8000001F:
+		if (!boot_cpu_has(X86_FEATURE_SEV))
+			entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
+		break;
  	/*Add support for Centaur's CPUID instruction*/
  	case 0xC0000000:
  		/*Just support up to 0xC0000004 now*/
arch/x86/kvm/svm.c +0 -0
index c5673bda4b66..79842329ebcd 100644
@@ -5936,13 +5936,6 @@ static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
  		if (npt_enabled)
  			entry->edx |= F(NPT);

arch/x86/kvm/svm.c static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry) · line 5941

Quoted 10 lines
arch/x86/kvm/svm.c +0 -7
-		break;
-	case 0x8000001F:
-		/* Support memory encryption cpuid if host supports it */
-		if (boot_cpu_has(X86_FEATURE_SEV))
-			cpuid(0x8000001f, &entry->eax, &entry->ebx,
-				&entry->ecx, &entry->edx);
-
  	}
  }
Reviewed-by Krish Sadhukhan <[email protected]>
maintainer
#3 lore raw

Re: [PATCH 1/2] KVM x86: Move kvm cpuid support out of svm

In reply to Peter Gonda
Quoted 38 lines · Peter Gonda

Memory encryption support does not have module parameter dependencies and can be moved into the general x86 cpuid __do_cpuid_ent function. This changes maintains current behavior of passing through all of CPUID.8000001F.

Suggested-by Jim Mattson <[email protected]>
Signed-off-by Peter Gonda <[email protected]>
Reviewed-by Jim Mattson <[email protected]>
---
 arch/x86/kvm/cpuid.c | 5 +++++
 arch/x86/kvm/svm.c   | 7 -------
 2 files changed, 5 insertions(+), 7 deletions(-)
arch/x86/kvm/cpuid.c +5 -0
index f68c0c753c38..946fa9cb9dd6 100644
@@ -778,6 +778,11 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 	case 0x8000001a:
 	case 0x8000001e:
 		break;
+	/* Support memory encryption cpuid if host supports it */
+	case 0x8000001F:
+		if (!boot_cpu_has(X86_FEATURE_SEV))
+			entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
+		break;
 	/*Add support for Centaur's CPUID instruction*/
 	case 0xC0000000:
 		/*Just support up to 0xC0000004 now*/
arch/x86/kvm/svm.c +0 -0
index c5673bda4b66..79842329ebcd 100644
@@ -5936,13 +5936,6 @@ static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
 		if (npt_enabled)
 			entry->edx |= F(NPT);

arch/x86/kvm/svm.c static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry) · line 5941

Quoted 10 lines
arch/x86/kvm/svm.c +0 -7
-		break;
-	case 0x8000001F:
-		/* Support memory encryption cpuid if host supports it */
-		if (boot_cpu_has(X86_FEATURE_SEV))
-			cpuid(0x8000001f, &entry->eax, &entry->ebx,
-				&entry->ecx, &entry->edx);
-
 	}
 }

Queued patch 1, only.

Paolo

author
#4 lore raw

PATCH 2/2 KVM x86: Mask memory encryption guest cpuid

In reply to Peter Gonda

Only pass through guest relevant CPUID information: Cbit location and SEV bit. The kernel does not support nested SEV guests so the other data in this CPUID leaf is unneeded by the guest.

Suggested-by Jim Mattson <[email protected]>
Signed-off-by Peter Gonda <[email protected]>
Reviewed-by Jim Mattson <[email protected]>
---
 arch/x86/kvm/cpuid.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
arch/x86/kvm/cpuid.c +7 -1
index 946fa9cb9dd6..6439fb1dbe76 100644
@@ -780,8 +780,14 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 		break;
 	/* Support memory encryption cpuid if host supports it */
 	case 0x8000001F:
-		if (!boot_cpu_has(X86_FEATURE_SEV))
+		if (boot_cpu_has(X86_FEATURE_SEV)) {
+			/* Expose only SEV bit and CBit location */
+			entry->eax &= F(SEV);
+			entry->ebx &= GENMASK(5, 0);
+			entry->edx = entry->ecx = 0;
+		} else {
 			entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
+		}
 		break;
 	/*Add support for Centaur's CPUID instruction*/
 	case 0xC0000000:
Signature · 1 line
2.24.0.432.g9d3f5f5b63-goog
#5 lore raw

Re: [PATCH 2/2] KVM x86: Mask memory encryption guest cpuid

Quoted 16 lines · Peter Gonda

Only pass through guest relevant CPUID information: Cbit location and SEV bit. The kernel does not support nested SEV guests so the other data in this CPUID leaf is unneeded by the guest.

Suggested-by Jim Mattson <[email protected]>
Signed-off-by Peter Gonda <[email protected]>
Reviewed-by Jim Mattson <[email protected]>
---
 arch/x86/kvm/cpuid.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
arch/x86/kvm/cpuid.c +0 -0
index 946fa9cb9dd6..6439fb1dbe76 100644

arch/x86/kvm/cpuid.c static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function, · line 785

Quoted 7 lines
arch/x86/kvm/cpuid.c +3 -1
@@ -780,8 +780,14 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 		break;
 	/* Support memory encryption cpuid if host supports it */
 	case 0x8000001F:
-		if (!boot_cpu_has(X86_FEATURE_SEV))
+		if (boot_cpu_has(X86_FEATURE_SEV)) {
+			/* Expose only SEV bit and CBit location */
+			entry->eax &= F(SEV);

I know SEV-ES patches are not accepted yet, but can I ask to pass the SEV-ES bit in eax?

Quoted 8 lines
arch/x86/kvm/cpuid.c +4 -0
+			entry->ebx &= GENMASK(5, 0);
+			entry->edx = entry->ecx = 0;
+		} else {
 			entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
+		}
 		break;
 	/*Add support for Centaur's CPUID instruction*/
 	case 0xC0000000:
maintainer
#6 lore raw

Re: [PATCH 2/2] KVM x86: Mask memory encryption guest cpuid

Quoted 13 lines · Brijesh Singh
Quoted 11 lines · Peter Gonda

Only pass through guest relevant CPUID information: Cbit location and SEV bit. The kernel does not support nested SEV guests so the other data in this CPUID leaf is unneeded by the guest.

Suggested-by Jim Mattson <[email protected]>
Signed-off-by Peter Gonda <[email protected]>
Reviewed-by Jim Mattson <[email protected]>
---
 arch/x86/kvm/cpuid.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
Quoted 16 lines

arch/x86/kvm/cpuid.c static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function, · line 785

Quoted 12 lines
arch/x86/kvm/cpuid.c +3 -1
index 946fa9cb9dd6..6439fb1dbe76 100644
@@ -780,8 +780,14 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 		break;
 	/* Support memory encryption cpuid if host supports it */
 	case 0x8000001F:
-		if (!boot_cpu_has(X86_FEATURE_SEV))
+		if (boot_cpu_has(X86_FEATURE_SEV)) {
+			/* Expose only SEV bit and CBit location */
+			entry->eax &= F(SEV);

I know SEV-ES patches are not accepted yet, but can I ask to pass the SEV-ES bit in eax?

I think it shouldn't be passed, since KVM does not support SEV-ES.

Paolo

Quoted 10 lines
Quoted 9 lines
arch/x86/kvm/cpuid.c +4 -0
+			entry->ebx &= GENMASK(5, 0);
+			entry->edx = entry->ecx = 0;
+		} else {
 			entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
+		}
 		break;
 	/*Add support for Centaur's CPUID instruction*/
 	case 0xC0000000:
#7 lore raw

Re: [PATCH 2/2] KVM x86: Mask memory encryption guest cpuid

Quoted 31 lines · Paolo Bonzini
Quoted 13 lines · Brijesh Singh
Quoted 11 lines · Peter Gonda

Only pass through guest relevant CPUID information: Cbit location and SEV bit. The kernel does not support nested SEV guests so the other data in this CPUID leaf is unneeded by the guest.

Suggested-by Jim Mattson <[email protected]>
Signed-off-by Peter Gonda <[email protected]>
Reviewed-by Jim Mattson <[email protected]>
---
  arch/x86/kvm/cpuid.c | 8 +++++++-
  1 file changed, 7 insertions(+), 1 deletion(-)
Quoted 16 lines

arch/x86/kvm/cpuid.c static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function, · line 785

Quoted 12 lines
arch/x86/kvm/cpuid.c +3 -1
index 946fa9cb9dd6..6439fb1dbe76 100644
@@ -780,8 +780,14 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
  		break;
  	/* Support memory encryption cpuid if host supports it */
  	case 0x8000001F:
-		if (!boot_cpu_has(X86_FEATURE_SEV))
+		if (boot_cpu_has(X86_FEATURE_SEV)) {
+			/* Expose only SEV bit and CBit location */
+			entry->eax &= F(SEV);

I know SEV-ES patches are not accepted yet, but can I ask to pass the SEV-ES bit in eax?

Quoted 2 lines

I think it shouldn't be passed, since KVM does not support SEV-ES.

Fair enough.

-Brijesh

#8 lore raw

Re: [PATCH 2/2] KVM x86: Mask memory encryption guest cpuid

In reply to Brijesh Singh

Does SEV-ES indicate that SEV-ES guests are supported, or that the current (v)CPU is running with SEV-ES enabled, or both?

Quoted 40 lines · Brijesh Singh
Quoted 16 lines · Peter Gonda

Only pass through guest relevant CPUID information: Cbit location and SEV bit. The kernel does not support nested SEV guests so the other data in this CPUID leaf is unneeded by the guest.

Suggested-by Jim Mattson <[email protected]>
Signed-off-by Peter Gonda <[email protected]>
Reviewed-by Jim Mattson <[email protected]>
---
 arch/x86/kvm/cpuid.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
arch/x86/kvm/cpuid.c +0 -0
index 946fa9cb9dd6..6439fb1dbe76 100644

arch/x86/kvm/cpuid.c static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function, · line 785

Quoted 7 lines
arch/x86/kvm/cpuid.c +3 -1
@@ -780,8 +780,14 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
              break;
      /* Support memory encryption cpuid if host supports it */
      case 0x8000001F:
-             if (!boot_cpu_has(X86_FEATURE_SEV))
+             if (boot_cpu_has(X86_FEATURE_SEV)) {
+                     /* Expose only SEV bit and CBit location */
+                     entry->eax &= F(SEV);

I know SEV-ES patches are not accepted yet, but can I ask to pass the SEV-ES bit in eax?

Quoted 8 lines
arch/x86/kvm/cpuid.c +4 -0
+                     entry->ebx &= GENMASK(5, 0);
+                     entry->edx = entry->ecx = 0;
+             } else {
                      entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
+             }
              break;
      /*Add support for Centaur's CPUID instruction*/
      case 0xC0000000:
author
#9 lore raw

Re: [PATCH 2/2] KVM x86: Mask memory encryption guest cpuid

I am not sure that the SevEs CPUID bit has the same problem as the Sev bit. It seems the reason the Sev bit was to be passed to the guest was to prevent the guest from reading the SEV MSR if it did not exist. If the guest is running with SevEs it must be also running with Sev. So the guest can safely read the SevStatus MSR to check the SevEsEnabled bit because the Sev CPUID bit will be set.

If I look at the AMD patches for ES. I see just that,
https://github.com/AMDESE/linux/commit/c19d84b803caf8e3130b1498868d0fcafc755da7,
it doesn't look for the SevEs CPUID bit.

} else {
/* For SEV, check the SEV MSR */
msr = __rdmsr(MSR_AMD64_SEV);
if (!(msr & MSR_AMD64_SEV_ENABLED))

    return;

/* SEV state cannot be controlled by a command line option */ sme_me_mask = me_mask;
sme_me_status |= SEV_ACTIVE;
physical_mask &= ~sme_me_mask;
+
+ if (!(msr & MSR_AMD64_SEV_ES_ENABLED))
+ return;
+
+ sme_me_status |= SEV_ES_ACTIVE;
return;
}

}

Quoted 45 lines · Jim Mattson

Does SEV-ES indicate that SEV-ES guests are supported, or that the current (v)CPU is running with SEV-ES enabled, or both?

Quoted 40 lines · Brijesh Singh
Quoted 16 lines · Peter Gonda

Only pass through guest relevant CPUID information: Cbit location and SEV bit. The kernel does not support nested SEV guests so the other data in this CPUID leaf is unneeded by the guest.

Suggested-by Jim Mattson <[email protected]>
Signed-off-by Peter Gonda <[email protected]>
Reviewed-by Jim Mattson <[email protected]>
---
 arch/x86/kvm/cpuid.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
arch/x86/kvm/cpuid.c +0 -0
index 946fa9cb9dd6..6439fb1dbe76 100644

arch/x86/kvm/cpuid.c static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function, · line 785

Quoted 7 lines
arch/x86/kvm/cpuid.c +3 -1
@@ -780,8 +780,14 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
              break;
      /* Support memory encryption cpuid if host supports it */
      case 0x8000001F:
-             if (!boot_cpu_has(X86_FEATURE_SEV))
+             if (boot_cpu_has(X86_FEATURE_SEV)) {
+                     /* Expose only SEV bit and CBit location */
+                     entry->eax &= F(SEV);

I know SEV-ES patches are not accepted yet, but can I ask to pass the SEV-ES bit in eax?

Quoted 8 lines
arch/x86/kvm/cpuid.c +4 -0
+                     entry->ebx &= GENMASK(5, 0);
+                     entry->edx = entry->ecx = 0;
+             } else {
                      entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
+             }
              break;
      /*Add support for Centaur's CPUID instruction*/
      case 0xC0000000:
#10 lore raw

Re: [PATCH 2/2] KVM x86: Mask memory encryption guest cpuid

Ah, I missed the fact that we don't need to pass the SevES bit to the guest because guest actually does not need it. It just needs the SevBit to make decision whether its safe to call the RDMSR for SEV_STATUS. The SEV_STATUS MSR will give information which SEV feature is enabled.

thanks

Quoted 77 lines · Peter Gonda

I am not sure that the SevEs CPUID bit has the same problem as the Sev bit. It seems the reason the Sev bit was to be passed to the guest was to prevent the guest from reading the SEV MSR if it did not exist. If the guest is running with SevEs it must be also running with Sev. So the guest can safely read the SevStatus MSR to check the SevEsEnabled bit because the Sev CPUID bit will be set.

If I look at the AMD patches for ES. I see just that,
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Flinux%2Fcommit%2Fc19d84b803caf8e3130b1498868d0fcafc755da7&amp;data=02%7C01%7Cbrijesh.singh%40amd.com%7Cfe5a46e348a5464ea52b08d76f85909b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637100491764446005&amp;sdata=R6RrRO0TpcfM7uzpBbsGhVp47bA%2BVoz624IBQif%2BxjA%3D&amp;reserved=0,
it doesn't look for the SevEs CPUID bit.

} else {
   /* For SEV, check the SEV MSR */
   msr = __rdmsr(MSR_AMD64_SEV);
   if (!(msr & MSR_AMD64_SEV_ENABLED))
     return;
   /* SEV state cannot be controlled by a command line option */
   sme_me_mask = me_mask;
   sme_me_status |= SEV_ACTIVE;
   physical_mask &= ~sme_me_mask;
(quoted diff) +5 -0
+
+  if (!(msr & MSR_AMD64_SEV_ES_ENABLED))
+    return;
+
+  sme_me_status |= SEV_ES_ACTIVE;
   return;

}

}

Quoted 45 lines · Jim Mattson

Does SEV-ES indicate that SEV-ES guests are supported, or that the current (v)CPU is running with SEV-ES enabled, or both?

Quoted 40 lines · Brijesh Singh
Quoted 16 lines · Peter Gonda

Only pass through guest relevant CPUID information: Cbit location and SEV bit. The kernel does not support nested SEV guests so the other data in this CPUID leaf is unneeded by the guest.

Suggested-by Jim Mattson <[email protected]>
Signed-off-by Peter Gonda <[email protected]>
Reviewed-by Jim Mattson <[email protected]>
---
  arch/x86/kvm/cpuid.c | 8 +++++++-
  1 file changed, 7 insertions(+), 1 deletion(-)
arch/x86/kvm/cpuid.c +0 -0
index 946fa9cb9dd6..6439fb1dbe76 100644

arch/x86/kvm/cpuid.c static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function, · line 785

Quoted 7 lines
arch/x86/kvm/cpuid.c +3 -1
@@ -780,8 +780,14 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
               break;
       /* Support memory encryption cpuid if host supports it */
       case 0x8000001F:
-             if (!boot_cpu_has(X86_FEATURE_SEV))
+             if (boot_cpu_has(X86_FEATURE_SEV)) {
+                     /* Expose only SEV bit and CBit location */
+                     entry->eax &= F(SEV);

I know SEV-ES patches are not accepted yet, but can I ask to pass the SEV-ES bit in eax?

Quoted 8 lines
arch/x86/kvm/cpuid.c +4 -0
+                     entry->ebx &= GENMASK(5, 0);
+                     entry->edx = entry->ecx = 0;
+             } else {
                       entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
+             }
               break;
       /*Add support for Centaur's CPUID instruction*/
       case 0xC0000000:
#11 lore raw

Re: [PATCH 2/2] KVM x86: Mask memory encryption guest cpuid

Quoted 6 lines · Brijesh Singh

Ah, I missed the fact that we don't need to pass the SevES bit to the guest because guest actually does not need it. It just needs the SevBit to make decision whether its safe to call the RDMSR for SEV_STATUS. The SEV_STATUS MSR will give information which SEV feature is enabled.

Why does it have to be safe to read the SEV_STATUS MSR? We read nonexistent MSRs all the time.

Quoted 80 lines

thanks

Quoted 77 lines · Peter Gonda

I am not sure that the SevEs CPUID bit has the same problem as the Sev bit. It seems the reason the Sev bit was to be passed to the guest was to prevent the guest from reading the SEV MSR if it did not exist. If the guest is running with SevEs it must be also running with Sev. So the guest can safely read the SevStatus MSR to check the SevEsEnabled bit because the Sev CPUID bit will be set.

If I look at the AMD patches for ES. I see just that,
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Flinux%2Fcommit%2Fc19d84b803caf8e3130b1498868d0fcafc755da7&amp;data=02%7C01%7Cbrijesh.singh%40amd.com%7Cfe5a46e348a5464ea52b08d76f85909b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637100491764446005&amp;sdata=R6RrRO0TpcfM7uzpBbsGhVp47bA%2BVoz624IBQif%2BxjA%3D&amp;reserved=0,
it doesn't look for the SevEs CPUID bit.

} else {
   /* For SEV, check the SEV MSR */
   msr = __rdmsr(MSR_AMD64_SEV);
   if (!(msr & MSR_AMD64_SEV_ENABLED))
     return;
   /* SEV state cannot be controlled by a command line option */
   sme_me_mask = me_mask;
   sme_me_status |= SEV_ACTIVE;
   physical_mask &= ~sme_me_mask;
(quoted diff) +5 -0
+
+  if (!(msr & MSR_AMD64_SEV_ES_ENABLED))
+    return;
+
+  sme_me_status |= SEV_ES_ACTIVE;
   return;

}

}

Quoted 45 lines · Jim Mattson

Does SEV-ES indicate that SEV-ES guests are supported, or that the current (v)CPU is running with SEV-ES enabled, or both?

Quoted 40 lines · Brijesh Singh
Quoted 16 lines · Peter Gonda

Only pass through guest relevant CPUID information: Cbit location and SEV bit. The kernel does not support nested SEV guests so the other data in this CPUID leaf is unneeded by the guest.

Suggested-by Jim Mattson <[email protected]>
Signed-off-by Peter Gonda <[email protected]>
Reviewed-by Jim Mattson <[email protected]>
---
  arch/x86/kvm/cpuid.c | 8 +++++++-
  1 file changed, 7 insertions(+), 1 deletion(-)
arch/x86/kvm/cpuid.c +0 -0
index 946fa9cb9dd6..6439fb1dbe76 100644

arch/x86/kvm/cpuid.c static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function, · line 785

Quoted 7 lines
arch/x86/kvm/cpuid.c +3 -1
@@ -780,8 +780,14 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
               break;
       /* Support memory encryption cpuid if host supports it */
       case 0x8000001F:
-             if (!boot_cpu_has(X86_FEATURE_SEV))
+             if (boot_cpu_has(X86_FEATURE_SEV)) {
+                     /* Expose only SEV bit and CBit location */
+                     entry->eax &= F(SEV);

I know SEV-ES patches are not accepted yet, but can I ask to pass the SEV-ES bit in eax?

Quoted 8 lines
arch/x86/kvm/cpuid.c +4 -0
+                     entry->ebx &= GENMASK(5, 0);
+                     entry->edx = entry->ecx = 0;
+             } else {
                       entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
+             }
               break;
       /*Add support for Centaur's CPUID instruction*/
       case 0xC0000000:
#12 lore raw

Re: [PATCH 2/2] KVM x86: Mask memory encryption guest cpuid

Quoted 11 lines · Jim Mattson
Quoted 6 lines · Brijesh Singh

Ah, I missed the fact that we don't need to pass the SevES bit to the guest because guest actually does not need it. It just needs the SevBit to make decision whether its safe to call the RDMSR for SEV_STATUS. The SEV_STATUS MSR will give information which SEV feature is enabled.

Why does it have to be safe to read the SEV_STATUS MSR? We read nonexistent MSRs all the time.

The MSR access happens very early in the boot, IIRC calling this MSR on non AMD platform may result in #GP. If OS is not ready to handle the #GP so early then we will have problem.

Quoted 80 lines
Quoted 80 lines

thanks

Quoted 77 lines · Peter Gonda

I am not sure that the SevEs CPUID bit has the same problem as the Sev bit. It seems the reason the Sev bit was to be passed to the guest was to prevent the guest from reading the SEV MSR if it did not exist. If the guest is running with SevEs it must be also running with Sev. So the guest can safely read the SevStatus MSR to check the SevEsEnabled bit because the Sev CPUID bit will be set.

If I look at the AMD patches for ES. I see just that,
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Flinux%2Fcommit%2Fc19d84b803caf8e3130b1498868d0fcafc755da7&amp;data=02%7C01%7Cbrijesh.singh%40amd.com%7C86545e99d62e4f8e8eb508d76f92720c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637100547082927245&amp;sdata=5YsknSUmboS95T0OfLWvJ%2BcOQQk5sIllGfshNqf0j6Y%3D&amp;reserved=0,
it doesn't look for the SevEs CPUID bit.

} else {
    /* For SEV, check the SEV MSR */
    msr = __rdmsr(MSR_AMD64_SEV);
    if (!(msr & MSR_AMD64_SEV_ENABLED))
      return;
    /* SEV state cannot be controlled by a command line option */
    sme_me_mask = me_mask;
    sme_me_status |= SEV_ACTIVE;
    physical_mask &= ~sme_me_mask;
(quoted diff) +5 -0
+
+  if (!(msr & MSR_AMD64_SEV_ES_ENABLED))
+    return;
+
+  sme_me_status |= SEV_ES_ACTIVE;
    return;

}

}

Quoted 45 lines · Jim Mattson

Does SEV-ES indicate that SEV-ES guests are supported, or that the current (v)CPU is running with SEV-ES enabled, or both?

Quoted 40 lines · Brijesh Singh
Quoted 16 lines · Peter Gonda

Only pass through guest relevant CPUID information: Cbit location and SEV bit. The kernel does not support nested SEV guests so the other data in this CPUID leaf is unneeded by the guest.

Suggested-by Jim Mattson <[email protected]>
Signed-off-by Peter Gonda <[email protected]>
Reviewed-by Jim Mattson <[email protected]>
---
   arch/x86/kvm/cpuid.c | 8 +++++++-
   1 file changed, 7 insertions(+), 1 deletion(-)
arch/x86/kvm/cpuid.c +0 -0
index 946fa9cb9dd6..6439fb1dbe76 100644

arch/x86/kvm/cpuid.c static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function, · line 785

Quoted 7 lines
arch/x86/kvm/cpuid.c +3 -1
@@ -780,8 +780,14 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
                break;
        /* Support memory encryption cpuid if host supports it */
        case 0x8000001F:
-             if (!boot_cpu_has(X86_FEATURE_SEV))
+             if (boot_cpu_has(X86_FEATURE_SEV)) {
+                     /* Expose only SEV bit and CBit location */
+                     entry->eax &= F(SEV);

I know SEV-ES patches are not accepted yet, but can I ask to pass the SEV-ES bit in eax?

Quoted 8 lines
arch/x86/kvm/cpuid.c +4 -0
+                     entry->ebx &= GENMASK(5, 0);
+                     entry->edx = entry->ecx = 0;
+             } else {
                        entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
+             }
                break;
        /*Add support for Centaur's CPUID instruction*/
        case 0xC0000000:
#13 lore raw

Re: [PATCH 2/2] KVM x86: Mask memory encryption guest cpuid

Quoted 12 lines · Brijesh Singh
Quoted 8 lines · Jim Mattson
Quoted 6 lines · Brijesh Singh

Ah, I missed the fact that we don't need to pass the SevES bit to the guest because guest actually does not need it. It just needs the SevBit to make decision whether its safe to call the RDMSR for SEV_STATUS. The SEV_STATUS MSR will give information which SEV feature is enabled.

Quoted 7 lines
Quoted 3 lines

Why does it have to be safe to read the SEV_STATUS MSR? We read nonexistent MSRs all the time.

The MSR access happens very early in the boot, IIRC calling this MSR on non AMD platform may result in #GP. If OS is not ready to handle the #GP so early then we will have problem.

Ah. So, the SEV CPUID bit simply indicates the presence of the SEV_STATUS MSR. Nothing more, nothing less.

Quoted 82 lines
Quoted 82 lines
Quoted 82 lines

thanks

Quoted 77 lines · Peter Gonda

I am not sure that the SevEs CPUID bit has the same problem as the Sev bit. It seems the reason the Sev bit was to be passed to the guest was to prevent the guest from reading the SEV MSR if it did not exist. If the guest is running with SevEs it must be also running with Sev. So the guest can safely read the SevStatus MSR to check the SevEsEnabled bit because the Sev CPUID bit will be set.

If I look at the AMD patches for ES. I see just that,
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Flinux%2Fcommit%2Fc19d84b803caf8e3130b1498868d0fcafc755da7&amp;data=02%7C01%7Cbrijesh.singh%40amd.com%7C86545e99d62e4f8e8eb508d76f92720c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637100547082927245&amp;sdata=5YsknSUmboS95T0OfLWvJ%2BcOQQk5sIllGfshNqf0j6Y%3D&amp;reserved=0,
it doesn't look for the SevEs CPUID bit.

} else {
    /* For SEV, check the SEV MSR */
    msr = __rdmsr(MSR_AMD64_SEV);
    if (!(msr & MSR_AMD64_SEV_ENABLED))
      return;
    /* SEV state cannot be controlled by a command line option */
    sme_me_mask = me_mask;
    sme_me_status |= SEV_ACTIVE;
    physical_mask &= ~sme_me_mask;
(quoted diff) +5 -0
+
+  if (!(msr & MSR_AMD64_SEV_ES_ENABLED))
+    return;
+
+  sme_me_status |= SEV_ES_ACTIVE;
    return;

}

}

Quoted 45 lines · Jim Mattson

Does SEV-ES indicate that SEV-ES guests are supported, or that the current (v)CPU is running with SEV-ES enabled, or both?

Quoted 40 lines · Brijesh Singh
Quoted 16 lines · Peter Gonda

Only pass through guest relevant CPUID information: Cbit location and SEV bit. The kernel does not support nested SEV guests so the other data in this CPUID leaf is unneeded by the guest.

Suggested-by Jim Mattson <[email protected]>
Signed-off-by Peter Gonda <[email protected]>
Reviewed-by Jim Mattson <[email protected]>
---
   arch/x86/kvm/cpuid.c | 8 +++++++-
   1 file changed, 7 insertions(+), 1 deletion(-)
arch/x86/kvm/cpuid.c +0 -0
index 946fa9cb9dd6..6439fb1dbe76 100644

arch/x86/kvm/cpuid.c static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function, · line 785

Quoted 7 lines
arch/x86/kvm/cpuid.c +3 -1
@@ -780,8 +780,14 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
                break;
        /* Support memory encryption cpuid if host supports it */
        case 0x8000001F:
-             if (!boot_cpu_has(X86_FEATURE_SEV))
+             if (boot_cpu_has(X86_FEATURE_SEV)) {
+                     /* Expose only SEV bit and CBit location */
+                     entry->eax &= F(SEV);

I know SEV-ES patches are not accepted yet, but can I ask to pass the SEV-ES bit in eax?

Quoted 8 lines
arch/x86/kvm/cpuid.c +4 -0
+                     entry->ebx &= GENMASK(5, 0);
+                     entry->edx = entry->ecx = 0;
+             } else {
                        entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
+             }
                break;
        /*Add support for Centaur's CPUID instruction*/
        case 0xC0000000:
Reviewed-by on the v1 series
#14 lore raw
In reply to Peter Gonda
Quoted 5 lines · Peter Gonda

KVM_GET_SUPPORTED_CPUID for 0x8000001F currently passes through all data if X86_FEATURE_SEV is enabled. Guests only need the SEV bit and Cbit location to work correctly. This series moves handing of this cpuid function out of svm.c to the general x86 function and masks out host data.

Quoted 8 lines

Peter Gonda (2):
KVM x86: Move kvm cpuid support out of svm
KVM x86: Mask memory encryption guest cpuid

  arch/x86/kvm/cpuid.c | 11 +++++++++++
  arch/x86/kvm/svm.c   |  7 -------
  2 files changed, 11 insertions(+), 7 deletions(-)
Reviewed-by Brijesh Singh <[email protected]>