Re: [PATCH 01/10] qapi: expose rtc-reset-reinjection command unconditionally

["Daniel P. Berrangé" <berrange@redhat.com>]

On Sat, May 10, 2025 at 11:57:10AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > This removes the TARGET_I386 condition from the rtc-reset-reinjection
> > command. This requires providing a QMP command stub for non-i386 target.
> > This in turn requires moving the command out of misc-target.json, since
> > that will trigger symbol poisoning errors when built from target
> > independent code.
> >
> > Rather than putting the command into misc.json, it is proposed to create
> > misc-$TARGET.json files to hold commands whose impl is conceptually
> > only applicable to a single target. This gives an obvious docs hint to
> > consumers that the command is only useful in relation a specific target,
> > while misc.json is for commands applicable to 2 or more targets.
> 
> Starting with this patch, the series structures the manual like this:
> 
>     = Machines
>     ... contents of machine.json ...
>     == Specific to S390
>     ... contents of machine-s390.json ...
> 
> and
> 
>     = Miscellanea
>     ... contents of misc.json ...
>     == Specific to ARM
>     ... contents of misc-arm.json ...
>     == Specific to i386
>     ... contents of misc-i386.json ...
> 
> Except it doesn't add == subsection headers, but that's detail.  The
> text I show for them here is crap.
> 
> Possible alternative: collect the target-specific stuff in one place
> rather than two:
> 
>     = Targets
>     == ARM
>     == i386
>     == S390
> 
> Again the header text is crap.
> 
> Is separating the current contents of misc-<target>.json from
> machine-<target>.json useful?

I'm fairly confused what the intended difference is between
stuff in 'misc.json' and stuff in 'machine.json' already,
and just preserved that split rather than try to think about
it in more detail.


> > diff --git a/qapi/misc-i386.json b/qapi/misc-i386.json
> > new file mode 100644
> > index 0000000000..d5bfd91405
> > --- /dev/null
> > +++ b/qapi/misc-i386.json
> > @@ -0,0 +1,24 @@
> > +# -*- Mode: Python -*-
> > +# vim: filetype=python
> > +#
> > +# SPDX-License-Identifier: GPL-2.0-or-later
> 
> Might be cleaner to add this to all qapi/*.json first, and in a separate
> patch.

Adding SPDX-License-Identifier to existing files is something we're
not generally doing, only newly created files are getting it.


> > +
> > +##
> > +# @rtc-reset-reinjection:
> > +#
> > +# This command will reset the RTC interrupt reinjection backlog.  Can
> > +# be used if another mechanism to synchronize guest time is in effect,
> > +# for example QEMU guest agent's guest-set-time command.
> > +#
> > +# Use of this command is only applicable for x86 machines with an RTC,
> > +# and on other machines will silently return without performing any
> > +# action.
> 
> This paragraph replaces ...
> 
> > +#
> > +# Since: 2.1
> > +#
> > +# .. qmp-example::
> > +#
> > +#     -> { "execute": "rtc-reset-reinjection" }
> > +#     <- { "return": {} }
> > +##
> > +{ 'command': 'rtc-reset-reinjection' }
> > diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> > index 42e4a7417d..5d0ffb0164 100644
> > --- a/qapi/misc-target.json
> > +++ b/qapi/misc-target.json
> > @@ -2,23 +2,6 @@
> >  # vim: filetype=python
> >  #
> >  
> > -##
> > -# @rtc-reset-reinjection:
> > -#
> > -# This command will reset the RTC interrupt reinjection backlog.  Can
> > -# be used if another mechanism to synchronize guest time is in effect,
> > -# for example QEMU guest agent's guest-set-time command.
> > -#
> > -# Since: 2.1
> > -#
> > -# .. qmp-example::
> > -#
> > -#     -> { "execute": "rtc-reset-reinjection" }
> > -#     <- { "return": {} }
> > -##
> > -{ 'command': 'rtc-reset-reinjection',
> > -  'if': 'TARGET_I386' }
> 
> ... the conditional.
> 
> Before, attempting to execute the command fails with CommandNotFound.
> 
> Afterwards it succeeds without doing anything.  I think it should fail
> instead.  CommandNotFound would be a lie, so change it to GenericError.

See also my comment in the commit message that this has different
behaviour on x86 vs non-x86, when no RTC is present - x86 treats
"no RTC" as a no-op, but non-x86 treats it as an error.

I don't mind if we preserve this inconsistency though.


> > diff --git a/stubs/monitor-i386-rtc.c b/stubs/monitor-i386-rtc.c
> > new file mode 100644
> > index 0000000000..ee2e60d95b
> > --- /dev/null
> > +++ b/stubs/monitor-i386-rtc.c
> > @@ -0,0 +1,10 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qapi/error.h"
> > +#include "qapi/qapi-commands-misc-i386.h"
> > +
> > +void qmp_rtc_reset_reinjection(Error **errp)
> > +{
> > +    /* Nothing to do since non-x86 machines lack an RTC */
> > +}
> 
> I think I'd create one stub file per qapi/<foo>-<target>.json.

That's what I started doing but I forgot that doesn't work out well
with the linker.

The linker's granularity for dropping stubs is per-file, not per-symbol.

If /any/ method in a stub/nnn.c file is needed, the linker will pull in
all methods. This results in duplicate symbol errors if only a subset
of stub methods were actually needed.

This forces us to have a separate stub file per build configuration
scenario that can affect use of stubs.

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