Re: [PATCH v5 52/65] i386/tdx: Wire TDX_REPORT_FATAL_ERROR with GuestPanic facility

[Markus Armbruster]

Xiaoyao Li <xiaoyao.li@intel.com> writes:

> On 2/29/2024 4:51 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 in v5:
>>> - mention additional error information in gpa when it presents;
>>> - refine the documentation; (Markus)
>>>
>>> Changes in v4:
>>> - refine the documentation; (Markus)
>>>
>>> Changes in v3:
>>> - Add docmentation of new type and struct; (Daniel)
>>> - refine the error message handling; (Daniel)
>>> ---
>>>   qapi/run-state.json   | 31 +++++++++++++++++++++--
>>>   system/runstate.c     | 58 +++++++++++++++++++++++++++++++++++++++++++
>>>   target/i386/kvm/tdx.c | 24 +++++++++++++++++-
>>>   3 files changed, 110 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/qapi/run-state.json b/qapi/run-state.json
>>> index dd0770b379e5..b71dd1884eb6 100644
>>> --- a/qapi/run-state.json
>>> +++ b/qapi/run-state.json
>>> @@ -483,10 +483,12 @@
>>>  #
>>>  # @s390: s390 guest panic information type (Since: 2.12)
>>>  #
>>> +# @tdx: tdx guest panic information type (Since: 9.0)
>>> +#
>>>  # Since: 2.9
>>>  ##
>>>  { 'enum': 'GuestPanicInformationType',
>>> -  'data': [ 'hyper-v', 's390' ] }
>>> +  'data': [ 'hyper-v', 's390', 'tdx' ] }
>>>     ##
>>> # @GuestPanicInformation:
>>> @@ -501,7 +503,8 @@
>>>    'base': {'type': 'GuestPanicInformationType'},
>>>    'discriminator': 'type',
>>>    'data': {'hyper-v': 'GuestPanicInformationHyperV',
>>> -          's390': 'GuestPanicInformationS390'}}
>>> +          's390': 'GuestPanicInformationS390',
>>> +          'tdx' : 'GuestPanicInformationTdx'}}
>>>  ##
>>>  # @GuestPanicInformationHyperV:
>>> @@ -564,6 +567,30 @@
>>>             'psw-addr': 'uint64',
>>>             'reason': 'S390CrashReason'}}
>>> +##
>>> +# @GuestPanicInformationTdx:
>>> +#
>>> +# TDX Guest panic information specific to TDX, as specified in the
>>> +# "Guest-Hypervisor Communication Interface (GHCI) Specification",
>>> +# section TDG.VP.VMCALL<ReportFatalError>.
>>> +#
>>> +# @error-code: TD-specific error code
>>> +#
>>> +# @message: Human-readable error message provided by the guest. Not
>>> +#     to be trusted.
>>> +#
>>> +# @gpa: guest-physical address of a page that contains more verbose
>>> +#     error information, as zero-terminated string.  Present when the
>>> +#     "GPA valid" bit (bit 63) is set in @error-code.
>>
>> Uh, peeking at GHCI Spec section 3.4 TDG.VP.VMCALL<ReportFatalError>, I
>> see operand R12 consists of
>>
>>      bits    name                        description
>>      31:0    TD-specific error code      TD-specific error code
>>                                          Panic – 0x0.
>>                                          Values – 0x1 to 0xFFFFFFFF
>>                                          reserved.
>>      62:32   TD-specific extended        TD-specific extended error code.
>>              error code                  TD software defined.
>>      63      GPA Valid                   Set if the TD specified additional
>>                                          information in the GPA parameter
>>                                          (R13).
>> Is @error-code all of R12, or just bits 31:0?
>> If it's all of R12, description of @error-code as "TD-specific error
>> code" is misleading.
>
> We pass all of R12 to @error_code.
>
> Here it wants to use "error_code" as generic as the whole R12. Do you have 
> any better description of it ?

Sadly, the spec is of no help: it doesn't name the entire thing, only
the three sub-fields TD-specific error code, TD-specific extended error
code, GPA valid.

We could take the hint, and provide the sub-fields instead:

* @error-code contains the TD-specific error code (bits 31:0)

* @extended-error-code contains the TD-specific extended error code
  (bits 62:32)

* we don't need @gpa-valid, because it's the same as "@gpa is present"

If we decide to keep the single member, we do need another name for it.
@error-codes (plural) doesn't exactly feel wonderful, but it gives at
least a subtle hint that it's not just *the* error code.

>> If it's just bits 31:0, then 'Present when the "GPA valid" bit (bit 63)
>> is set in @error-code' is wrong.  Could go with 'Only present when the
>> guest provides this information'.
>> 
>>> +#
>>> +#
>>
>> Drop one of these two lines, please.
>> 
>>> +# Since: 9.0
>>> +##
>>> +{'struct': 'GuestPanicInformationTdx',
>>> + 'data': {'error-code': 'uint64',
>>> +          'message': 'str',
>>> +          '*gpa': 'uint64'}}
>>> +
>>>   ##
>>>   # @MEMORY_FAILURE:
>>>   #
>>