[PATCH] xen/passthrough: add missing error-report include

[PATCH] xen/passthrough: add missing error-report include

[Adam Williamson]

In cfcacba an `error_report` was added to this file, but the
corresponding include of `qemu/error-report.h` was missed. This
only becomes apparent when building against Xen 4.20+.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
---
 hw/xen/xen_pt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 9d16644d82..006b5b55f2 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -54,6 +54,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qemu/error-report.h"
 #include <sys/ioctl.h>
 
 #include "hw/pci/pci.h"
-- 
2.50.1

Re: [PATCH] xen/passthrough: add missing error-report include

[Markus Armbruster]

Adam Williamson <awilliam@redhat.com> writes:

> In cfcacba an `error_report` was added to this file, but the
> corresponding include of `qemu/error-report.h` was missed. This
> only becomes apparent when building against Xen 4.20+.
>
> Signed-off-by: Adam Williamson <awilliam@redhat.com>
> ---
>  hw/xen/xen_pt.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index 9d16644d82..006b5b55f2 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -54,6 +54,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> +#include "qemu/error-report.h"
>  #include <sys/ioctl.h>
>  
>  #include "hw/pci/pci.h"

Uh, error-report.h is included without this for me.  To see, build with
-H:

. /work/armbru/qemu/hw/xen/xen_pt.h
.. /work/armbru/qemu/include/hw/xen/xen_native.h
... /work/armbru/qemu/hw/xen/trace.h
.... ./trace/trace-hw_xen.h
..... /work/armbru/qemu/include/qemu/error-report.h

Code smell: header in include/... includes header from hw/.  Not this
patch's concern.

Re: [PATCH] xen/passthrough: add missing error-report include

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Adam Williamson <awilliam@redhat.com> writes:
>
>> In cfcacba an `error_report` was added to this file, but the
>> corresponding include of `qemu/error-report.h` was missed. This
>> only becomes apparent when building against Xen 4.20+.
>>
>> Signed-off-by: Adam Williamson <awilliam@redhat.com>
>> ---
>>  hw/xen/xen_pt.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
>> index 9d16644d82..006b5b55f2 100644
>> --- a/hw/xen/xen_pt.c
>> +++ b/hw/xen/xen_pt.c
>> @@ -54,6 +54,7 @@
>>  
>>  #include "qemu/osdep.h"
>>  #include "qapi/error.h"
>> +#include "qemu/error-report.h"
>>  #include <sys/ioctl.h>
>>  
>>  #include "hw/pci/pci.h"
>
> Uh, error-report.h is included without this for me.  To see, build with
> -H:
>
> . /work/armbru/qemu/hw/xen/xen_pt.h
> .. /work/armbru/qemu/include/hw/xen/xen_native.h
> ... /work/armbru/qemu/hw/xen/trace.h
> .... ./trace/trace-hw_xen.h
> ..... /work/armbru/qemu/include/qemu/error-report.h

Just remembered: the generated trace header includes error-report.h only
when trace's log backend is enabled.

Suggested commit message:

  In commit cfcacba an `error_report` was added to this file, but the
  corresponding include of `qemu/error-report.h` was missed. This
  only becomes apparent when building against Xen 4.20+ with trace
  backend log disabled.

  Fixes: cfcacbab38e4 (xen/passthrough: use gsi to map pirq when dom0 is PVH)

With something like that
Reviewed-by: Markus Armbruster <armbru@redhat.com>

> Code smell: header in include/... includes header from hw/.  Not this
> patch's concern.

Re: [PATCH] xen/passthrough: add missing error-report include

[Philippe Mathieu-Daudé]

On 18/7/25 07:11, Markus Armbruster wrote:
> Adam Williamson <awilliam@redhat.com> writes:
> 
>> In cfcacba an `error_report` was added to this file, but the

    In commit cfcacbab38e ("xen/passthrough: use gsi to map pirq when
    dom0 is PVH") an `error_report` was added to this file, but the

>> corresponding include of `qemu/error-report.h` was missed. This
>> only becomes apparent when building against Xen 4.20+.
>>
>> Signed-off-by: Adam Williamson <awilliam@redhat.com>
>> ---
>>   hw/xen/xen_pt.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
>> index 9d16644d82..006b5b55f2 100644
>> --- a/hw/xen/xen_pt.c
>> +++ b/hw/xen/xen_pt.c
>> @@ -54,6 +54,7 @@
>>   
>>   #include "qemu/osdep.h"
>>   #include "qapi/error.h"
>> +#include "qemu/error-report.h"
>>   #include <sys/ioctl.h>
>>   
>>   #include "hw/pci/pci.h"
> 
> Uh, error-report.h is included without this for me.  To see, build with
> -H:
> 
> . /work/armbru/qemu/hw/xen/xen_pt.h
> .. /work/armbru/qemu/include/hw/xen/xen_native.h
> ... /work/armbru/qemu/hw/xen/trace.h
> .... ./trace/trace-hw_xen.h
> ..... /work/armbru/qemu/include/qemu/error-report.h
> 
> Code smell: header in include/... includes header from hw/.  Not this
> patch's concern.

Lucky side effect of including "trace.h" a include/ file due to trace
event being called in inlined function. Bad pattern indeed.

Back to this patch:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH] xen/passthrough: add missing error-report include

[Philippe Mathieu-Daudé]

On 18/7/25 08:05, Philippe Mathieu-Daudé wrote:
> On 18/7/25 07:11, Markus Armbruster wrote:
>> Adam Williamson <awilliam@redhat.com> writes:
>>
>>> In cfcacba an `error_report` was added to this file, but the
> 
>     In commit cfcacbab38e ("xen/passthrough: use gsi to map pirq when
>     dom0 is PVH") an `error_report` was added to this file, but the
> 
>>> corresponding include of `qemu/error-report.h` was missed. This
>>> only becomes apparent when building against Xen 4.20+.
>>>
>>> Signed-off-by: Adam Williamson <awilliam@redhat.com>
>>> ---
>>>   hw/xen/xen_pt.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
>>> index 9d16644d82..006b5b55f2 100644
>>> --- a/hw/xen/xen_pt.c
>>> +++ b/hw/xen/xen_pt.c
>>> @@ -54,6 +54,7 @@
>>>   #include "qemu/osdep.h"
>>>   #include "qapi/error.h"
>>> +#include "qemu/error-report.h"
>>>   #include <sys/ioctl.h>
>>>   #include "hw/pci/pci.h"
>>
>> Uh, error-report.h is included without this for me.  To see, build with
>> -H:
>>
>> . /work/armbru/qemu/hw/xen/xen_pt.h
>> .. /work/armbru/qemu/include/hw/xen/xen_native.h
>> ... /work/armbru/qemu/hw/xen/trace.h
>> .... ./trace/trace-hw_xen.h
>> ..... /work/armbru/qemu/include/qemu/error-report.h

FYI "qemu/error-report.h" was added in commit 418ed14268f ("trace:
make the 'log' backend timestamp configurable") to access the
message_with_timestamp declaration.

>>
>> Code smell: header in include/... includes header from hw/.  Not this
>> patch's concern.
> 
> Lucky side effect of including "trace.h" a include/ file due to trace
> event being called in inlined function. Bad pattern indeed.
> 
> Back to this patch:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> 

Re: [PATCH] xen/passthrough: add missing error-report include

[Daniel P. Berrangé]

On Fri, Jul 18, 2025 at 07:59:50AM +0200, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Adam Williamson <awilliam@redhat.com> writes:
> >
> >> In cfcacba an `error_report` was added to this file, but the
> >> corresponding include of `qemu/error-report.h` was missed. This
> >> only becomes apparent when building against Xen 4.20+.
> >>
> >> Signed-off-by: Adam Williamson <awilliam@redhat.com>
> >> ---
> >>  hw/xen/xen_pt.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> >> index 9d16644d82..006b5b55f2 100644
> >> --- a/hw/xen/xen_pt.c
> >> +++ b/hw/xen/xen_pt.c
> >> @@ -54,6 +54,7 @@
> >>  
> >>  #include "qemu/osdep.h"
> >>  #include "qapi/error.h"
> >> +#include "qemu/error-report.h"
> >>  #include <sys/ioctl.h>
> >>  
> >>  #include "hw/pci/pci.h"
> >
> > Uh, error-report.h is included without this for me.  To see, build with
> > -H:
> >
> > . /work/armbru/qemu/hw/xen/xen_pt.h
> > .. /work/armbru/qemu/include/hw/xen/xen_native.h
> > ... /work/armbru/qemu/hw/xen/trace.h
> > .... ./trace/trace-hw_xen.h
> > ..... /work/armbru/qemu/include/qemu/error-report.h
> 
> Just remembered: the generated trace header includes error-report.h only
> when trace's log backend is enabled.

Hmm, that's rather an unfortunate trap-door :-( Given that 'log' is enabled
by default when building from git we'll never see missing error-report.h
problems in daily work.

Looking at the log backend it appears that originally it would
unconditionally include the timestamp when calling qemu_log, but
then changed that to opt-in with

  commit 418ed14268f797a5142b60cd557cd598eb548c66
  Author: Stefan Hajnoczi <stefanha@redhat.com>
  Date:   Mon Jan 25 11:35:07 2021 +0000

    trace: make the 'log' backend timestamp configurable

requiring -msg timestamp=on, which was a pre-existing flag that already
did a similar toggle for the 'error_report' function. The goal makes
sense, but it introduced the error-report.h trap door

When I see that I also question why the 'log' backend should be a
special case user of qemu_log() ?  Why shouldn't we emit timestamps
for all usage of qemu_log() ?

If we changed the qemu_log impl to honour the timestamp toggle, then
all users of qemu_log benefit. We then eliminate error-report.h usage
in trace.h headers, and also cut the code size for trace points
significantly


static inline void _nocheck__trace_object_dynamic_cast_assert(const char * type, const char * target, const char * file, int line, const char * func)
{
    if (trace_event_get_state(TRACE_OBJECT_DYNAMIC_CAST_ASSERT) && qemu_loglevel_mask(LOG_TRACE)) {
        if (message_with_timestamp) {
            struct timeval _now;
            gettimeofday(&_now, NULL);
            qemu_log("%d@%zu.%06zu:object_dynamic_cast_assert " "%s->%s (%s:%d:%s)" "\n",
                     qemu_get_thread_id(),
                     (size_t)_now.tv_sec, (size_t)_now.tv_usec
                     , type, target, file, line, func);
        } else {
            qemu_log("object_dynamic_cast_assert " "%s->%s (%s:%d:%s)" "\n", type, target, file, line, func);
        }
    }
}

down to


static inline void _nocheck__trace_object_dynamic_cast_assert(const char * type, const char * target, const char * file, int line, const char * func)
{
    if (trace_event_get_state(TRACE_OBJECT_DYNAMIC_CAST_ASSERT) && qemu_loglevel_mask(LOG_TRACE)) {
            qemu_log("object_dynamic_cast_assert " "%s->%s (%s:%d:%s)" "\n", type, target, file, line, func);
    }
}

which feels more in keeping with the kind of level of complexity you should
want to be inlined in trace callers.

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] xen/passthrough: add missing error-report include

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Jul 18, 2025 at 07:59:50AM +0200, Markus Armbruster wrote:
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>> > Adam Williamson <awilliam@redhat.com> writes:
>> >
>> >> In cfcacba an `error_report` was added to this file, but the
>> >> corresponding include of `qemu/error-report.h` was missed. This
>> >> only becomes apparent when building against Xen 4.20+.
>> >>
>> >> Signed-off-by: Adam Williamson <awilliam@redhat.com>
>> >> ---
>> >>  hw/xen/xen_pt.c | 1 +
>> >>  1 file changed, 1 insertion(+)
>> >>
>> >> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
>> >> index 9d16644d82..006b5b55f2 100644
>> >> --- a/hw/xen/xen_pt.c
>> >> +++ b/hw/xen/xen_pt.c
>> >> @@ -54,6 +54,7 @@
>> >>  
>> >>  #include "qemu/osdep.h"
>> >>  #include "qapi/error.h"
>> >> +#include "qemu/error-report.h"
>> >>  #include <sys/ioctl.h>
>> >>  
>> >>  #include "hw/pci/pci.h"
>> >
>> > Uh, error-report.h is included without this for me.  To see, build with
>> > -H:
>> >
>> > . /work/armbru/qemu/hw/xen/xen_pt.h
>> > .. /work/armbru/qemu/include/hw/xen/xen_native.h
>> > ... /work/armbru/qemu/hw/xen/trace.h
>> > .... ./trace/trace-hw_xen.h
>> > ..... /work/armbru/qemu/include/qemu/error-report.h
>> 
>> Just remembered: the generated trace header includes error-report.h only
>> when trace's log backend is enabled.
>
> Hmm, that's rather an unfortunate trap-door :-( Given that 'log' is enabled
> by default when building from git we'll never see missing error-report.h
> problems in daily work.

Correct.

> Looking at the log backend it appears that originally it would
> unconditionally include the timestamp when calling qemu_log, but
> then changed that to opt-in with
>
>   commit 418ed14268f797a5142b60cd557cd598eb548c66
>   Author: Stefan Hajnoczi <stefanha@redhat.com>
>   Date:   Mon Jan 25 11:35:07 2021 +0000
>
>     trace: make the 'log' backend timestamp configurable
>
> requiring -msg timestamp=on, which was a pre-existing flag that already
> did a similar toggle for the 'error_report' function. The goal makes
> sense, but it introduced the error-report.h trap door
>
> When I see that I also question why the 'log' backend should be a
> special case user of qemu_log() ?  Why shouldn't we emit timestamps
> for all usage of qemu_log() ?
>
> If we changed the qemu_log impl to honour the timestamp toggle, then
> all users of qemu_log benefit. We then eliminate error-report.h usage
> in trace.h headers, and also cut the code size for trace points
> significantly
>
>
> static inline void _nocheck__trace_object_dynamic_cast_assert(const char * type, const char * target, const char * file, int line, const char * func)
> {
>     if (trace_event_get_state(TRACE_OBJECT_DYNAMIC_CAST_ASSERT) && qemu_loglevel_mask(LOG_TRACE)) {
>         if (message_with_timestamp) {
>             struct timeval _now;
>             gettimeofday(&_now, NULL);
>             qemu_log("%d@%zu.%06zu:object_dynamic_cast_assert " "%s->%s (%s:%d:%s)" "\n",
>                      qemu_get_thread_id(),
>                      (size_t)_now.tv_sec, (size_t)_now.tv_usec
>                      , type, target, file, line, func);
>         } else {
>             qemu_log("object_dynamic_cast_assert " "%s->%s (%s:%d:%s)" "\n", type, target, file, line, func);
>         }
>     }
> }
>
> down to
>
>
> static inline void _nocheck__trace_object_dynamic_cast_assert(const char * type, const char * target, const char * file, int line, const char * func)
> {
>     if (trace_event_get_state(TRACE_OBJECT_DYNAMIC_CAST_ASSERT) && qemu_loglevel_mask(LOG_TRACE)) {
>             qemu_log("object_dynamic_cast_assert " "%s->%s (%s:%d:%s)" "\n", type, target, file, line, func);
>     }
> }
>
> which feels more in keeping with the kind of level of complexity you should
> want to be inlined in trace callers.

Oh yes.  We should do this even if we find a reason for keeping
qemu_log() as it is.  The obvious way would be a new function
qemu_log_with_timestamp().

Re: [PATCH] xen/passthrough: add missing error-report include

[Daniel P. Berrangé]

On Fri, Jul 18, 2025 at 03:20:35PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Fri, Jul 18, 2025 at 07:59:50AM +0200, Markus Armbruster wrote:
> >> Markus Armbruster <armbru@redhat.com> writes:
> >> 
> >> > Adam Williamson <awilliam@redhat.com> writes:
> >> >
> >> >> In cfcacba an `error_report` was added to this file, but the
> >> >> corresponding include of `qemu/error-report.h` was missed. This
> >> >> only becomes apparent when building against Xen 4.20+.
> >> >>
> >> >> Signed-off-by: Adam Williamson <awilliam@redhat.com>
> >> >> ---
> >> >>  hw/xen/xen_pt.c | 1 +
> >> >>  1 file changed, 1 insertion(+)
> >> >>
> >> >> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> >> >> index 9d16644d82..006b5b55f2 100644
> >> >> --- a/hw/xen/xen_pt.c
> >> >> +++ b/hw/xen/xen_pt.c
> >> >> @@ -54,6 +54,7 @@
> >> >>  
> >> >>  #include "qemu/osdep.h"
> >> >>  #include "qapi/error.h"
> >> >> +#include "qemu/error-report.h"
> >> >>  #include <sys/ioctl.h>
> >> >>  
> >> >>  #include "hw/pci/pci.h"
> >> >
> >> > Uh, error-report.h is included without this for me.  To see, build with
> >> > -H:
> >> >
> >> > . /work/armbru/qemu/hw/xen/xen_pt.h
> >> > .. /work/armbru/qemu/include/hw/xen/xen_native.h
> >> > ... /work/armbru/qemu/hw/xen/trace.h
> >> > .... ./trace/trace-hw_xen.h
> >> > ..... /work/armbru/qemu/include/qemu/error-report.h
> >> 
> >> Just remembered: the generated trace header includes error-report.h only
> >> when trace's log backend is enabled.
> >
> > Hmm, that's rather an unfortunate trap-door :-( Given that 'log' is enabled
> > by default when building from git we'll never see missing error-report.h
> > problems in daily work.
> 
> Correct.
> 
> > Looking at the log backend it appears that originally it would
> > unconditionally include the timestamp when calling qemu_log, but
> > then changed that to opt-in with
> >
> >   commit 418ed14268f797a5142b60cd557cd598eb548c66
> >   Author: Stefan Hajnoczi <stefanha@redhat.com>
> >   Date:   Mon Jan 25 11:35:07 2021 +0000
> >
> >     trace: make the 'log' backend timestamp configurable
> >
> > requiring -msg timestamp=on, which was a pre-existing flag that already
> > did a similar toggle for the 'error_report' function. The goal makes
> > sense, but it introduced the error-report.h trap door
> >
> > When I see that I also question why the 'log' backend should be a
> > special case user of qemu_log() ?  Why shouldn't we emit timestamps
> > for all usage of qemu_log() ?
> >
> > If we changed the qemu_log impl to honour the timestamp toggle, then
> > all users of qemu_log benefit. We then eliminate error-report.h usage
> > in trace.h headers, and also cut the code size for trace points
> > significantly
> >
> >
> > static inline void _nocheck__trace_object_dynamic_cast_assert(const char * type, const char * target, const char * file, int line, const char * func)
> > {
> >     if (trace_event_get_state(TRACE_OBJECT_DYNAMIC_CAST_ASSERT) && qemu_loglevel_mask(LOG_TRACE)) {
> >         if (message_with_timestamp) {
> >             struct timeval _now;
> >             gettimeofday(&_now, NULL);
> >             qemu_log("%d@%zu.%06zu:object_dynamic_cast_assert " "%s->%s (%s:%d:%s)" "\n",
> >                      qemu_get_thread_id(),
> >                      (size_t)_now.tv_sec, (size_t)_now.tv_usec
> >                      , type, target, file, line, func);
> >         } else {
> >             qemu_log("object_dynamic_cast_assert " "%s->%s (%s:%d:%s)" "\n", type, target, file, line, func);
> >         }
> >     }
> > }
> >
> > down to
> >
> >
> > static inline void _nocheck__trace_object_dynamic_cast_assert(const char * type, const char * target, const char * file, int line, const char * func)
> > {
> >     if (trace_event_get_state(TRACE_OBJECT_DYNAMIC_CAST_ASSERT) && qemu_loglevel_mask(LOG_TRACE)) {
> >             qemu_log("object_dynamic_cast_assert " "%s->%s (%s:%d:%s)" "\n", type, target, file, line, func);
> >     }
> > }
> >
> > which feels more in keeping with the kind of level of complexity you should
> > want to be inlined in trace callers.
> 
> Oh yes.  We should do this even if we find a reason for keeping
> qemu_log() as it is.  The obvious way would be a new function
> qemu_log_with_timestamp().

Now proposed at:

  https://lists.nongnu.org/archive/html/qemu-devel/2025-07/msg05234.html

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] xen/passthrough: add missing error-report include

[Bernhard Beschow]

Am 17. Juli 2025 22:02:07 UTC schrieb Adam Williamson <awilliam@redhat.com>:
>In cfcacba an `error_report` was added to this file, but the
>corresponding include of `qemu/error-report.h` was missed. This
>only becomes apparent when building against Xen 4.20+.
>
>Signed-off-by: Adam Williamson <awilliam@redhat.com>
>---
> hw/xen/xen_pt.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
>index 9d16644d82..006b5b55f2 100644
>--- a/hw/xen/xen_pt.c
>+++ b/hw/xen/xen_pt.c
>@@ -54,6 +54,7 @@
> 
> #include "qemu/osdep.h"
> #include "qapi/error.h"
>+#include "qemu/error-report.h"
> #include <sys/ioctl.h>
> 
> #include "hw/pci/pci.h"

Reviewed-by: Bernhard Beschow <shentey@gmail.com>

Re: [PATCH] xen/passthrough: add missing error-report include

[Philippe Mathieu-Daudé]

On 18/7/25 00:02, Adam Williamson wrote:
> In cfcacba an `error_report` was added to this file, but the
> corresponding include of `qemu/error-report.h` was missed. This
> only becomes apparent when building against Xen 4.20+.
> 
> Signed-off-by: Adam Williamson <awilliam@redhat.com>
> ---
>   hw/xen/xen_pt.c | 1 +
>   1 file changed, 1 insertion(+)

Patch queued, thanks!