| Message ID | 20230818095041.1973309-48-xiaoyao.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TDX QEMU support | expand |
On Fri, Aug 18, 2023 at 05:50:30AM -0400, Xiaoyao Li wrote: > Originated-from: Isaku Yamahata <isaku.yamahata@intel.com> > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > --- > qapi/run-state.json | 17 +++++++++++++-- > softmmu/runstate.c | 49 +++++++++++++++++++++++++++++++++++++++++++ > target/i386/kvm/tdx.c | 24 ++++++++++++++++++++- > 3 files changed, 87 insertions(+), 3 deletions(-) > > diff --git a/qapi/run-state.json b/qapi/run-state.json > index f216ba54ec4c..506bbe31541f 100644 > --- a/qapi/run-state.json > +++ b/qapi/run-state.json > @@ -499,7 +499,7 @@ > # Since: 2.9 > ## > { 'enum': 'GuestPanicInformationType', > - 'data': [ 'hyper-v', 's390' ] } > + 'data': [ 'hyper-v', 's390', 'tdx' ] } Missing documentation for the 'tdx' value > > ## > # @GuestPanicInformation: > @@ -514,7 +514,8 @@ > 'base': {'type': 'GuestPanicInformationType'}, > 'discriminator': 'type', > 'data': {'hyper-v': 'GuestPanicInformationHyperV', > - 's390': 'GuestPanicInformationS390'}} > + 's390': 'GuestPanicInformationS390', > + 'tdx' : 'GuestPanicInformationTdx'}} > > ## > # @GuestPanicInformationHyperV: > @@ -577,6 +578,18 @@ > 'psw-addr': 'uint64', > 'reason': 'S390CrashReason'}} > > +## > +# @GuestPanicInformationTdx: > +# > +# TDX GHCI TDG.VP.VMCALL<ReportFatalError> specific guest panic information Not documented any of the struct members. Especially please include the warning that 'message' comes from the guest and so must not be trusted, not assumed to be well formed. > +# > +# Since: 8.2 > +## > +{'struct': 'GuestPanicInformationTdx', > + 'data': {'error-code': 'uint64', > + 'gpa': 'uint64', > + 'message': 'str'}} > + > ## > # @MEMORY_FAILURE: > # > diff --git a/softmmu/runstate.c b/softmmu/runstate.c > index f3bd86281813..cab11484ed7e 100644 > --- a/softmmu/runstate.c > +++ b/softmmu/runstate.c > @@ -518,7 +518,56 @@ void qemu_system_guest_panicked(GuestPanicInformation *info) > S390CrashReason_str(info->u.s390.reason), > info->u.s390.psw_mask, > info->u.s390.psw_addr); > + } else if (info->type == GUEST_PANIC_INFORMATION_TYPE_TDX) { > + char *buf = NULL; > + bool printable = false; > + > + /* > + * Although message is defined as a json string, we shouldn't > + * unconditionally treat it as is because the guest generated it and > + * it's not necessarily trustable. > + */ > + if (info->u.tdx.message) { > + /* The caller guarantees the NUL-terminated string. */ > + int len = strlen(info->u.tdx.message); > + int i; > + > + printable = len > 0; > + for (i = 0; i < len; i++) { > + if (!(0x20 <= info->u.tdx.message[i] && > + info->u.tdx.message[i] <= 0x7e)) { > + printable = false; > + break; > + } > + } > + > + /* 3 = length of "%02x " */ > + buf = g_malloc(len * 3); > + for (i = 0; i < len; i++) { > + if (info->u.tdx.message[i] == '\0') { > + break; > + } else { > + sprintf(buf + 3 * i, "%02x ", info->u.tdx.message[i]); > + } > + } > + if (i > 0) > + /* replace the last ' '(space) to NUL */ > + buf[i * 3 - 1] = '\0'; > + else > + buf[0] = '\0'; You're building this escaped buffer but... > + } > + > + qemu_log_mask(LOG_GUEST_ERROR, > + //" TDX report fatal error:\"%s\" %s", > + " TDX report fatal error:\"%s\"" > + "error: 0x%016" PRIx64 " gpa page: 0x%016" PRIx64 "\n", > + printable ? info->u.tdx.message : "", > + //buf ? buf : "", ...then not actually using it Either delete the 'buf' code, or use it. > + info->u.tdx.error_code, > + info->u.tdx.gpa); > + g_free(buf); > } > + > qapi_free_GuestPanicInformation(info); > } > } > diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c > index f111b46dac92..7efaa13f59e2 100644 > --- a/target/i386/kvm/tdx.c > +++ b/target/i386/kvm/tdx.c > @@ -18,6 +18,7 @@ > #include "qom/object_interfaces.h" > #include "standard-headers/asm-x86/kvm_para.h" > #include "sysemu/kvm.h" > +#include "sysemu/runstate.h" > #include "sysemu/sysemu.h" > #include "exec/address-spaces.h" > #include "exec/ramblock.h" > @@ -1408,11 +1409,26 @@ static void tdx_handle_get_quote(X86CPU *cpu, struct kvm_tdx_vmcall *vmcall) > vmcall->status_code = TDG_VP_VMCALL_SUCCESS; > } > > +static void tdx_panicked_on_fatal_error(X86CPU *cpu, uint64_t error_code, > + uint64_t gpa, char *message) > +{ > + GuestPanicInformation *panic_info; > + > + panic_info = g_new0(GuestPanicInformation, 1); > + panic_info->type = GUEST_PANIC_INFORMATION_TYPE_TDX; > + panic_info->u.tdx.error_code = error_code; > + panic_info->u.tdx.gpa = gpa; > + panic_info->u.tdx.message = (char *)message; > + > + qemu_system_guest_panicked(panic_info); > +} > + > static void tdx_handle_report_fatal_error(X86CPU *cpu, > struct kvm_tdx_vmcall *vmcall) > { > uint64_t error_code = vmcall->in_r12; > char *message = NULL; > + uint64_t gpa = -1ull; > > if (error_code & 0xffff) { > error_report("invalid error code of TDG.VP.VMCALL<REPORT_FATAL_ERROR>\n"); > @@ -1441,7 +1457,13 @@ static void tdx_handle_report_fatal_error(X86CPU *cpu, > } > > error_report("TD guest reports fatal error. %s\n", message ? : ""); In tdx_panicked_on_fatal_error you're avoiding printing 'message' if it contains non-printable characters, but here you're printing it regardless. Do we still need this error_report call at all ? > - exit(1); > + > +#define TDX_REPORT_FATAL_ERROR_GPA_VALID BIT_ULL(63) > + if (error_code & TDX_REPORT_FATAL_ERROR_GPA_VALID) { > + gpa = vmcall->in_r13; Bad indent > + } > + > + tdx_panicked_on_fatal_error(cpu, error_code, gpa, message); > } > > static void tdx_handle_setup_event_notify_interrupt(X86CPU *cpu, > -- > 2.34.1 > With regards, Daniel
On 8/21/2023 5:58 PM, Daniel P. Berrangé wrote: > On Fri, Aug 18, 2023 at 05:50:30AM -0400, Xiaoyao Li wrote: >> Originated-from: Isaku Yamahata <isaku.yamahata@intel.com> >> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> >> --- >> qapi/run-state.json | 17 +++++++++++++-- >> softmmu/runstate.c | 49 +++++++++++++++++++++++++++++++++++++++++++ >> target/i386/kvm/tdx.c | 24 ++++++++++++++++++++- >> 3 files changed, 87 insertions(+), 3 deletions(-) >> >> diff --git a/qapi/run-state.json b/qapi/run-state.json >> index f216ba54ec4c..506bbe31541f 100644 >> --- a/qapi/run-state.json >> +++ b/qapi/run-state.json >> @@ -499,7 +499,7 @@ >> # Since: 2.9 >> ## >> { 'enum': 'GuestPanicInformationType', >> - 'data': [ 'hyper-v', 's390' ] } >> + 'data': [ 'hyper-v', 's390', 'tdx' ] } > > Missing documentation for the 'tdx' value > >> >> ## >> # @GuestPanicInformation: >> @@ -514,7 +514,8 @@ >> 'base': {'type': 'GuestPanicInformationType'}, >> 'discriminator': 'type', >> 'data': {'hyper-v': 'GuestPanicInformationHyperV', >> - 's390': 'GuestPanicInformationS390'}} >> + 's390': 'GuestPanicInformationS390', >> + 'tdx' : 'GuestPanicInformationTdx'}} >> >> ## >> # @GuestPanicInformationHyperV: >> @@ -577,6 +578,18 @@ >> 'psw-addr': 'uint64', >> 'reason': 'S390CrashReason'}} >> >> +## >> +# @GuestPanicInformationTdx: >> +# >> +# TDX GHCI TDG.VP.VMCALL<ReportFatalError> specific guest panic information > > Not documented any of the struct members. Especially please include > the warning that 'message' comes from the guest and so must not be > trusted, not assumed to be well formed. Will do it in next version. thanks! >> +# >> +# Since: 8.2 >> +## >> +{'struct': 'GuestPanicInformationTdx', >> + 'data': {'error-code': 'uint64', >> + 'gpa': 'uint64', >> + 'message': 'str'}} >> + >> ## >> # @MEMORY_FAILURE: >> # >> diff --git a/softmmu/runstate.c b/softmmu/runstate.c >> index f3bd86281813..cab11484ed7e 100644 >> --- a/softmmu/runstate.c >> +++ b/softmmu/runstate.c >> @@ -518,7 +518,56 @@ void qemu_system_guest_panicked(GuestPanicInformation *info) >> S390CrashReason_str(info->u.s390.reason), >> info->u.s390.psw_mask, >> info->u.s390.psw_addr); >> + } else if (info->type == GUEST_PANIC_INFORMATION_TYPE_TDX) { >> + char *buf = NULL; >> + bool printable = false; >> + >> + /* >> + * Although message is defined as a json string, we shouldn't >> + * unconditionally treat it as is because the guest generated it and >> + * it's not necessarily trustable. >> + */ >> + if (info->u.tdx.message) { >> + /* The caller guarantees the NUL-terminated string. */ >> + int len = strlen(info->u.tdx.message); >> + int i; >> + >> + printable = len > 0; >> + for (i = 0; i < len; i++) { >> + if (!(0x20 <= info->u.tdx.message[i] && >> + info->u.tdx.message[i] <= 0x7e)) { >> + printable = false; >> + break; >> + } >> + } >> + >> + /* 3 = length of "%02x " */ >> + buf = g_malloc(len * 3); >> + for (i = 0; i < len; i++) { >> + if (info->u.tdx.message[i] == '\0') { >> + break; >> + } else { >> + sprintf(buf + 3 * i, "%02x ", info->u.tdx.message[i]); >> + } >> + } >> + if (i > 0) >> + /* replace the last ' '(space) to NUL */ >> + buf[i * 3 - 1] = '\0'; >> + else >> + buf[0] = '\0'; > > You're building this escaped buffer but... > >> + } >> + >> + qemu_log_mask(LOG_GUEST_ERROR, >> + //" TDX report fatal error:\"%s\" %s", >> + " TDX report fatal error:\"%s\"" >> + "error: 0x%016" PRIx64 " gpa page: 0x%016" PRIx64 "\n", >> + printable ? info->u.tdx.message : "", >> + //buf ? buf : "", > > ...then not actually using it > > Either delete the 'buf' code, or use it. Sorry for posting some internal testing version. Does below look good to you? @@ -518,7 +518,56 @@ void qemu_system_guest_panicked(GuestPanicInformation *info) S390CrashReason_str(info->u.s390.reason), info->u.s390.psw_mask, info->u.s390.psw_addr); + } else if (info->type == GUEST_PANIC_INFORMATION_TYPE_TDX) { + bool printable = false; + char *buf = NULL; + int len = 0, i; + + /* + * Although message is defined as a json string, we shouldn't + * unconditionally treat it as is because the guest generated it and + * it's not necessarily trustable. + */ + if (info->u.tdx.message) { + /* The caller guarantees the NUL-terminated string. */ + len = strlen(info->u.tdx.message); + + printable = len > 0; + for (i = 0; i < len; i++) { + if (!(0x20 <= info->u.tdx.message[i] && + info->u.tdx.message[i] <= 0x7e)) { + printable = false; + break; + } + } + } + + if (!printable && len) { + /* 3 = length of "%02x " */ + buf = g_malloc(len * 3); + for (i = 0; i < len; i++) { + if (info->u.tdx.message[i] == '\0') { + break; + } else { + sprintf(buf + 3 * i, "%02x ", info->u.tdx.message[i]); + } + } + if (i > 0) + /* replace the last ' '(space) to NUL */ + buf[i * 3 - 1] = '\0'; + else + buf[0] = '\0'; + } + + qemu_log_mask(LOG_GUEST_ERROR, + " TDX guest reports fatal error:\"%s\"" + " error code: 0x%016" PRIx64 " gpa page: 0x%016" PRIx64 "\n", + printable ? info->u.tdx.message : buf, + info->u.tdx.error_code, + info->u.tdx.gpa); + g_free(buf); } >> + info->u.tdx.error_code, >> + info->u.tdx.gpa); >> + g_free(buf); >> } >> + >> qapi_free_GuestPanicInformation(info); >> } >> } >> diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c >> index f111b46dac92..7efaa13f59e2 100644 >> --- a/target/i386/kvm/tdx.c >> +++ b/target/i386/kvm/tdx.c >> @@ -18,6 +18,7 @@ >> #include "qom/object_interfaces.h" >> #include "standard-headers/asm-x86/kvm_para.h" >> #include "sysemu/kvm.h" >> +#include "sysemu/runstate.h" >> #include "sysemu/sysemu.h" >> #include "exec/address-spaces.h" >> #include "exec/ramblock.h" >> @@ -1408,11 +1409,26 @@ static void tdx_handle_get_quote(X86CPU *cpu, struct kvm_tdx_vmcall *vmcall) >> vmcall->status_code = TDG_VP_VMCALL_SUCCESS; >> } >> >> +static void tdx_panicked_on_fatal_error(X86CPU *cpu, uint64_t error_code, >> + uint64_t gpa, char *message) >> +{ >> + GuestPanicInformation *panic_info; >> + >> + panic_info = g_new0(GuestPanicInformation, 1); >> + panic_info->type = GUEST_PANIC_INFORMATION_TYPE_TDX; >> + panic_info->u.tdx.error_code = error_code; >> + panic_info->u.tdx.gpa = gpa; >> + panic_info->u.tdx.message = (char *)message; >> + >> + qemu_system_guest_panicked(panic_info); >> +} >> + >> static void tdx_handle_report_fatal_error(X86CPU *cpu, >> struct kvm_tdx_vmcall *vmcall) >> { >> uint64_t error_code = vmcall->in_r12; >> char *message = NULL; >> + uint64_t gpa = -1ull; >> >> if (error_code & 0xffff) { >> error_report("invalid error code of TDG.VP.VMCALL<REPORT_FATAL_ERROR>\n"); >> @@ -1441,7 +1457,13 @@ static void tdx_handle_report_fatal_error(X86CPU *cpu, >> } >> >> error_report("TD guest reports fatal error. %s\n", message ? : ""); > > In tdx_panicked_on_fatal_error you're avoiding printing 'message' if it > contains non-printable characters, but here you're printing it regardless. I guess you meant qemu_system_guest_panicked(). > Do we still need this error_report call at all ? yes. It can and should be dropped before I sent to maillist. I keep it internally for testing purpose because qemu_log_mask() doesn't get printed by default. >> - exit(1); >> + >> +#define TDX_REPORT_FATAL_ERROR_GPA_VALID BIT_ULL(63) >> + if (error_code & TDX_REPORT_FATAL_ERROR_GPA_VALID) { >> + gpa = vmcall->in_r13; > > Bad indent Fixed. >> + } >> + >> + tdx_panicked_on_fatal_error(cpu, error_code, gpa, message); >> } >> >> static void tdx_handle_setup_event_notify_interrupt(X86CPU *cpu, >> -- >> 2.34.1 >> > > With regards, > Daniel
On Mon, Aug 28, 2023 at 09:14:41PM +0800, Xiaoyao Li wrote: > On 8/21/2023 5:58 PM, Daniel P. Berrangé wrote: > > On Fri, Aug 18, 2023 at 05:50:30AM -0400, Xiaoyao Li wrote: > > > Originated-from: Isaku Yamahata <isaku.yamahata@intel.com> > > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > > > --- > > > qapi/run-state.json | 17 +++++++++++++-- > > > softmmu/runstate.c | 49 +++++++++++++++++++++++++++++++++++++++++++ > > > target/i386/kvm/tdx.c | 24 ++++++++++++++++++++- > > > 3 files changed, 87 insertions(+), 3 deletions(-) > > > > > > diff --git a/qapi/run-state.json b/qapi/run-state.json > > > index f216ba54ec4c..506bbe31541f 100644 > > > --- a/qapi/run-state.json > > > +++ b/qapi/run-state.json > > > @@ -499,7 +499,7 @@ > > > # Since: 2.9 > > > ## > > > { 'enum': 'GuestPanicInformationType', > > > - 'data': [ 'hyper-v', 's390' ] } > > > + 'data': [ 'hyper-v', 's390', 'tdx' ] } > > > > +# > > > +# Since: 8.2 > > > +## > > > +{'struct': 'GuestPanicInformationTdx', > > > + 'data': {'error-code': 'uint64', > > > + 'gpa': 'uint64', > > > + 'message': 'str'}} > > > + > > > ## > > > # @MEMORY_FAILURE: > > > # > > > diff --git a/softmmu/runstate.c b/softmmu/runstate.c > > > index f3bd86281813..cab11484ed7e 100644 > > > --- a/softmmu/runstate.c > > > +++ b/softmmu/runstate.c > > > @@ -518,7 +518,56 @@ void qemu_system_guest_panicked(GuestPanicInformation *info) > > > S390CrashReason_str(info->u.s390.reason), > > > info->u.s390.psw_mask, > > > info->u.s390.psw_addr); > > > + } else if (info->type == GUEST_PANIC_INFORMATION_TYPE_TDX) { > > > + char *buf = NULL; > > > + bool printable = false; > > > + > > > + /* > > > + * Although message is defined as a json string, we shouldn't > > > + * unconditionally treat it as is because the guest generated it and > > > + * it's not necessarily trustable. > > > + */ > > > + if (info->u.tdx.message) { > > > + /* The caller guarantees the NUL-terminated string. */ > > > + int len = strlen(info->u.tdx.message); > > > + int i; > > > + > > > + printable = len > 0; > > > + for (i = 0; i < len; i++) { > > > + if (!(0x20 <= info->u.tdx.message[i] && > > > + info->u.tdx.message[i] <= 0x7e)) { > > > + printable = false; > > > + break; > > > + } > > > + } > > > + > > > + /* 3 = length of "%02x " */ > > > + buf = g_malloc(len * 3); > > > + for (i = 0; i < len; i++) { > > > + if (info->u.tdx.message[i] == '\0') { > > > + break; > > > + } else { > > > + sprintf(buf + 3 * i, "%02x ", info->u.tdx.message[i]); > > > + } > > > + } > > > + if (i > 0) > > > + /* replace the last ' '(space) to NUL */ > > > + buf[i * 3 - 1] = '\0'; > > > + else > > > + buf[0] = '\0'; > > > > You're building this escaped buffer but... > > > > > + } > > > + > > > + qemu_log_mask(LOG_GUEST_ERROR, > > > + //" TDX report fatal error:\"%s\" %s", > > > + " TDX report fatal error:\"%s\"" > > > + "error: 0x%016" PRIx64 " gpa page: 0x%016" PRIx64 "\n", > > > + printable ? info->u.tdx.message : "", > > > + //buf ? buf : "", > > > > ...then not actually using it > > > > Either delete the 'buf' code, or use it. > > Sorry for posting some internal testing version. > Does below look good to you? > > @@ -518,7 +518,56 @@ void qemu_system_guest_panicked(GuestPanicInformation > *info) > S390CrashReason_str(info->u.s390.reason), > info->u.s390.psw_mask, > info->u.s390.psw_addr); > + } else if (info->type == GUEST_PANIC_INFORMATION_TYPE_TDX) { > + bool printable = false; > + char *buf = NULL; > + int len = 0, i; > + > + /* > + * Although message is defined as a json string, we shouldn't > + * unconditionally treat it as is because the guest generated > it and > + * it's not necessarily trustable. > + */ > + if (info->u.tdx.message) { > + /* The caller guarantees the NUL-terminated string. */ > + len = strlen(info->u.tdx.message); > + > + printable = len > 0; > + for (i = 0; i < len; i++) { > + if (!(0x20 <= info->u.tdx.message[i] && > + info->u.tdx.message[i] <= 0x7e)) { > + printable = false; > + break; > + } > + } > + } > + > + if (!printable && len) { > + /* 3 = length of "%02x " */ > + buf = g_malloc(len * 3); > + for (i = 0; i < len; i++) { > + if (info->u.tdx.message[i] == '\0') { > + break; > + } else { > + sprintf(buf + 3 * i, "%02x ", > info->u.tdx.message[i]); > + } > + } > + if (i > 0) > + /* replace the last ' '(space) to NUL */ > + buf[i * 3 - 1] = '\0'; > + else > + buf[0] = '\0'; > + } > + > + qemu_log_mask(LOG_GUEST_ERROR, > + " TDX guest reports fatal error:\"%s\"" > + " error code: 0x%016" PRIx64 " gpa page: 0x%016" > PRIx64 "\n", > + printable ? info->u.tdx.message : buf, > + info->u.tdx.error_code, > + info->u.tdx.gpa); > + g_free(buf); > } Ok that makes more sense now. BTW, probably a nice idea to create a separate helper method that santizes the guest provided JSON into the safe 'buf' string. With regards, Daniel
On 8/29/2023 6:28 PM, Daniel P. Berrangé wrote: > On Mon, Aug 28, 2023 at 09:14:41PM +0800, Xiaoyao Li wrote: >> On 8/21/2023 5:58 PM, Daniel P. Berrangé wrote: >>> On Fri, Aug 18, 2023 at 05:50:30AM -0400, Xiaoyao Li wrote: >>>> Originated-from: Isaku Yamahata <isaku.yamahata@intel.com> >>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> >>>> --- >>>> qapi/run-state.json | 17 +++++++++++++-- >>>> softmmu/runstate.c | 49 +++++++++++++++++++++++++++++++++++++++++++ >>>> target/i386/kvm/tdx.c | 24 ++++++++++++++++++++- >>>> 3 files changed, 87 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/qapi/run-state.json b/qapi/run-state.json >>>> index f216ba54ec4c..506bbe31541f 100644 >>>> --- a/qapi/run-state.json >>>> +++ b/qapi/run-state.json >>>> @@ -499,7 +499,7 @@ >>>> # Since: 2.9 >>>> ## >>>> { 'enum': 'GuestPanicInformationType', >>>> - 'data': [ 'hyper-v', 's390' ] } >>>> + 'data': [ 'hyper-v', 's390', 'tdx' ] } > >> >>>> +# >>>> +# Since: 8.2 >>>> +## >>>> +{'struct': 'GuestPanicInformationTdx', >>>> + 'data': {'error-code': 'uint64', >>>> + 'gpa': 'uint64', >>>> + 'message': 'str'}} >>>> + >>>> ## >>>> # @MEMORY_FAILURE: >>>> # >>>> diff --git a/softmmu/runstate.c b/softmmu/runstate.c >>>> index f3bd86281813..cab11484ed7e 100644 >>>> --- a/softmmu/runstate.c >>>> +++ b/softmmu/runstate.c >>>> @@ -518,7 +518,56 @@ void qemu_system_guest_panicked(GuestPanicInformation *info) >>>> S390CrashReason_str(info->u.s390.reason), >>>> info->u.s390.psw_mask, >>>> info->u.s390.psw_addr); >>>> + } else if (info->type == GUEST_PANIC_INFORMATION_TYPE_TDX) { >>>> + char *buf = NULL; >>>> + bool printable = false; >>>> + >>>> + /* >>>> + * Although message is defined as a json string, we shouldn't >>>> + * unconditionally treat it as is because the guest generated it and >>>> + * it's not necessarily trustable. >>>> + */ >>>> + if (info->u.tdx.message) { >>>> + /* The caller guarantees the NUL-terminated string. */ >>>> + int len = strlen(info->u.tdx.message); >>>> + int i; >>>> + >>>> + printable = len > 0; >>>> + for (i = 0; i < len; i++) { >>>> + if (!(0x20 <= info->u.tdx.message[i] && >>>> + info->u.tdx.message[i] <= 0x7e)) { >>>> + printable = false; >>>> + break; >>>> + } >>>> + } >>>> + >>>> + /* 3 = length of "%02x " */ >>>> + buf = g_malloc(len * 3); >>>> + for (i = 0; i < len; i++) { >>>> + if (info->u.tdx.message[i] == '\0') { >>>> + break; >>>> + } else { >>>> + sprintf(buf + 3 * i, "%02x ", info->u.tdx.message[i]); >>>> + } >>>> + } >>>> + if (i > 0) >>>> + /* replace the last ' '(space) to NUL */ >>>> + buf[i * 3 - 1] = '\0'; >>>> + else >>>> + buf[0] = '\0'; >>> >>> You're building this escaped buffer but... >>> >>>> + } >>>> + >>>> + qemu_log_mask(LOG_GUEST_ERROR, >>>> + //" TDX report fatal error:\"%s\" %s", >>>> + " TDX report fatal error:\"%s\"" >>>> + "error: 0x%016" PRIx64 " gpa page: 0x%016" PRIx64 "\n", >>>> + printable ? info->u.tdx.message : "", >>>> + //buf ? buf : "", >>> >>> ...then not actually using it >>> >>> Either delete the 'buf' code, or use it. >> >> Sorry for posting some internal testing version. >> Does below look good to you? >> >> @@ -518,7 +518,56 @@ void qemu_system_guest_panicked(GuestPanicInformation >> *info) >> S390CrashReason_str(info->u.s390.reason), >> info->u.s390.psw_mask, >> info->u.s390.psw_addr); >> + } else if (info->type == GUEST_PANIC_INFORMATION_TYPE_TDX) { >> + bool printable = false; >> + char *buf = NULL; >> + int len = 0, i; >> + >> + /* >> + * Although message is defined as a json string, we shouldn't >> + * unconditionally treat it as is because the guest generated >> it and >> + * it's not necessarily trustable. >> + */ >> + if (info->u.tdx.message) { >> + /* The caller guarantees the NUL-terminated string. */ >> + len = strlen(info->u.tdx.message); >> + >> + printable = len > 0; >> + for (i = 0; i < len; i++) { >> + if (!(0x20 <= info->u.tdx.message[i] && >> + info->u.tdx.message[i] <= 0x7e)) { >> + printable = false; >> + break; >> + } >> + } >> + } >> + >> + if (!printable && len) { >> + /* 3 = length of "%02x " */ >> + buf = g_malloc(len * 3); >> + for (i = 0; i < len; i++) { >> + if (info->u.tdx.message[i] == '\0') { >> + break; >> + } else { >> + sprintf(buf + 3 * i, "%02x ", >> info->u.tdx.message[i]); >> + } >> + } >> + if (i > 0) >> + /* replace the last ' '(space) to NUL */ >> + buf[i * 3 - 1] = '\0'; >> + else >> + buf[0] = '\0'; >> + } >> + >> + qemu_log_mask(LOG_GUEST_ERROR, >> + " TDX guest reports fatal error:\"%s\"" >> + " error code: 0x%016" PRIx64 " gpa page: 0x%016" >> PRIx64 "\n", >> + printable ? info->u.tdx.message : buf, >> + info->u.tdx.error_code, >> + info->u.tdx.gpa); >> + g_free(buf); >> } > > > Ok that makes more sense now. BTW, probably a nice idea to create a > separate helper method that santizes the guest provided JSON into > the safe 'buf' string. > OK. Thanks for the suggestion. Will do it in next version. > With regards, > Daniel
diff --git a/qapi/run-state.json b/qapi/run-state.json index f216ba54ec4c..506bbe31541f 100644 --- a/qapi/run-state.json +++ b/qapi/run-state.json @@ -499,7 +499,7 @@ # Since: 2.9 ## { 'enum': 'GuestPanicInformationType', - 'data': [ 'hyper-v', 's390' ] } + 'data': [ 'hyper-v', 's390', 'tdx' ] } ## # @GuestPanicInformation: @@ -514,7 +514,8 @@ 'base': {'type': 'GuestPanicInformationType'}, 'discriminator': 'type', 'data': {'hyper-v': 'GuestPanicInformationHyperV', - 's390': 'GuestPanicInformationS390'}} + 's390': 'GuestPanicInformationS390', + 'tdx' : 'GuestPanicInformationTdx'}} ## # @GuestPanicInformationHyperV: @@ -577,6 +578,18 @@ 'psw-addr': 'uint64', 'reason': 'S390CrashReason'}} +## +# @GuestPanicInformationTdx: +# +# TDX GHCI TDG.VP.VMCALL<ReportFatalError> specific guest panic information +# +# Since: 8.2 +## +{'struct': 'GuestPanicInformationTdx', + 'data': {'error-code': 'uint64', + 'gpa': 'uint64', + 'message': 'str'}} + ## # @MEMORY_FAILURE: # diff --git a/softmmu/runstate.c b/softmmu/runstate.c index f3bd86281813..cab11484ed7e 100644 --- a/softmmu/runstate.c +++ b/softmmu/runstate.c @@ -518,7 +518,56 @@ void qemu_system_guest_panicked(GuestPanicInformation *info) S390CrashReason_str(info->u.s390.reason), info->u.s390.psw_mask, info->u.s390.psw_addr); + } else if (info->type == GUEST_PANIC_INFORMATION_TYPE_TDX) { + char *buf = NULL; + bool printable = false; + + /* + * Although message is defined as a json string, we shouldn't + * unconditionally treat it as is because the guest generated it and + * it's not necessarily trustable. + */ + if (info->u.tdx.message) { + /* The caller guarantees the NUL-terminated string. */ + int len = strlen(info->u.tdx.message); + int i; + + printable = len > 0; + for (i = 0; i < len; i++) { + if (!(0x20 <= info->u.tdx.message[i] && + info->u.tdx.message[i] <= 0x7e)) { + printable = false; + break; + } + } + + /* 3 = length of "%02x " */ + buf = g_malloc(len * 3); + for (i = 0; i < len; i++) { + if (info->u.tdx.message[i] == '\0') { + break; + } else { + sprintf(buf + 3 * i, "%02x ", info->u.tdx.message[i]); + } + } + if (i > 0) + /* replace the last ' '(space) to NUL */ + buf[i * 3 - 1] = '\0'; + else + buf[0] = '\0'; + } + + qemu_log_mask(LOG_GUEST_ERROR, + //" TDX report fatal error:\"%s\" %s", + " TDX report fatal error:\"%s\"" + "error: 0x%016" PRIx64 " gpa page: 0x%016" PRIx64 "\n", + printable ? info->u.tdx.message : "", + //buf ? buf : "", + info->u.tdx.error_code, + info->u.tdx.gpa); + g_free(buf); } + qapi_free_GuestPanicInformation(info); } } diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c index f111b46dac92..7efaa13f59e2 100644 --- a/target/i386/kvm/tdx.c +++ b/target/i386/kvm/tdx.c @@ -18,6 +18,7 @@ #include "qom/object_interfaces.h" #include "standard-headers/asm-x86/kvm_para.h" #include "sysemu/kvm.h" +#include "sysemu/runstate.h" #include "sysemu/sysemu.h" #include "exec/address-spaces.h" #include "exec/ramblock.h" @@ -1408,11 +1409,26 @@ static void tdx_handle_get_quote(X86CPU *cpu, struct kvm_tdx_vmcall *vmcall) vmcall->status_code = TDG_VP_VMCALL_SUCCESS; } +static void tdx_panicked_on_fatal_error(X86CPU *cpu, uint64_t error_code, + uint64_t gpa, char *message) +{ + GuestPanicInformation *panic_info; + + panic_info = g_new0(GuestPanicInformation, 1); + panic_info->type = GUEST_PANIC_INFORMATION_TYPE_TDX; + panic_info->u.tdx.error_code = error_code; + panic_info->u.tdx.gpa = gpa; + panic_info->u.tdx.message = (char *)message; + + qemu_system_guest_panicked(panic_info); +} + static void tdx_handle_report_fatal_error(X86CPU *cpu, struct kvm_tdx_vmcall *vmcall) { uint64_t error_code = vmcall->in_r12; char *message = NULL; + uint64_t gpa = -1ull; if (error_code & 0xffff) { error_report("invalid error code of TDG.VP.VMCALL<REPORT_FATAL_ERROR>\n"); @@ -1441,7 +1457,13 @@ static void tdx_handle_report_fatal_error(X86CPU *cpu, } error_report("TD guest reports fatal error. %s\n", message ? : ""); - exit(1); + +#define TDX_REPORT_FATAL_ERROR_GPA_VALID BIT_ULL(63) + if (error_code & TDX_REPORT_FATAL_ERROR_GPA_VALID) { + gpa = vmcall->in_r13; + } + + tdx_panicked_on_fatal_error(cpu, error_code, gpa, message); } static void tdx_handle_setup_event_notify_interrupt(X86CPU *cpu,
Originated-from: Isaku Yamahata <isaku.yamahata@intel.com> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> --- qapi/run-state.json | 17 +++++++++++++-- softmmu/runstate.c | 49 +++++++++++++++++++++++++++++++++++++++++++ target/i386/kvm/tdx.c | 24 ++++++++++++++++++++- 3 files changed, 87 insertions(+), 3 deletions(-)