| Message ID | 20250515172732.3992504-2-pierrick.bouvier@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | qapi: remove all TARGET_* conditionals from the schema | expand |
Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: > From: Daniel P. Berrangé <berrange@redhat.com> > > 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. > > 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. > > The stub impl for non-i386 targets retains this no-op behaviour. > However, it is now reporting an Error mentioning this command is not > available for current target. > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> [...] > diff --git a/stubs/monitor-i386-rtc.c b/stubs/monitor-i386-rtc.c > new file mode 100644 > index 00000000000..d810f33efec > --- /dev/null > +++ b/stubs/monitor-i386-rtc.c > @@ -0,0 +1,11 @@ > +/* 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) > +{ > + error_setg(errp, > + "rtc-reset-injection is only available for x86 machines with RTC"); > +} We get this stub exactly when the command did not exist before the patch. Thus, the command fails before and after, only error code and message change, which is fine. However, the error message feels brittle: it becomes misleading when we implement the command for other targets. Let's dumb it down to something like "RTC interrupt reinjection backlog reset is not available for this machine". [...]
On 5/17/25 1:21 AM, Markus Armbruster wrote: > Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: > >> From: Daniel P. Berrangé <berrange@redhat.com> >> >> 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. >> >> 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. >> >> The stub impl for non-i386 targets retains this no-op behaviour. >> However, it is now reporting an Error mentioning this command is not >> available for current target. >> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> >> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> > > [...] > >> diff --git a/stubs/monitor-i386-rtc.c b/stubs/monitor-i386-rtc.c >> new file mode 100644 >> index 00000000000..d810f33efec >> --- /dev/null >> +++ b/stubs/monitor-i386-rtc.c >> @@ -0,0 +1,11 @@ >> +/* 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) >> +{ >> + error_setg(errp, >> + "rtc-reset-injection is only available for x86 machines with RTC"); >> +} > > We get this stub exactly when the command did not exist before the > patch. Thus, the command fails before and after, only error code and > message change, which is fine. > > However, the error message feels brittle: it becomes misleading when we > implement the command for other targets. Let's dumb it down to > something like "RTC interrupt reinjection backlog reset is not available > for this machine". > Sounds good, I'll change it. > [...] >
diff --git a/qapi/misc-i386.json b/qapi/misc-i386.json new file mode 100644 index 00000000000..d5bfd91405e --- /dev/null +++ b/qapi/misc-i386.json @@ -0,0 +1,24 @@ +# -*- Mode: Python -*- +# vim: filetype=python +# +# SPDX-License-Identifier: GPL-2.0-or-later + +## +# @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. +# +# 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 42e4a7417dc..5d0ffb0164f 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' } - ## # @SevState: # diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json index 7bc600bb768..96f6aa44133 100644 --- a/qapi/qapi-schema.json +++ b/qapi/qapi-schema.json @@ -61,6 +61,7 @@ { 'include': 'replay.json' } { 'include': 'yank.json' } { 'include': 'misc.json' } +{ 'include': 'misc-i386.json' } { 'include': 'misc-target.json' } { 'include': 'audio.json' } { 'include': 'acpi.json' } diff --git a/hw/i386/monitor.c b/hw/i386/monitor.c index 1921e4d52e9..79df96562f6 100644 --- a/hw/i386/monitor.c +++ b/hw/i386/monitor.c @@ -26,7 +26,7 @@ #include "monitor/monitor.h" #include "qobject/qdict.h" #include "qapi/error.h" -#include "qapi/qapi-commands-misc-target.h" +#include "qapi/qapi-commands-misc-i386.h" #include "hw/i386/x86.h" #include "hw/rtc/mc146818rtc.h" diff --git a/stubs/monitor-i386-rtc.c b/stubs/monitor-i386-rtc.c new file mode 100644 index 00000000000..d810f33efec --- /dev/null +++ b/stubs/monitor-i386-rtc.c @@ -0,0 +1,11 @@ +/* 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) +{ + error_setg(errp, + "rtc-reset-injection is only available for x86 machines with RTC"); +} diff --git a/qapi/meson.build b/qapi/meson.build index eadde4db307..3a9bd061047 100644 --- a/qapi/meson.build +++ b/qapi/meson.build @@ -64,6 +64,7 @@ if have_system 'qdev', 'pci', 'rocker', + 'misc-i386', 'tpm', 'uefi', ] diff --git a/stubs/meson.build b/stubs/meson.build index 63392f5e785..9907b54c1e6 100644 --- a/stubs/meson.build +++ b/stubs/meson.build @@ -77,6 +77,7 @@ if have_system stub_ss.add(files('target-monitor-defs.c')) stub_ss.add(files('win32-kbd-hook.c')) stub_ss.add(files('xen-hw-stub.c')) + stub_ss.add(files('monitor-i386-rtc.c')) endif if have_system or have_user