[PATCH v3 0/3] Rework FIXMEs and fprintfs in hcd-xhci.c

[PATCH v3 0/3] Rework FIXMEs and fprintfs in hcd-xhci.c

[Thomas Huth]

Replace the local FIXME macro and fprintf statement with
qemu_log_mask() and g_assert_not_reached().

Thomas Huth (3):
  hw/usb/hcd-xhci: Turn guest-triggerable abort() into qemu_log_mask()
  hw/usb/hcd-xhci: Remove the FIXME macro
  hw/usb/hcd-xhci: Use qemu_log_mask() instead of fprintf() statement

 hw/usb/hcd-xhci.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

-- 
2.55.0

[PATCH v3 1/3] hw/usb/hcd-xhci: Turn guest-triggerable abort() into qemu_log_mask()

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

The FIXME macros in xhci_alloc_device_streams() can be triggered
by a (malicious) guest. Since the macro also contains an abort()
statement, this terminates QEMU. Turn the FIXME statements into
a qemu_log_mask() instead to avoid that a guest can shoot itself
this way.

Reported-by: Feifan Qian <bea1e@proton.me>
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3784
Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/usb/hcd-xhci.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 2cdab3ba0e4..f5bb7d25600 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -965,11 +965,13 @@ static TRBCCode xhci_alloc_device_streams(XHCIState *xhci, unsigned int slotid,
          * together and make an usb_device_alloc_streams call per group.
          */
         if (epctxs[i]->nr_pstreams != req_nr_streams) {
-            FIXME("guest streams config not identical for all eps");
+            qemu_log_mask(LOG_UNIMP,
+                          "guest streams config not identical for all eps\n");
             return CC_RESOURCE_ERROR;
         }
         if (eps[i]->max_streams != dev_max_streams) {
-            FIXME("device streams config not identical for all eps");
+            qemu_log_mask(LOG_UNIMP,
+                          "device streams config not identical for all eps\n");
             return CC_RESOURCE_ERROR;
         }
     }
-- 
2.55.0

[PATCH v3 2/3] hw/usb/hcd-xhci: Remove the FIXME macro

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

The FIXME macro is only used in one case, which should hopefully
never trigger: The containing function handles all the USB_RET_*
values except for USB_RET_ADD_TO_QUEUE and USB_RET_REMOVE_FROM_QUEUE,
which are both internal return values for when an async packet needs
to be queued or dequeued, and which shouldn't still be the status by
the time we get to this function. Thus let's simplify this spot
and use a g_assert_not_reached() instead.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/usb/hcd-xhci.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index f5bb7d25600..ee913d5c4d5 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -39,8 +39,6 @@
 #else
 #define DPRINTF(...) do {} while (0)
 #endif
-#define FIXME(_msg) do { fprintf(stderr, "FIXME %s:%d %s\n", \
-                                 __func__, __LINE__, _msg); abort(); } while (0)
 
 #define TRB_LINK_LIMIT  32
 #define COMMAND_LIMIT   256
@@ -1675,7 +1673,7 @@ static int xhci_try_complete_packet(XHCITransfer *xfer)
     default:
         DPRINTF("%s: FIXME: status = %d\n", __func__,
                 xfer->packet.status);
-        FIXME("unhandled USB_RET_*");
+        g_assert_not_reached();
     }
     return 0;
 }
-- 
2.55.0

[PATCH v3 3/3] hw/usb/hcd-xhci: Use qemu_log_mask() instead of fprintf() statement

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

We've got a proper way for logging unimplemented hardware features,
so use qemu_log_mask() instead of the fprintf() here now.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/usb/hcd-xhci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index ee913d5c4d5..527910d92d2 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -1017,7 +1017,8 @@ static XHCIStreamContext *xhci_find_stream(XHCIEPContext *epctx,
         }
         sctx = epctx->pstreams + streamid;
     } else {
-        fprintf(stderr, "xhci: FIXME: secondary streams not implemented yet");
+        qemu_log_mask(LOG_UNIMP,
+                      "xhci: secondary streams not implemented yet");
         *cc_error = CC_INVALID_STREAM_TYPE_ERROR;
         return NULL;
     }
-- 
2.55.0

Re: [PATCH v3 1/3] hw/usb/hcd-xhci: Turn guest-triggerable abort() into qemu_log_mask()

[Peter Maydell]

On Mon, 13 Jul 2026 at 11:05, Thomas Huth <thuth@redhat.com> wrote:
>
> From: Thomas Huth <thuth@redhat.com>
>
> The FIXME macros in xhci_alloc_device_streams() can be triggered
> by a (malicious) guest. Since the macro also contains an abort()
> statement, this terminates QEMU. Turn the FIXME statements into
> a qemu_log_mask() instead to avoid that a guest can shoot itself
> this way.
>
> Reported-by: Feifan Qian <bea1e@proton.me>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3784
> Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/usb/hcd-xhci.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 2cdab3ba0e4..f5bb7d25600 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -965,11 +965,13 @@ static TRBCCode xhci_alloc_device_streams(XHCIState *xhci, unsigned int slotid,
>           * together and make an usb_device_alloc_streams call per group.
>           */
>          if (epctxs[i]->nr_pstreams != req_nr_streams) {
> -            FIXME("guest streams config not identical for all eps");
> +            qemu_log_mask(LOG_UNIMP,
> +                          "guest streams config not identical for all eps\n");
>              return CC_RESOURCE_ERROR;
>          }
>          if (eps[i]->max_streams != dev_max_streams) {
> -            FIXME("device streams config not identical for all eps");
> +            qemu_log_mask(LOG_UNIMP,
> +                          "device streams config not identical for all eps\n");
>              return CC_RESOURCE_ERROR;
>          }
>      }


Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v3 2/3] hw/usb/hcd-xhci: Remove the FIXME macro

[Peter Maydell]

On Mon, 13 Jul 2026 at 11:05, Thomas Huth <thuth@redhat.com> wrote:
>
> From: Thomas Huth <thuth@redhat.com>
>
> The FIXME macro is only used in one case, which should hopefully
> never trigger: The containing function handles all the USB_RET_*
> values except for USB_RET_ADD_TO_QUEUE and USB_RET_REMOVE_FROM_QUEUE,
> which are both internal return values for when an async packet needs
> to be queued or dequeued, and which shouldn't still be the status by
> the time we get to this function. Thus let's simplify this spot
> and use a g_assert_not_reached() instead.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/usb/hcd-xhci.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Not sure if some compilers/analyzers would complain about the
DPRINTF followed by g_assert_not_reached().

thanks
-- PMM

Re: [PATCH v3 3/3] hw/usb/hcd-xhci: Use qemu_log_mask() instead of fprintf() statement

[Peter Maydell]

On Mon, 13 Jul 2026 at 11:05, Thomas Huth <thuth@redhat.com> wrote:
>
> From: Thomas Huth <thuth@redhat.com>
>
> We've got a proper way for logging unimplemented hardware features,
> so use qemu_log_mask() instead of the fprintf() here now.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/usb/hcd-xhci.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index ee913d5c4d5..527910d92d2 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -1017,7 +1017,8 @@ static XHCIStreamContext *xhci_find_stream(XHCIEPContext *epctx,
>          }
>          sctx = epctx->pstreams + streamid;
>      } else {
> -        fprintf(stderr, "xhci: FIXME: secondary streams not implemented yet");
> +        qemu_log_mask(LOG_UNIMP,
> +                      "xhci: secondary streams not implemented yet");
>          *cc_error = CC_INVALID_STREAM_TYPE_ERROR;
>          return NULL;
>      }

qemu_log_mask() strings need a trailing "\n" (as of course does
printing messages to stderr :-)).

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v3 3/3] hw/usb/hcd-xhci: Use qemu_log_mask() instead of fprintf() statement

[Thomas Huth]

On 13/07/2026 12.17, Peter Maydell wrote:
> On Mon, 13 Jul 2026 at 11:05, Thomas Huth <thuth@redhat.com> wrote:
>>
>> From: Thomas Huth <thuth@redhat.com>
>>
>> We've got a proper way for logging unimplemented hardware features,
>> so use qemu_log_mask() instead of the fprintf() here now.
>>
>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   hw/usb/hcd-xhci.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
>> index ee913d5c4d5..527910d92d2 100644
>> --- a/hw/usb/hcd-xhci.c
>> +++ b/hw/usb/hcd-xhci.c
>> @@ -1017,7 +1017,8 @@ static XHCIStreamContext *xhci_find_stream(XHCIEPContext *epctx,
>>           }
>>           sctx = epctx->pstreams + streamid;
>>       } else {
>> -        fprintf(stderr, "xhci: FIXME: secondary streams not implemented yet");
>> +        qemu_log_mask(LOG_UNIMP,
>> +                      "xhci: secondary streams not implemented yet");
>>           *cc_error = CC_INVALID_STREAM_TYPE_ERROR;
>>           return NULL;
>>       }
> 
> qemu_log_mask() strings need a trailing "\n" (as of course does
> printing messages to stderr :-)).

D'oh, I forgot it again! Thanks for spotting it! I'll send an updated version...

> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Thanks!

  Thomas

Re: [PATCH v3 2/3] hw/usb/hcd-xhci: Remove the FIXME macro

[Thomas Huth]

On 13/07/2026 12.16, Peter Maydell wrote:
> On Mon, 13 Jul 2026 at 11:05, Thomas Huth <thuth@redhat.com> wrote:
>>
>> From: Thomas Huth <thuth@redhat.com>
>>
>> The FIXME macro is only used in one case, which should hopefully
>> never trigger: The containing function handles all the USB_RET_*
>> values except for USB_RET_ADD_TO_QUEUE and USB_RET_REMOVE_FROM_QUEUE,
>> which are both internal return values for when an async packet needs
>> to be queued or dequeued, and which shouldn't still be the status by
>> the time we get to this function. Thus let's simplify this spot
>> and use a g_assert_not_reached() instead.
>>
>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   hw/usb/hcd-xhci.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> Not sure if some compilers/analyzers would complain about the
> DPRINTF followed by g_assert_not_reached().
I hope they don't complain since the macro is disabled by default...? Or 
shall we play save and remove the DPRINTF here, too?

  Thomas

Re: [PATCH v3 2/3] hw/usb/hcd-xhci: Remove the FIXME macro

[Philippe Mathieu-Daudé]

On 13/7/26 12:32, Thomas Huth wrote:
> On 13/07/2026 12.16, Peter Maydell wrote:
>> On Mon, 13 Jul 2026 at 11:05, Thomas Huth <thuth@redhat.com> wrote:
>>>
>>> From: Thomas Huth <thuth@redhat.com>
>>>
>>> The FIXME macro is only used in one case, which should hopefully
>>> never trigger: The containing function handles all the USB_RET_*
>>> values except for USB_RET_ADD_TO_QUEUE and USB_RET_REMOVE_FROM_QUEUE,
>>> which are both internal return values for when an async packet needs
>>> to be queued or dequeued, and which shouldn't still be the status by
>>> the time we get to this function. Thus let's simplify this spot
>>> and use a g_assert_not_reached() instead.
>>>
>>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   hw/usb/hcd-xhci.c | 4 +---
>>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> Not sure if some compilers/analyzers would complain about the
>> DPRINTF followed by g_assert_not_reached().
> I hope they don't complain since the macro is disabled by default...? Or 
> shall we play save and remove the DPRINTF here, too?

I'd remove this pointless DPRINTF().

Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>