Re: [PATCH v3 57/70] i386/tdx: Wire TDX_REPORT_FATAL_ERROR with GuestPanic facility

Xiaoyao Li
2023-12-07T08:11:17Z

On 12/1/2023 7:11 PM, Markus Armbruster wrote:
> Xiaoyao Li <xiaoyao.li@intel.com> writes:
>
>> Integrate TDX's TDX_REPORT_FATAL_ERROR into QEMU GuestPanic facility
>>
>> Originated-from: Isaku Yamahata <isaku.yamahata@intel.com>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>> Changes from v2:
>> - Add docmentation of new type and struct (Daniel)
>> - refine the error message handling (Daniel)
>> ---
>> qapi/run-state.json | 27 ++++++++++++++++++++--
>> system/runstate.c | 54 +++++++++++++++++++++++++++++++++++++++++++
>> target/i386/kvm/tdx.c | 24 +++++++++++++++++--
>> 3 files changed, 101 insertions(+), 4 deletions(-)
>>
>> diff --git a/qapi/run-state.json b/qapi/run-state.json
>> index f216ba54ec4c..e18f62eaef77 100644
>> --- a/qapi/run-state.json
>> +++ b/qapi/run-state.json
>> @@ -496,10 +496,12 @@
>> #
>> # @s390: s390 guest panic information type (Since: 2.12)
>> #
>> +# @tdx: tdx guest panic information type (Since: 8.2)
>> +#
>> # Since: 2.9
>> ##
>> { 'enum': 'GuestPanicInformationType',
>> - 'data': [ 'hyper-v', 's390' ] }
>> + 'data': [ 'hyper-v', 's390', 'tdx' ] }
>>
>> ##
>> # @GuestPanicInformation:
>> @@ -514,7 +516,8 @@
>> 'base': {'type': 'GuestPanicInformationType'},
>> 'discriminator': 'type',
>> 'data': {'hyper-v': 'GuestPanicInformationHyperV',
>> - 's390': 'GuestPanicInformationS390'}}
>> + 's390': 'GuestPanicInformationS390',
>> + 'tdx' : 'GuestPanicInformationTdx'}}
>>
>> ##
>> # @GuestPanicInformationHyperV:
>> @@ -577,6 +580,26 @@
>> 'psw-addr': 'uint64',
>> 'reason': 'S390CrashReason'}}
>>
>> +##
>> +# @GuestPanicInformationTdx:
>> +#
>> +# TDX GHCI TDG.VP.VMCALL<ReportFatalError> specific guest panic information
>
> Long line. Suggest
>
> # Guest panic information specific to TDX GHCI
> # TDG.VP.VMCALL<ReportFatalError>.

As I asked in patch #52, what's the limitation of one line?

>> +#
>> +# @error-code: TD-specific error code
>> +#
>> +# @gpa: 4KB-aligned guest physical address of the page that containing
>> +# additional error data
>
> "address of a page" implies the address is page-aligned. 4KB-aligned
> feels redundant. What about
>
> # @qpa: guest-physical address of a page that contains additional
> # error data.
>
> But in what format is the "additional error data"?

it's expected to hold a zero-terminated string.

>> +#
>> +# @message: TD guest provided message string. (It's not so trustable
>> +# and cannot be assumed to be well formed because it comes from guest)
>
> guest-provided
>
> For "well-formed" to make sense, we'd need an idea of the form / syntax.
>
> If it's a human-readable error message, we could go with
>
> # @message: Human-readable error message provided by the guest. Not
> # to be trusted.
>

looks good. I will your version.

>> +#
>> +# Since: 8.2
>> +##
>> +{'struct': 'GuestPanicInformationTdx',
>> + 'data': {'error-code': 'uint64',
>> + 'gpa': 'uint64',
>> + 'message': 'str'}}
>> +
>> ##
>> # @MEMORY_FAILURE:
>> #
>
> [...]
>