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

[Xiaoyao Li]

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
> > > > @@ -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.

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.

If no objection from others, I will use @error-codes (plural) 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'.
> > >
> > > > +#
> > > > +#
> > >
> > > Drop one of these two lines, please.
> > >
> > > > +# Since: 9.0
> > > > +##
> > > > +{'struct': 'GuestPanicInformationTdx',
> > > > + 'data': {'error-code': 'uint64',
> > > > +          'message': 'str',
> > > > +          '*gpa': 'uint64'}}
> > > > +
> > > >    ##
> > > >    # @MEMORY_FAILURE:
> > > >    #