[PATCH 0/4] i386: Clean up SGX for microvm, completely

[PATCH 0/4] i386: Clean up SGX for microvm, completely

[Zhao Liu]

Hi,

Currently, only PC machine supports SGX and microvm doesn't.

The commit 13be929aff80 ("target/i386: do not crash if microvm guest
uses SGX CPUID leaves") has cleaned up the CPUID 0x12.{0x2..N} for
microvm to avoid Guest crash.

Per my comment on that commit [1], microvm deserves more cleanup to
mask off CPUID 0x12.{0x0,0x1} subleaves as well. But once I actually got
my hands on this, I realized not only we need to clean up CPUID 0x12,
but also we should clean up CPUID 0x7.0.ebx[SGX] for microvm.

Thus, I have this series to completely clean up SGX for microvm.

[1]: https://lore.kernel.org/qemu-devel/Zpcz0cFjW8extm9T@intel.com/

Thanks and Best Regards,
Zhao
---
Zhao Liu (4):
  target/i386/cpu: Remove unnecessary SGX feature words checks
  target/i386/cpu: Explicitly express SGX_LC and SGX feature words
    dependency
  target/i386/cpu: Add dependencies of CPUID 0x12 leaves
  target/i386/cpu: Mask off SGX/SGX_LC feature words for non-PC machine

 hw/i386/sgx-stub.c        |  5 ++++
 hw/i386/sgx.c             |  8 +++++++
 include/hw/i386/sgx-epc.h |  1 +
 target/i386/cpu.c         | 50 ++++++++++++++++++++++++---------------
 4 files changed, 45 insertions(+), 19 deletions(-)

-- 
2.34.1

[PATCH 1/4] target/i386/cpu: Remove unnecessary SGX feature words checks

[Zhao Liu]

CPUID.0x7.0.ebx and CPUID.0x7.0.ecx leaves have been expressed as the
feature word lists, and the Host capability support has been checked
in x86_cpu_filter_features().

Therefore, such checks on SGX feature "words" are redundant, and
the follow-up adjustments to those feature "words" will not actually
take effect.

Remove unnecessary SGX feature words related checks.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 target/i386/cpu.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 4688d140c2d9..2b3642c9b13c 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6537,8 +6537,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
     case 7:
         /* Structured Extended Feature Flags Enumeration Leaf */
         if (count == 0) {
-            uint32_t eax_0_unused, ebx_0, ecx_0, edx_0_unused;
-
             /* Maximum ECX value for sub-leaves */
             *eax = env->cpuid_level_func7;
             *ebx = env->features[FEAT_7_0_EBX]; /* Feature flags */
@@ -6548,20 +6546,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             }
             *edx = env->features[FEAT_7_0_EDX]; /* Feature flags */
 
-            /*
-             * SGX cannot be emulated in software.  If hardware does not
-             * support enabling SGX and/or SGX flexible launch control,
-             * then we need to update the VM's CPUID values accordingly.
-             */
-            x86_cpu_get_supported_cpuid(0x7, 0,
-                                        &eax_0_unused, &ebx_0,
-                                        &ecx_0, &edx_0_unused);
-            if ((*ebx & CPUID_7_0_EBX_SGX) && !(ebx_0 & CPUID_7_0_EBX_SGX)) {
-                *ebx &= ~CPUID_7_0_EBX_SGX;
-            }
-
             if ((*ecx & CPUID_7_0_ECX_SGX_LC)
-                    && (!(*ebx & CPUID_7_0_EBX_SGX) || !(ecx_0 & CPUID_7_0_ECX_SGX_LC))) {
+                    && (!(*ebx & CPUID_7_0_EBX_SGX))) {
                 *ecx &= ~CPUID_7_0_ECX_SGX_LC;
             }
         } else if (count == 1) {
-- 
2.34.1

[PATCH 2/4] target/i386/cpu: Explicitly express SGX_LC and SGX feature words dependency

[Zhao Liu]

At present, cpu_x86_cpuid() silently masks off SGX_LC if SGX is absent.

This is not proper because the user is not told about the dependency
between the two.

So explicitly define the dependency between SGX_LC and SGX feature
words, so that user could get a warning when SGX_LC is enabled but
SGX is absent.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 target/i386/cpu.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 2b3642c9b13c..7a6d0b05ce27 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1730,6 +1730,10 @@ static FeatureDep feature_dependencies[] = {
         .from = { FEAT_7_1_EAX,             CPUID_7_1_EAX_WRMSRNS },
         .to = { FEAT_7_1_EAX,               CPUID_7_1_EAX_FRED },
     },
+    {
+        .from = { FEAT_7_0_EBX,             CPUID_7_0_EBX_SGX },
+        .to = { FEAT_7_0_ECX,               CPUID_7_0_ECX_SGX_LC },
+    },
 };
 
 typedef struct X86RegisterInfo32 {
@@ -6545,11 +6549,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                 *ecx |= CPUID_7_0_ECX_OSPKE;
             }
             *edx = env->features[FEAT_7_0_EDX]; /* Feature flags */
-
-            if ((*ecx & CPUID_7_0_ECX_SGX_LC)
-                    && (!(*ebx & CPUID_7_0_EBX_SGX))) {
-                *ecx &= ~CPUID_7_0_ECX_SGX_LC;
-            }
         } else if (count == 1) {
             *eax = env->features[FEAT_7_1_EAX];
             *edx = env->features[FEAT_7_1_EDX];
-- 
2.34.1

[PATCH 3/4] target/i386/cpu: Add dependencies of CPUID 0x12 leaves

[Zhao Liu]

As SDM stated, CPUID 0x12 leaves depend on CPUID_7_0_EBX_SGX (SGX
feature word).

Since FEAT_SGX_12_0_EAX, FEAT_SGX_12_0_EBX and FEAT_SGX_12_1_EAX define
multiple feature words, add the dependencies of those registers to
report the warning to user if SGX is absent.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 target/i386/cpu.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 7a6d0b05ce27..7f55e9ba3ed8 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1734,6 +1734,18 @@ static FeatureDep feature_dependencies[] = {
         .from = { FEAT_7_0_EBX,             CPUID_7_0_EBX_SGX },
         .to = { FEAT_7_0_ECX,               CPUID_7_0_ECX_SGX_LC },
     },
+    {
+        .from = { FEAT_7_0_EBX,             CPUID_7_0_EBX_SGX },
+        .to = { FEAT_SGX_12_0_EAX,          ~0ull },
+    },
+    {
+        .from = { FEAT_7_0_EBX,             CPUID_7_0_EBX_SGX },
+        .to = { FEAT_SGX_12_0_EBX,          ~0ull },
+    },
+    {
+        .from = { FEAT_7_0_EBX,             CPUID_7_0_EBX_SGX },
+        .to = { FEAT_SGX_12_1_EAX,          ~0ull },
+    },
 };
 
 typedef struct X86RegisterInfo32 {
-- 
2.34.1

[PATCH 4/4] target/i386/cpu: Mask off SGX/SGX_LC feature words for non-PC machine

[Zhao Liu]

Only PC machine supports SGX, so mask off SGX related feature words for
non-PC machine (microvm).

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/i386/sgx-stub.c        |  5 +++++
 hw/i386/sgx.c             |  8 ++++++++
 include/hw/i386/sgx-epc.h |  1 +
 target/i386/cpu.c         | 15 +++++++++++++++
 4 files changed, 29 insertions(+)

diff --git a/hw/i386/sgx-stub.c b/hw/i386/sgx-stub.c
index 16b1dfd90bb5..38ff75e9f377 100644
--- a/hw/i386/sgx-stub.c
+++ b/hw/i386/sgx-stub.c
@@ -32,6 +32,11 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
     memset(&pcms->sgx_epc, 0, sizeof(SGXEPCState));
 }
 
+bool check_sgx_support(void)
+{
+    return false;
+}
+
 bool sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size)
 {
     return true;
diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
index 849472a12865..4900dd414a1f 100644
--- a/hw/i386/sgx.c
+++ b/hw/i386/sgx.c
@@ -266,6 +266,14 @@ void hmp_info_sgx(Monitor *mon, const QDict *qdict)
                    size);
 }
 
+bool check_sgx_support(void)
+{
+    if (!object_dynamic_cast(qdev_get_machine(), TYPE_PC_MACHINE)) {
+        return false;
+    }
+    return true;
+}
+
 bool sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size)
 {
     PCMachineState *pcms =
diff --git a/include/hw/i386/sgx-epc.h b/include/hw/i386/sgx-epc.h
index 3e00efd870c9..41d55da47999 100644
--- a/include/hw/i386/sgx-epc.h
+++ b/include/hw/i386/sgx-epc.h
@@ -58,6 +58,7 @@ typedef struct SGXEPCState {
     int nr_sections;
 } SGXEPCState;
 
+bool check_sgx_support(void);
 bool sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size);
 void sgx_epc_build_srat(GArray *table_data);
 
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 7f55e9ba3ed8..66f9737a117c 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6103,6 +6103,21 @@ uint64_t x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w)
         }
         break;
 
+    case FEAT_7_0_EBX:
+#ifndef CONFIG_USER_ONLY
+        if (!check_sgx_support()) {
+            unavail = CPUID_7_0_EBX_SGX;
+        }
+#endif
+        break;
+    case FEAT_7_0_ECX:
+#ifndef CONFIG_USER_ONLY
+        if (!check_sgx_support()) {
+            unavail = CPUID_7_0_ECX_SGX_LC;
+        }
+#endif
+        break;
+
     default:
         break;
     }
-- 
2.34.1

Re: [PATCH 0/4] i386: Clean up SGX for microvm, completely

[Paolo Bonzini]

Queued, thanks.

Paolo

Re: [PATCH 0/4] i386: Clean up SGX for microvm, completely

[Zhao Liu]

On Tue, Jul 30, 2024 at 05:44:30PM +0200, Paolo Bonzini wrote:
> Date: Tue, 30 Jul 2024 17:44:30 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 0/4] i386: Clean up SGX for microvm, completely
> X-Mailer: git-send-email 2.45.2
> 
> Queued, thanks.
> 

Thanks Paolo! BTW, could you please have a look at another 2 cleanup
seriess from me? :)

https://lore.kernel.org/qemu-devel/20240716161015.263031-1-zhao1.liu@intel.com/

and

https://lore.kernel.org/qemu-devel/20240619144215.3273989-1-zhao1.liu@intel.com/

Best Regards,
Zhao