Re: [PATCH v8 28/55] i386/tdx: Handle KVM_SYSTEM_EVENT_TDX_FATAL

[Xiaoyao Li]

On 4/28/2025 11:00 PM, Zhao Liu wrote:
On Tue, Apr 01, 2025 at 09:01:38AM -0400, Xiaoyao Li wrote:
Date: Tue,  1 Apr 2025 09:01:38 -0400
From: Xiaoyao Li <xiaoyao...@intel.com>
Subject: [PATCH v8 28/55] i386/tdx: Handle KVM_SYSTEM_EVENT_TDX_FATAL
X-Mailer: git-send-email 2.34.1
TD guest can use TDG.VP.VMCALL<REPORT_FATAL_ERROR> to request
termination. KVM translates such request into KVM_EXIT_SYSTEM_EVENT with
type of KVM_SYSTEM_EVENT_TDX_FATAL.

Add hanlder for such exit. Parse and print the error message, and
terminate the TD guest in the handler.

Signed-off-by: Xiaoyao Li <xiaoyao...@intel.com>
---
Changes in v8:
  - update to the new data ABI of KVM_SYSTEM_EVENT_TDX_FATAL;

Changes in v6:
  - replace the patch " i386/tdx: Handle TDG.VP.VMCALL<REPORT_FATAL_ERROR>"
    in v5;
---
  target/i386/kvm/kvm.c      | 10 +++++++++
  target/i386/kvm/tdx-stub.c |  5 +++++
  target/i386/kvm/tdx.c      | 45 ++++++++++++++++++++++++++++++++++++++
  target/i386/kvm/tdx.h      |  2 ++
  4 files changed, 62 insertions(+)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 7de5014051eb..a76f34537908 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -6128,6 +6128,16 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run 
*run)
      case KVM_EXIT_HYPERCALL:
          ret = kvm_handle_hypercall(run);
          break;
+    case KVM_EXIT_SYSTEM_EVENT:
+        switch (run->system_event.type) {
+        case KVM_SYSTEM_EVENT_TDX_FATAL:
+            ret = tdx_handle_report_fatal_error(cpu, run);
+            break;
+        default:
+            ret = -1;
+            break;
+        }
+        break;
      default:
          fprintf(stderr, "KVM: unknown exit reason %d\n", run->exit_reason);
          ret = -1;
diff --git a/target/i386/kvm/tdx-stub.c b/target/i386/kvm/tdx-stub.c
index 7748b6d0a446..720a4ff046ee 100644
--- a/target/i386/kvm/tdx-stub.c
+++ b/target/i386/kvm/tdx-stub.c
@@ -13,3 +13,8 @@ int tdx_parse_tdvf(void *flash_ptr, int size)
  {
      return -EINVAL;
  }
+
+int tdx_handle_report_fatal_error(X86CPU *cpu, struct kvm_run *run)
+{
+    return -EINVAL;
+}
diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
index f8953f598584..74b7e3ac85fe 100644
--- a/target/i386/kvm/tdx.c
+++ b/target/i386/kvm/tdx.c
@@ -607,6 +607,51 @@ int tdx_parse_tdvf(void *flash_ptr, int size)
      return tdvf_parse_metadata(&tdx_guest->tdvf, flash_ptr, size);
  }
  
+int tdx_handle_report_fatal_error(X86CPU *cpu, struct kvm_run *run)
+{
+    uint64_t error_code = run->system_event.data[R_R12];
+    uint64_t reg_mask = run->system_event.data[R_ECX];
+    char *message = NULL;
+    uint64_t *tmp;
+
+    if (error_code & 0xffff) {
+        error_report("TDX: REPORT_FATAL_ERROR: invalid error code: 0x%lx",
+                     error_code);
+        return -1;
+    }
+
+/*
+ * Only 8 registers can contain valid ASCII byte stream to form the fatal
+ * message, and their sequence is: R14, R15, RBX, RDI, RSI, R8, R9, RDX
+ */
+#define TDX_FATAL_MESSAGE_MAX        64

At least, for this macro, and TDX_REPORT_FATAL_ERROR_GPA_VALID in later
patch, could we move these simple macro definitions out of the function?

This could improve the readability for this one function.

To me, it's common that put the one-off MACRO right before the line it 
is used.

Anyway, it's not a big deal to move them out of the function. So I will 
move them for your preference.

+    if (reg_mask) {
+        message = g_malloc0(TDX_FATAL_MESSAGE_MAX + 1);
+        tmp = (uint64_t *)message;
+
+#define COPY_REG(REG)                               \
+    do {                                            \
+        if (reg_mask & BIT_ULL(REG)) {              \
+            *(tmp++) = run->system_event.data[REG]; \
+        }                                           \
+    } while (0)
+
+        COPY_REG(R_R14);
+        COPY_REG(R_R15);
+        COPY_REG(R_EBX);
+        COPY_REG(R_EDI);
+        COPY_REG(R_ESI);
+        COPY_REG(R_R8);
+        COPY_REG(R_R9);
+        COPY_REG(R_EDX);
+        *((char *)tmp) = '\0';
+    }
+#undef COPY_REG
+
+    error_report("TD guest reports fatal error. %s", message ? : "");
+    return -1;
+}
+

Otherwise,

Reviewed-by: Zhao Liu <zhao1....@intel.com>