| Message ID | 20241105062408.3533704-46-xiaoyao.li@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | QEMU TDX support | expand |
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 > > > >
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");