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

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

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
>>>> @@ -485,10 +485,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:
>>>> @@ -503,7 +505,8 @@
>>>>     'base': {'type': 'GuestPanicInformationType'},
>>>>     'discriminator': 'type',
>>>>     'data': {'hyper-v': 'GuestPanicInformationHyperV',
>>>> -          's390': 'GuestPanicInformationS390'}}
>>>> +          's390': 'GuestPanicInformationS390',
>>>> +          'tdx' : 'GuestPanicInformationTdx'}}
>>>>    
>>>>   ##
>>>>   # @GuestPanicInformationHyperV:
>>>> @@ -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

>>>> +#
>>>> +# @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 


> 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:
>>>>    #
>>>
>>> [...]
>>>
>