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

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 3/11/2024 3:29 PM, Markus Armbruster wrote:
> Xiaoyao Li <xiaoyao.li@intel.com> writes:
> 
>> On 3/7/2024 9:51 PM, Markus Armbruster wrote:
>>> 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
> 
> [...]
> 
>>>>>> @@ -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.
>>
>> The reason we only defined one single member, is that the
>> extended-error-code is not used now, and I believe it won't be used in
>> the near future.
> 
> Aha!  Then I recommend
> 
> * @error-code contains the TD-specific error code (bits 31:0)
> 
> * Omit bits 62:32 from the reply; if we later find an actual use for
>    them, we can add a suitable member
> 
> * Omit bit 63, because it's the same as "@gpa is present"
> 
>> If no objection from others, I will use @error-codes (plural) in the
>> next version.
> 
> I recommend to keep the @error-code name, but narrow its value to the
> actual error code, i.e. bits 31:0.

It works for me. I will got this direction in the next version.

>>>>> 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'.
> 
> [...]
>