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

[Xiaoyao Li]

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 ?

> 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:
> >   #