[PATCH] i386/tdx: Fix the report of gpa in QAPI

[PATCH] i386/tdx: Fix the report of gpa in QAPI

[Zhenzhong Duan]

Gpa is defined in QAPI but never reported to monitor because has_gpa is
never set to ture.

Fix it by setting has_gpa to ture when TDX_REPORT_FATAL_ERROR_GPA_VALID
is set in error_code.

Fixes: 6e250463b08b ("i386/tdx: Wire TDX_REPORT_FATAL_ERROR with GuestPanic facility")
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 target/i386/kvm/tdx.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
index e809e4b2df..370a9b6e65 100644
--- a/target/i386/kvm/tdx.c
+++ b/target/i386/kvm/tdx.c
@@ -1269,7 +1269,8 @@ void tdx_handle_get_tdvmcall_info(X86CPU *cpu, struct kvm_run *run)
 }
 
 static void tdx_panicked_on_fatal_error(X86CPU *cpu, uint64_t error_code,
-                                        char *message, uint64_t gpa)
+                                        char *message, bool has_gpa,
+                                        uint64_t gpa)
 {
     GuestPanicInformation *panic_info;
 
@@ -1278,6 +1279,7 @@ static void tdx_panicked_on_fatal_error(X86CPU *cpu, uint64_t error_code,
     panic_info->u.tdx.error_code = (uint32_t) error_code;
     panic_info->u.tdx.message = message;
     panic_info->u.tdx.gpa = gpa;
+    panic_info->u.tdx.has_gpa = has_gpa;
 
     qemu_system_guest_panicked(panic_info);
 }
@@ -1297,6 +1299,7 @@ int tdx_handle_report_fatal_error(X86CPU *cpu, struct kvm_run *run)
     char *message = NULL;
     uint64_t *tmp;
     uint64_t gpa = -1ull;
+    bool has_gpa = false;
 
     if (error_code & 0xffff) {
         error_report("TDX: REPORT_FATAL_ERROR: invalid error code: 0x%"PRIx64,
@@ -1329,9 +1332,10 @@ int tdx_handle_report_fatal_error(X86CPU *cpu, struct kvm_run *run)
 
     if (error_code & TDX_REPORT_FATAL_ERROR_GPA_VALID) {
         gpa = run->system_event.data[R_R13];
+        has_gpa = true;
     }
 
-    tdx_panicked_on_fatal_error(cpu, error_code, message, gpa);
+    tdx_panicked_on_fatal_error(cpu, error_code, message, has_gpa, gpa);
 
     return -1;
 }
-- 
2.47.1

Re: [PATCH] i386/tdx: Fix the report of gpa in QAPI

[Daniel P. Berrangé]

On Wed, Jul 09, 2025 at 11:55:38PM -0400, Zhenzhong Duan wrote:
> Gpa is defined in QAPI but never reported to monitor because has_gpa is
> never set to ture.
> 
> Fix it by setting has_gpa to ture when TDX_REPORT_FATAL_ERROR_GPA_VALID
> is set in error_code.
> 
> Fixes: 6e250463b08b ("i386/tdx: Wire TDX_REPORT_FATAL_ERROR with GuestPanic facility")
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  target/i386/kvm/tdx.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] i386/tdx: Fix the report of gpa in QAPI

[Paolo Bonzini]

Queued, thanks.

Paolo

Re: [PATCH] i386/tdx: Fix the report of gpa in QAPI

[Xiaoyao Li]

On 7/10/2025 11:55 AM, Zhenzhong Duan wrote:
> Gpa is defined in QAPI but never reported to monitor because has_gpa is
> never set to ture.
> 
> Fix it by setting has_gpa to ture when TDX_REPORT_FATAL_ERROR_GPA_VALID
> is set in error_code.

Hi Zhenzhong,

I would like to understand what the problem is without 
panic_info->u.tdx.has_gpa being set?

> Fixes: 6e250463b08b ("i386/tdx: Wire TDX_REPORT_FATAL_ERROR with GuestPanic facility")
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   target/i386/kvm/tdx.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
> index e809e4b2df..370a9b6e65 100644
> --- a/target/i386/kvm/tdx.c
> +++ b/target/i386/kvm/tdx.c
> @@ -1269,7 +1269,8 @@ void tdx_handle_get_tdvmcall_info(X86CPU *cpu, struct kvm_run *run)
>   }
>   
>   static void tdx_panicked_on_fatal_error(X86CPU *cpu, uint64_t error_code,
> -                                        char *message, uint64_t gpa)
> +                                        char *message, bool has_gpa,
> +                                        uint64_t gpa)
>   {
>       GuestPanicInformation *panic_info;
>   
> @@ -1278,6 +1279,7 @@ static void tdx_panicked_on_fatal_error(X86CPU *cpu, uint64_t error_code,
>       panic_info->u.tdx.error_code = (uint32_t) error_code;
>       panic_info->u.tdx.message = message;
>       panic_info->u.tdx.gpa = gpa;
> +    panic_info->u.tdx.has_gpa = has_gpa;
>   
>       qemu_system_guest_panicked(panic_info);
>   }
> @@ -1297,6 +1299,7 @@ int tdx_handle_report_fatal_error(X86CPU *cpu, struct kvm_run *run)
>       char *message = NULL;
>       uint64_t *tmp;
>       uint64_t gpa = -1ull;
> +    bool has_gpa = false;
>   
>       if (error_code & 0xffff) {
>           error_report("TDX: REPORT_FATAL_ERROR: invalid error code: 0x%"PRIx64,
> @@ -1329,9 +1332,10 @@ int tdx_handle_report_fatal_error(X86CPU *cpu, struct kvm_run *run)
>   
>       if (error_code & TDX_REPORT_FATAL_ERROR_GPA_VALID) {
>           gpa = run->system_event.data[R_R13];
> +        has_gpa = true;
>       }
>   
> -    tdx_panicked_on_fatal_error(cpu, error_code, message, gpa);
> +    tdx_panicked_on_fatal_error(cpu, error_code, message, has_gpa, gpa);
>   
>       return -1;
>   }

Re: [PATCH] i386/tdx: Fix the report of gpa in QAPI

[Xiaoyao Li]

On 7/10/2025 7:40 PM, Xiaoyao Li wrote:
> On 7/10/2025 11:55 AM, Zhenzhong Duan wrote:
>> Gpa is defined in QAPI but never reported to monitor because has_gpa is
>> never set to ture.
>>
>> Fix it by setting has_gpa to ture when TDX_REPORT_FATAL_ERROR_GPA_VALID
>> is set in error_code.
> 
> Hi Zhenzhong,
> 
> I would like to understand what the problem is without panic_info- 
>  >u.tdx.has_gpa being set?

Never mind, I figure it out.

qapi_event_send_guest_panicked() in qemu_system_guest_panicked will send 
the event and data. Libvirt tries to parse the data, but find the 
.has_data field is not set correctly.

My original patch used (gpa == -1) as the indicator for whether gpa is 
valid, this needs to be cleaned up. I will send the cleanup patch.

Re: [PATCH] i386/tdx: Fix the report of gpa in QAPI

[Daniel P. Berrangé]

On Thu, Jul 10, 2025 at 10:06:10PM +0800, Xiaoyao Li wrote:
> On 7/10/2025 7:40 PM, Xiaoyao Li wrote:
> > On 7/10/2025 11:55 AM, Zhenzhong Duan wrote:
> > > Gpa is defined in QAPI but never reported to monitor because has_gpa is
> > > never set to ture.
> > > 
> > > Fix it by setting has_gpa to ture when TDX_REPORT_FATAL_ERROR_GPA_VALID
> > > is set in error_code.
> > 
> > Hi Zhenzhong,
> > 
> > I would like to understand what the problem is without panic_info-
> > >u.tdx.has_gpa being set?
> 
> Never mind, I figure it out.
> 
> qapi_event_send_guest_panicked() in qemu_system_guest_panicked will send the
> event and data. Libvirt tries to parse the data, but find the .has_data
> field is not set correctly.

Close, but not quite.

The "has_" fields are only present in the QEMU internal data structures,
never on the wire in the JSON messages that libvirt receives.

The problem is with QEMU's struct -> JSON serialization - if the 'has_gpa'
field is not set to 'true', then QEMU will *never* emit the 'gpa' field
in the JSON it sends to the client app.

So what libvirt receives is this:

 {"timestamp": {"seconds": 1752156518, "microseconds": 809325},
  "event": "GUEST_PANICKED",
  "data": {"action": "pause", "info": {
      "error-code": 0,
      "message": "TD misconfiguration: SEPT #VE has to be disabled",
      "type": "tdx"}}}

which has no 'gpa' present.

> My original patch used (gpa == -1) as the indicator for whether gpa is
> valid, this needs to be cleaned up. I will send the cleanup patch.

The value you assign to 'gpa' doesn't matter when 'has_gpa' is false,
as it'll never get into the JSON event, so having it be '-1' is not
significantly different from leaving it on 0.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] i386/tdx: Fix the report of gpa in QAPI

[Xiaoyao Li]

On 7/10/2025 10:11 PM, Daniel P. Berrangé wrote:
> On Thu, Jul 10, 2025 at 10:06:10PM +0800, Xiaoyao Li wrote:
>> On 7/10/2025 7:40 PM, Xiaoyao Li wrote:
>>> On 7/10/2025 11:55 AM, Zhenzhong Duan wrote:
>>>> Gpa is defined in QAPI but never reported to monitor because has_gpa is
>>>> never set to ture.
>>>>
>>>> Fix it by setting has_gpa to ture when TDX_REPORT_FATAL_ERROR_GPA_VALID
>>>> is set in error_code.
>>>
>>> Hi Zhenzhong,
>>>
>>> I would like to understand what the problem is without panic_info-
>>>> u.tdx.has_gpa being set?
>>
>> Never mind, I figure it out.
>>
>> qapi_event_send_guest_panicked() in qemu_system_guest_panicked will send the
>> event and data. Libvirt tries to parse the data, but find the .has_data
>> field is not set correctly.
> 
> Close, but not quite.
> 
> The "has_" fields are only present in the QEMU internal data structures,
> never on the wire in the JSON messages that libvirt receives.
> 
> The problem is with QEMU's struct -> JSON serialization - if the 'has_gpa'
> field is not set to 'true', then QEMU will *never* emit the 'gpa' field
> in the JSON it sends to the client app.
> 
> So what libvirt receives is this:
> 
>   {"timestamp": {"seconds": 1752156518, "microseconds": 809325},
>    "event": "GUEST_PANICKED",
>    "data": {"action": "pause", "info": {
>        "error-code": 0,
>        "message": "TD misconfiguration: SEPT #VE has to be disabled",
>        "type": "tdx"}}}
> 
> which has no 'gpa' present.

Thanks for the explanation! It's much clear to me now!

>> My original patch used (gpa == -1) as the indicator for whether gpa is
>> valid, this needs to be cleaned up. I will send the cleanup patch.
> 
> The value you assign to 'gpa' doesn't matter when 'has_gpa' is false,
> as it'll never get into the JSON event, so having it be '-1' is not
> significantly different from leaving it on 0.

I meant cleanup the QEMU internal logic in qemu_system_guest_panicked()

--- a/system/runstate.c
+++ b/system/runstate.c
@@ -690,7 +690,7 @@ void 
qemu_system_guest_panicked(GuestPanicInformation *info)
                            " error code: 0x%" PRIx32 " error 
message:\"%s\"\n",
                            info->u.tdx.error_code, message);
              g_free(message);
-            if (info->u.tdx.gpa != -1ull) {
+            if (info->u.tdx.has_gpa) {
                  qemu_log_mask(LOG_GUEST_ERROR, "Additional error 
information "
                                "can be found at gpa page: 0x%" PRIx64 "\n",
                                info->u.tdx.gpa);


> With regards,
> Daniel

Re: [PATCH] i386/tdx: Fix the report of gpa in QAPI

[Daniel P. Berrangé]

On Thu, Jul 10, 2025 at 10:21:12PM +0800, Xiaoyao Li wrote:
> On 7/10/2025 10:11 PM, Daniel P. Berrangé wrote:
> > On Thu, Jul 10, 2025 at 10:06:10PM +0800, Xiaoyao Li wrote:
> > > My original patch used (gpa == -1) as the indicator for whether gpa is
> > > valid, this needs to be cleaned up. I will send the cleanup patch.
> > 
> > The value you assign to 'gpa' doesn't matter when 'has_gpa' is false,
> > as it'll never get into the JSON event, so having it be '-1' is not
> > significantly different from leaving it on 0.
> 
> I meant cleanup the QEMU internal logic in qemu_system_guest_panicked()
> 
> --- a/system/runstate.c
> +++ b/system/runstate.c
> @@ -690,7 +690,7 @@ void qemu_system_guest_panicked(GuestPanicInformation
> *info)
>                            " error code: 0x%" PRIx32 " error
> message:\"%s\"\n",
>                            info->u.tdx.error_code, message);
>              g_free(message);
> -            if (info->u.tdx.gpa != -1ull) {
> +            if (info->u.tdx.has_gpa) {
>                  qemu_log_mask(LOG_GUEST_ERROR, "Additional error
> information "
>                                "can be found at gpa page: 0x%" PRIx64 "\n",
>                                info->u.tdx.gpa);

Yes, that would be better


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|