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

[Markus Armbruster <armbru@redhat.com>]

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

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

Fair enough, we can always rearrange things on top.

>> > The current impl of qmp_rtc_reset_reinject() is a no-op if the i386
>> > RTC is disabled in Kconfig, or if the running machine type lack any
>> > RTC device. Thus the stub impl for non-i386 targets retains this
>> > no-op behaviour, instead of reporting a Error which is the more usual
>> > choice for commands invoked against unsupported configurations.
>> >
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

[...]

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

I see.

Code motion between files where the license isn't obviously the same
gives me pause.

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

I see.

The command is a mess.  It goes back to commit f2ae8abf1fa (mc146818rtc:
add rtc-reset-reinjection QMP command).  Here's the commit message's
rationale:

    It is necessary to reset RTC interrupt reinjection backlog if
    guest time is synchronized via a different mechanism, such as
    QGA's guest-set-time command.
    
    Failing to do so causes both corrections to be applied (summed),
    resulting in an incorrect guest time.

Plausible enough, but why is the solution limited to target i386
machines with an MC146818 RTC?  I think I found the answer in recent
commit d0be0ac2c37 (hw/i386: move rtc-reset-reinjection command out of
hw/rtc):

    The rtc-reset-reinjection QMP command is specific to x86, other boards do not
    have the ACK tracking functionality that is needed for RTC interrupt
    reinjection.  Therefore the QMP command is only included in x86, but
    qmp_rtc_reset_reinjection() is implemented by hw/rtc/mc146818rtc.c
    and requires tracking of all created RTC devices.  Move the implementation
    to hw/i386, so that 1) it is available even if no RTC device exist
    2) the only RTC that exists is easily found in x86ms->rtc.

So, the time sync problem is serious enough for us to provide a
solution, but it's only serious enough for i386 machines that use the
RTC most common there.  This makes no sense to me.

Code:

    void qmp_rtc_reset_reinjection(Error **errp)
    {
        X86MachineState *x86ms = X86_MACHINE(qdev_get_machine());

    #ifdef CONFIG_MC146818RTC
        if (x86ms->rtc) {
            rtc_reset_reinjection(MC146818_RTC(x86ms->rtc));
        }
    #else
        assert(!x86ms->rtc);
    #endif
    }

Behavior:

* target other than i386: CommandNotFound

* else if the machine's ->rtc is null: silently do nothing

* else if CONFIG_MC146818RTC: works as advertized as long ->rtc is an
  MC146818, else crash

* else: crash

WTF!?!

>> > 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 */

The comment is wrong: pretty much all machines do have a real time
clock.

>> > +}
>> 
>> 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.

You're right.