Re: [PATCH v4 53/66] i386/tdx: Wire TDX_REPORT_FATAL_ERROR with GuestPanic facility

[Markus Armbruster]

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

> On 2/27/2024 7:51 PM, Markus Armbruster wrote:
>> Xiaoyao Li <xiaoyao.li@intel.com> writes:
>> 
>>> On 2/19/2024 8:53 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 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   | 28 ++++++++++++++++++++--
>>>>>    system/runstate.c     | 54 +++++++++++++++++++++++++++++++++++++++++++
>>>>>    target/i386/kvm/tdx.c | 24 ++++++++++++++++++-
>>>>>    3 files changed, 103 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/qapi/run-state.json b/qapi/run-state.json
>>>>> index 08bc99cb8561..5429116679e3 100644
>>>>> --- a/qapi/run-state.json
>>>>> +++ b/qapi/run-state.json

[...]

>>>>> @@ -566,6 +569,27 @@
>>>>>              'psw-addr': 'uint64',
>>>>>              'reason': 'S390CrashReason'}}
>>>>>    
>>>>> +##
>>>>> +# @GuestPanicInformationTdx:
>>>>> +#
>>>>> +# TDX Guest panic information specific to TDX GCHI
>>>>> +# TDG.VP.VMCALL<ReportFatalError>.
>>>>> +#
>>>>> +# @error-code: TD-specific error code
>>>>
>>>> Where could a user find information on these error codes?
>>>
>>> TDX GHCI (Guset-host-communication-Interface)spec. It defines all the
>>> TDVMCALL leaves.
>>>
>>> 0: panic;
>>> 0x1 - 0xffffffff: reserved.
>> 
>> Would it make sense to add a reference?
>
> https://cdrdv2.intel.com/v1/dl/getContent/726792

URLs have this annoying tendency to rot.

What about

# @error-code: Error code as defined in "Guest-Hypervisor Communication
#     Interface (GHCI) Specification for Intel TDX 1.5"

>>>>> +#
>>>>> +# @gpa: guest-physical address of a page that contains additional
>>>>> +#     error data, in forms of zero-terminated string.
>>>>
>>>> "in the form of a zero-terminated string"
>>>
>>> fixed.
>>>
>>>>> +#
>>>>> +# @message: Human-readable error message provided by the guest. Not
>>>>> +#     to be trusted.
>>>>
>>>> How is this message related to the one pointed to by @gpa?
>>>
>>> In general, @message contains a brief message of the error. While @gpa
>>> (when valid) contains a verbose message.
>>>
>>> The reason why we need both is because sometime when TD guest hits a
>>> fatal error, its memory may get corrupted so we cannot pass information
>>> via @gpa. Information in @message is passed through GPRs.
>> 
>> Well, we do pass information via @gpa, always.  I guess it page's
>> contents can be corrupted.
>
> No. It's not always. the bit 63 of the error code is "GPA valid" bit. 
> @gpa is valid only when bit 63 of error code is 1.
>
> And current Linux TD guest implementation doesn't use @gpa at all.
> https://github.com/torvalds/linux/blob/45ec2f5f6ed3ec3a79ba1329ad585497cdcbe663/arch/x86/coco/tdx/tdx.c#L131
>  

Aha!

Why would we want to include @gpa when the "GPA valid" bit is off?

If we do want it, then

# @gpa: guest-physical address of a page that contains more verbose
#     error information, as zero-terminated string.  Valid when the
#     "GPA valid" bit is set in @error-code.

If we don't, then make @gpa optional, present when valid, and document
it like this:

# @gpa: guest-physical address of a page that contains more verbose
#     error information, as zero-terminated string.  Present when the
#     "GPA valid" bit is set in @error-code.

>> Perhaps something like
>> 
>>      # @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.  Note that guest
>>      #     memory corruption can corrupt the page's contents.
>> 
>>>>> +#
>>>>> +# Since: 9.0
>>>>> +##
>>>>> +{'struct': 'GuestPanicInformationTdx',
>>>>> + 'data': {'error-code': 'uint64',
>>>>> +          'gpa': 'uint64',
>>>>> +          'message': 'str'}}
>> 
>> Note that my proposed doc string has the members in a different order.
>> Recommend to use the same order here.
>> 
>>>>> +
>>>>>    ##
>>>>>    # @MEMORY_FAILURE:
>>>>>    #
>>>>
>>>> [...]
>>>>
>>