From: Sean Christopherson <sean.j.christopherson@intel.com>

Don't get/put state of TDX VMs since accessing/mutating guest state of
production TDs is not supported.

Note, it will be allowed for a debug TD. Corresponding support will be
introduced when debug TD support is implemented in the future.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
---
 target/i386/kvm/kvm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index c39e879a77e9..e47aa32233e6 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -5254,6 +5254,11 @@ int kvm_arch_put_registers(CPUState *cpu, int level, Error **errp)
 
     assert(cpu_is_stopped(cpu) || qemu_cpu_is_self(cpu));
 
+    /* TODO: Allow accessing guest state for debug TDs. */
+    if (is_tdx_vm()) {
+        return 0;
+    }
+
     /*
      * Put MSR_IA32_FEATURE_CONTROL first, this ensures the VM gets out of VMX
      * root operation upon vCPU reset. kvm_put_msr_feature_control() should also
@@ -5368,6 +5373,12 @@ int kvm_arch_get_registers(CPUState *cs, Error **errp)
         error_setg_errno(errp, -ret, "Failed to get MP state");
         goto out;
     }
+
+    /* TODO: Allow accessing guest state for debug TDs. */
+    if (is_tdx_vm()) {
+        return 0;
+    }
+
     ret = kvm_getput_regs(cpu, 0);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Failed to get general purpose registers");
-- 
2.34.1

On 11/5/24 07:23, Xiaoyao Li wrote:
> From: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> Don't get/put state of TDX VMs since accessing/mutating guest state of
> production TDs is not supported.
> 
> Note, it will be allowed for a debug TD. Corresponding support will be
> introduced when debug TD support is implemented in the future.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>

This should be unnecessary now that QEMU has 
kvm_mark_guest_state_protected().

Paolo

> ---
>   target/i386/kvm/kvm.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index c39e879a77e9..e47aa32233e6 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -5254,6 +5254,11 @@ int kvm_arch_put_registers(CPUState *cpu, int level, Error **errp)
>   
>       assert(cpu_is_stopped(cpu) || qemu_cpu_is_self(cpu));
>   
> +    /* TODO: Allow accessing guest state for debug TDs. */
> +    if (is_tdx_vm()) {
> +        return 0;
> +    }
> +
>       /*
>        * Put MSR_IA32_FEATURE_CONTROL first, this ensures the VM gets out of VMX
>        * root operation upon vCPU reset. kvm_put_msr_feature_control() should also
> @@ -5368,6 +5373,12 @@ int kvm_arch_get_registers(CPUState *cs, Error **errp)
>           error_setg_errno(errp, -ret, "Failed to get MP state");
>           goto out;
>       }
> +
> +    /* TODO: Allow accessing guest state for debug TDs. */
> +    if (is_tdx_vm()) {
> +        return 0;
> +    }
> +
>       ret = kvm_getput_regs(cpu, 0);
>       if (ret < 0) {
>           error_setg_errno(errp, -ret, "Failed to get general purpose registers");

On 11/5/2024 5:55 PM, Paolo Bonzini wrote:
> On 11/5/24 07:23, Xiaoyao Li wrote:
>> From: Sean Christopherson <sean.j.christopherson@intel.com>
>>
>> Don't get/put state of TDX VMs since accessing/mutating guest state of
>> production TDs is not supported.
>>
>> Note, it will be allowed for a debug TD. Corresponding support will be
>> introduced when debug TD support is implemented in the future.
>>
>> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> This should be unnecessary now that QEMU has 
> kvm_mark_guest_state_protected().

Reverting this patch, we get:

tdx: tdx: error: failed to set MSR 0x174 to 0x0
tdx: ../../../go/src/tdx/tdx-qemu/target/i386/kvm/kvm.c:3859: 
kvm_buf_set_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
error: failed to set MSR 0x174 to 0x0
tdx: ../../../go/src/tdx/tdx-qemu/target/i386/kvm/kvm.c:3859: 
kvm_buf_set_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.

> Paolo
> 
>> ---
>>   target/i386/kvm/kvm.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index c39e879a77e9..e47aa32233e6 100644
>> --- a/target/i386/kvm/kvm.c
>> +++ b/target/i386/kvm/kvm.c
>> @@ -5254,6 +5254,11 @@ int kvm_arch_put_registers(CPUState *cpu, int 
>> level, Error **errp)
>>       assert(cpu_is_stopped(cpu) || qemu_cpu_is_self(cpu));
>> +    /* TODO: Allow accessing guest state for debug TDs. */
>> +    if (is_tdx_vm()) {
>> +        return 0;
>> +    }
>> +
>>       /*
>>        * Put MSR_IA32_FEATURE_CONTROL first, this ensures the VM gets 
>> out of VMX
>>        * root operation upon vCPU reset. kvm_put_msr_feature_control() 
>> should also
>> @@ -5368,6 +5373,12 @@ int kvm_arch_get_registers(CPUState *cs, Error 
>> **errp)
>>           error_setg_errno(errp, -ret, "Failed to get MP state");
>>           goto out;
>>       }
>> +
>> +    /* TODO: Allow accessing guest state for debug TDs. */
>> +    if (is_tdx_vm()) {
>> +        return 0;
>> +    }
>> +
>>       ret = kvm_getput_regs(cpu, 0);
>>       if (ret < 0) {
>>           error_setg_errno(errp, -ret, "Failed to get general purpose 
>> registers");
> 

On 11/5/24 12:25, Xiaoyao Li wrote:
> On 11/5/2024 5:55 PM, Paolo Bonzini wrote:
>> On 11/5/24 07:23, Xiaoyao Li wrote:
>>> From: Sean Christopherson <sean.j.christopherson@intel.com>
>>>
>>> Don't get/put state of TDX VMs since accessing/mutating guest state of
>>> production TDs is not supported.
>>>
>>> Note, it will be allowed for a debug TD. Corresponding support will be
>>> introduced when debug TD support is implemented in the future.
>>>
>>> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
>>
>> This should be unnecessary now that QEMU has 
>> kvm_mark_guest_state_protected().
> 
> Reverting this patch, we get:
> 
> tdx: tdx: error: failed to set MSR 0x174 to 0x0
> tdx: ../../../go/src/tdx/tdx-qemu/target/i386/kvm/kvm.c:3859: 
> kvm_buf_set_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
> error: failed to set MSR 0x174 to 0x0
> tdx: ../../../go/src/tdx/tdx-qemu/target/i386/kvm/kvm.c:3859: 
> kvm_buf_set_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
Difficult to "debug" without even a backtrace, but you might be calling 
kvm_mark_guest_state_protected() too late.  For SNP, the entry values of 
the registers are customizable, for TDX they're not.  So for TDX I think 
it should be called even before realize completes, whereas SNP only 
calls it on the first transition to RUNNING.

Paolo

On 11/5/2024 10:23 PM, Paolo Bonzini wrote:
> On 11/5/24 12:25, Xiaoyao Li wrote:
>> On 11/5/2024 5:55 PM, Paolo Bonzini wrote:
>>> On 11/5/24 07:23, Xiaoyao Li wrote:
>>>> From: Sean Christopherson <sean.j.christopherson@intel.com>
>>>>
>>>> Don't get/put state of TDX VMs since accessing/mutating guest state of
>>>> production TDs is not supported.
>>>>
>>>> Note, it will be allowed for a debug TD. Corresponding support will be
>>>> introduced when debug TD support is implemented in the future.
>>>>
>>>> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
>>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>>> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
>>>
>>> This should be unnecessary now that QEMU has 
>>> kvm_mark_guest_state_protected().
>>
>> Reverting this patch, we get:
>>
>> tdx: tdx: error: failed to set MSR 0x174 to 0x0
>> tdx: ../../../go/src/tdx/tdx-qemu/target/i386/kvm/kvm.c:3859: 
>> kvm_buf_set_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
>> error: failed to set MSR 0x174 to 0x0
>> tdx: ../../../go/src/tdx/tdx-qemu/target/i386/kvm/kvm.c:3859: 
>> kvm_buf_set_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
> Difficult to "debug" without even a backtrace, but you might be calling 
> kvm_mark_guest_state_protected() too late.  For SNP, the entry values of 
> the registers are customizable, for TDX they're not.  So for TDX I think 
> it should be called even before realize completes, whereas SNP only 
> calls it on the first transition to RUNNING.

TDX calls kvm_mark_guest_state_protected() very early in
   kvm_arch_init() -> tdx_kvm_init()

I find the call site. It's caused by kvm_arch_put_register() called in 
kvm_cpu_exec() because cpu->vcpu_dirty is set to true in kvm_create_vcpu().

Maybe we can do something like below?

8<<<<<<<<<<<<<<<<<<<<<<<<<<<<
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -457,7 +457,9 @@ int kvm_create_vcpu(CPUState *cpu)

      cpu->kvm_fd = kvm_fd;
      cpu->kvm_state = s;
-    cpu->vcpu_dirty = true;
+    if (!s->guest_state_protected) {
+        cpu->vcpu_dirty = true;
+    }
      cpu->dirty_pages = 0;
      cpu->throttle_us_per_full = 0;

> Paolo
> 

Il mer 6 nov 2024, 14:57 Xiaoyao Li <xiaoyao.li@intel.com> ha scritto:

> 8<<<<<<<<<<<<<<<<<<<<<<<<<<<<
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -457,7 +457,9 @@ int kvm_create_vcpu(CPUState *cpu)
>
>       cpu->kvm_fd = kvm_fd;
>       cpu->kvm_state = s;
> -    cpu->vcpu_dirty = true;
> +    if (!s->guest_state_protected) {
> +        cpu->vcpu_dirty = true;
> +    }
>

Yes, that works.

Paolo

      cpu->dirty_pages = 0;
>       cpu->throttle_us_per_full = 0;
>
> > Paolo
> >
>
>