[v8] QEMU TDX support

[PATCH v8 31/55] i386/cpu: introduce x86_confidential_guest_cpu_instance_init()

Posted by Xiaoyao Li 10 months, 2 weeks ago

To allow execute confidential guest specific cpu init operations.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
Changes in v6:
 - new patch;
---
 target/i386/confidential-guest.h | 11 +++++++++++
 target/i386/cpu.c                | 10 ++++++++++
 2 files changed, 21 insertions(+)

diff --git a/target/i386/confidential-guest.h b/target/i386/confidential-guest.h
index 164be7633a20..a86c42a47558 100644
--- a/target/i386/confidential-guest.h
+++ b/target/i386/confidential-guest.h
@@ -39,6 +39,7 @@ struct X86ConfidentialGuestClass {
 
     /* <public> */
     int (*kvm_type)(X86ConfidentialGuest *cg);
+    void (*cpu_instance_init)(X86ConfidentialGuest *cg, CPUState *cpu);
     uint32_t (*mask_cpuid_features)(X86ConfidentialGuest *cg, uint32_t feature, uint32_t index,
                                     int reg, uint32_t value);
 };
@@ -59,6 +60,16 @@ static inline int x86_confidential_guest_kvm_type(X86ConfidentialGuest *cg)
     }
 }
 
+static inline void x86_confidential_guest_cpu_instance_init(X86ConfidentialGuest *cg,
+                                                            CPUState *cpu)
+{
+    X86ConfidentialGuestClass *klass = X86_CONFIDENTIAL_GUEST_GET_CLASS(cg);
+
+    if (klass->cpu_instance_init) {
+        klass->cpu_instance_init(cg, cpu);
+    }
+}
+
 /**
  * x86_confidential_guest_mask_cpuid_features:
  *
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index add6430f7edd..5c69d1489365 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -36,6 +36,7 @@
 #include "hw/qdev-properties.h"
 #include "hw/i386/topology.h"
 #ifndef CONFIG_USER_ONLY
+#include "confidential-guest.h"
 #include "system/reset.h"
 #include "qapi/qapi-commands-machine-target.h"
 #include "exec/address-spaces.h"
@@ -8504,6 +8505,15 @@ static void x86_cpu_post_initfn(Object *obj)
     }
 
     accel_cpu_instance_init(CPU(obj));
+
+#ifndef CONFIG_USER_ONLY
+    MachineState *ms = MACHINE(object_dynamic_cast(qdev_get_machine(),
+                                                   TYPE_MACHINE));
+    if (ms && ms->cgs) {
+        x86_confidential_guest_cpu_instance_init(X86_CONFIDENTIAL_GUEST(ms->cgs),
+                                                 (CPU(obj)));
+    }
+#endif
 }
 
 static void x86_cpu_init_default_topo(X86CPU *cpu)
-- 
2.34.1

Re: [PATCH v8 31/55] i386/cpu: introduce x86_confidential_guest_cpu_instance_init()

Posted by Zhao Liu 9 months, 2 weeks ago

On Tue, Apr 01, 2025 at 09:01:41AM -0400, Xiaoyao Li wrote:
> Date: Tue,  1 Apr 2025 09:01:41 -0400
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: [PATCH v8 31/55] i386/cpu: introduce
>  x86_confidential_guest_cpu_instance_init()
> X-Mailer: git-send-email 2.34.1
> 
> To allow execute confidential guest specific cpu init operations.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> Changes in v6:
>  - new patch;
> ---
>  target/i386/confidential-guest.h | 11 +++++++++++
>  target/i386/cpu.c                | 10 ++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/target/i386/confidential-guest.h b/target/i386/confidential-guest.h
> index 164be7633a20..a86c42a47558 100644
> --- a/target/i386/confidential-guest.h
> +++ b/target/i386/confidential-guest.h
> @@ -39,6 +39,7 @@ struct X86ConfidentialGuestClass {
>  
>      /* <public> */
>      int (*kvm_type)(X86ConfidentialGuest *cg);
> +    void (*cpu_instance_init)(X86ConfidentialGuest *cg, CPUState *cpu);
>      uint32_t (*mask_cpuid_features)(X86ConfidentialGuest *cg, uint32_t feature, uint32_t index,
>                                      int reg, uint32_t value);
>  };
> @@ -59,6 +60,16 @@ static inline int x86_confidential_guest_kvm_type(X86ConfidentialGuest *cg)
>      }
>  }
>  
> +static inline void x86_confidential_guest_cpu_instance_init(X86ConfidentialGuest *cg,
> +                                                            CPUState *cpu)

Well, it's a so long function name.

Or maybe we can call it as "x86_confidential_guest_cpu_init" and rename
the hook as "cpu_init"?

> +{
> +    X86ConfidentialGuestClass *klass = X86_CONFIDENTIAL_GUEST_GET_CLASS(cg);
> +
> +    if (klass->cpu_instance_init) {
> +        klass->cpu_instance_init(cg, cpu);
> +    }
> +}
> +
>  /**
>   * x86_confidential_guest_mask_cpuid_features:
>   *
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index add6430f7edd..5c69d1489365 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -36,6 +36,7 @@
>  #include "hw/qdev-properties.h"
>  #include "hw/i386/topology.h"
>  #ifndef CONFIG_USER_ONLY
> +#include "confidential-guest.h"
>  #include "system/reset.h"
>  #include "qapi/qapi-commands-machine-target.h"
>  #include "exec/address-spaces.h"
> @@ -8504,6 +8505,15 @@ static void x86_cpu_post_initfn(Object *obj)
>      }
>  
>      accel_cpu_instance_init(CPU(obj));
> +
> +#ifndef CONFIG_USER_ONLY
> +    MachineState *ms = MACHINE(object_dynamic_cast(qdev_get_machine(),
> +                                                   TYPE_MACHINE));

There's no need to use object_dynamic_cast(), to cast to the basic machine
type.

MACHINE(qdev_get_machine()) is enough.

> +    if (ms && ms->cgs) {
> +        x86_confidential_guest_cpu_instance_init(X86_CONFIDENTIAL_GUEST(ms->cgs),
> +                                                 (CPU(obj)));
> +    }
> +#endif
>  }
>

Others are fine for me,

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

Re: [PATCH v8 31/55] i386/cpu: introduce x86_confidential_guest_cpu_instance_init()

Posted by Xiaoyao Li 9 months, 3 weeks ago

Hi Paolo,

On 4/1/2025 9:01 PM, Xiaoyao Li wrote:
...
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index add6430f7edd..5c69d1489365 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -36,6 +36,7 @@
>   #include "hw/qdev-properties.h"
>   #include "hw/i386/topology.h"
>   #ifndef CONFIG_USER_ONLY
> +#include "confidential-guest.h"
>   #include "system/reset.h"
>   #include "qapi/qapi-commands-machine-target.h"
>   #include "exec/address-spaces.h"
> @@ -8504,6 +8505,15 @@ static void x86_cpu_post_initfn(Object *obj)
>       }
>   
>       accel_cpu_instance_init(CPU(obj));
> +
> +#ifndef CONFIG_USER_ONLY
> +    MachineState *ms = MACHINE(object_dynamic_cast(qdev_get_machine(),
> +                                                   TYPE_MACHINE));

It leads to

   qemu-system-x86_64: ../hw/core/qdev.c:824: qdev_get_machine: 
Assertion `dev' failed.
   Aborted (core dumped)

for the case of "-cpu help" due to the assert(dev) in qdev_get_machine().

How do you want to resolve it? I can think of two:
1. remove the assert() in qdev_get_machine(). or
2. drop the callback introduce by this patch. Instead just do

    if (is_tdx_vm()) {
	tdx_cpu_instance_init();
    }

> +    if (ms && ms->cgs) {
> +        x86_confidential_guest_cpu_instance_init(X86_CONFIDENTIAL_GUEST(ms->cgs),
> +                                                 (CPU(obj)));
> +    }
> +#endif
>   }
>   
>   static void x86_cpu_init_default_topo(X86CPU *cpu)

Re: [PATCH v8 31/55] i386/cpu: introduce x86_confidential_guest_cpu_instance_init()

Posted by Zhao Liu 9 months, 2 weeks ago

On Thu, Apr 24, 2025 at 01:51:55PM +0800, Xiaoyao Li wrote:
> Date: Thu, 24 Apr 2025 13:51:55 +0800
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: Re: [PATCH v8 31/55] i386/cpu: introduce
>  x86_confidential_guest_cpu_instance_init()
> 
> Hi Paolo,
> 
> On 4/1/2025 9:01 PM, Xiaoyao Li wrote:
> ...
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index add6430f7edd..5c69d1489365 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -36,6 +36,7 @@
> >   #include "hw/qdev-properties.h"
> >   #include "hw/i386/topology.h"
> >   #ifndef CONFIG_USER_ONLY
> > +#include "confidential-guest.h"
> >   #include "system/reset.h"
> >   #include "qapi/qapi-commands-machine-target.h"
> >   #include "exec/address-spaces.h"
> > @@ -8504,6 +8505,15 @@ static void x86_cpu_post_initfn(Object *obj)
> >       }
> >       accel_cpu_instance_init(CPU(obj));
> > +
> > +#ifndef CONFIG_USER_ONLY
> > +    MachineState *ms = MACHINE(object_dynamic_cast(qdev_get_machine(),
> > +                                                   TYPE_MACHINE));
> 
> It leads to
> 
>   qemu-system-x86_64: ../hw/core/qdev.c:824: qdev_get_machine: Assertion
> `dev' failed.
>   Aborted (core dumped)
> 
> for the case of "-cpu help" due to the assert(dev) in qdev_get_machine().
> 
> How do you want to resolve it? I can think of two:
> 1. remove the assert() in qdev_get_machine(). or
> 2. drop the callback introduce by this patch. Instead just do
> 
>    if (is_tdx_vm()) {
> 	tdx_cpu_instance_init();
>    }

Sorry I missed this mail when review this patch.

What about checking `current_machine`?

@@ -8541,10 +8541,8 @@ static void x86_cpu_post_initfn(Object *obj)
     accel_cpu_instance_init(CPU(obj));

 #ifndef CONFIG_USER_ONLY
-    MachineState *ms = MACHINE(object_dynamic_cast(qdev_get_machine(),
-                                                   TYPE_MACHINE));
-    if (ms && ms->cgs) {
-        x86_confidential_guest_cpu_instance_init(X86_CONFIDENTIAL_GUEST(ms->cgs),
+    if (current_machine && current_machine->cgs) {
+        x86_confidential_guest_cpu_instance_init(X86_CONFIDENTIAL_GUEST(current_machine->cgs),
                                                  (CPU(obj)));
     }
 #endif
---

"-cpu help" is processed before machine creation. The cpu-core
(cpu_core_instance_init) also checks current_machine to avoid similar
issue.

Regards,
Zhao