Re: [PATCH] [for-10.1] qapi/block-core: derpecate some block-job- APIs
Vladimir Sementsov-Ogievskiy writes:
> On 04.04.25 17:13, Markus Armbruster wrote:
[...]
>> So, auto-finalize=true is silently ignored when another job in the same
>> transaction has auto-finalize=false?
>
> Yes, at least, it looks like so:
>
> static void job_completed_txn_success_locked(Job *job)
> {
>
> [...]
>
> /* If no jobs need manual finalization, automatically do so */
> if (job_txn_apply_locked(job, job_needs_finalize_locked) == 0) {
> job_do_finalize_locked(job);
> }
> }
Silently ignoring what the user specified is not okay whether the user's
instructions make sense or not.
Fixing this UI bug would break usage that relies on it. Should we care?
[...]
Re: [PATCH v2 1/2] qapi: synchronize jobs and block-jobs documentation
Vladimir Sementsov-Ogievskiy writes: > Actualize documentation and synchronize it for commands which actually > call the same functions internally. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > qapi/block-core.json | 59 +--- > qapi/job.json| 29 -- > 2 files changed, 61 insertions(+), 27 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index b1937780e1..d74a1f8b8b 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -2956,13 +2956,14 @@ > # > # Pause an active background block operation. > # > -# This command returns immediately after marking the active background > -# block operation for pausing. It is an error to call this command if > -# no operation is in progress or if the job is already paused. > +# This command returns immediately after marking the active job for > +# pausing. Pausing an already paused job is an error. > +# > +# The job will pause as soon as possible, which means transitioning > +# into the PAUSED state if it was RUNNING, or into STANDBY if it was > +# READY. The corresponding JOB_STATUS_CHANGE event will be emitted. > # > -# The operation will pause as soon as possible. No event is emitted > -# when the operation is actually paused. Cancelling a paused job > -# automatically resumes it. > +# Cancelling a paused job automatically resumes it. > # > # @device: The job identifier. This used to be a device name (hence > # the name of the parameter), but since QEMU 2.7 it can have other > @@ -2982,9 +2983,8 @@ > # > # Resume an active background block operation. > # > -# This command returns immediately after resuming a paused background > -# block operation. It is an error to call this command if no > -# operation is in progress or if the job is not paused. > +# This command returns immediately after resuming a paused job. > +# Resuming an already running job is an error. > # > # This command also clears the error status of the job. > # > @@ -3004,10 +3004,14 @@ > ## > # @block-job-complete: > # > -# Manually trigger completion of an active background block operation. > -# This is supported for drive mirroring, where it also switches the > -# device to write to the target path only. The ability to complete is > -# signaled with a BLOCK_JOB_READY event. > +# Manually trigger completion of an active job in the READY or STANDBY > +# state. Completing the job in any other state is an error. > +# > +# This is supported only for drive mirroring (which includes > +# drive-mirror, blockdev-mirror and block-commit job (only in case of > +# "active commit", when the node being commited is used by the guest)), I agree with Eric: needs a rephrasing to avoid the nested parenthesis. > +# where it also switches the device to write to the target path only. > +# The ability to complete is signaled with a BLOCK_JOB_READY event. > # > # This command completes an active background block operation > # synchronously. The ordering of this command's return with the > @@ -3017,8 +3021,6 @@ > # rerror/werror arguments that were specified when starting the > # operation. > # > -# A cancelled or paused job cannot be completed. > -# > # @device: The job identifier. This used to be a device name (hence > # the name of the parameter), but since QEMU 2.7 it can have other > # values. > @@ -3035,10 +3037,12 @@ > ## > # @block-job-dismiss: > # > -# For jobs that have already concluded, remove them from the > -# block-job-query list. This command only needs to be run for jobs > -# which were started with QEMU 2.12+ job lifetime management > -# semantics. > +# Deletes a job that is in the CONCLUDED state. This command only > +# needs to be run explicitly for jobs that don't have automatic > +# dismiss enabled. In turn, automatic dismiss may be enabled only > +# for jobs that have @auto-dismiss option, which are drive-backup, > +# blockdev-backup, drive-mirror, blockdev-mirror, block-commit and > +# block-stream. > # > # This command will refuse to operate on any job that has not yet > # reached its terminal state, JOB_STATUS_CONCLUDED. For jobs that > @@ -3055,12 +3059,17 @@ > ## > # @block-job-finalize: > # > -# Once a job that has manual=true reaches the pending state, it can be > -# instructed to finalize any graph changes and do any necessary > -# cleanup via this command. For jobs in a transaction, instructing > -# one job to finalize will force ALL jobs in the transaction to > -# finalize, so it is only necessary to instruct a single member job to > -# finalize. > +# Instructs all jobs in a transaction (or a single job if it is not > +# part of any transaction) to finalize any graph changes and do any > +# necessary cleanup. This command requires that all involved jobs are > +# in the PENDING state. > +# > +# For jobs in a transaction, instructing one job to finalize will > +# force ALL jobs in the transaction to finalize, so it
Re: [PATCH V1 0/6] fast qom tree get
Steven Sistare writes: > On 4/9/2025 10:44 AM, Markus Armbruster wrote: >> Steven Sistare writes: >> >>> On 4/9/2025 9:34 AM, Markus Armbruster wrote: Steven Sistare writes: > On 4/9/2025 3:39 AM, Markus Armbruster wrote: [...] Anyway, asking you to fix design mistakes all over the place wouldn't be fair. So I'm asking you something else instead: do you actually need the error information? >>> >>> I don't need the specific error message. >>> >>> I could return a boolean meaning "property not available" instead of >>> returning >>> the exact error message, as long as folks are OK with the output of the >>> qom-tree >>> script changing for these properties. >> >> Let's put aside the qom-tree script for a moment. [...] >> Back to qom-tree. I believe this script is a development aid that >> exists because qom-get is painful to use for humans. Your qom-tree >> command would completely obsolete it. I wouldn't worry about it. >> If you think I'm wrong there, please speak up! > > Regarding dropping the error messages, I agree, I was just pointing it out > in case anyone objected. Appreciated. > Yes, the new command plus a formatter like jq obsoletes the qom-tree script. > Just to be clear, I do not propose to delete the script, since folks are > accustomed to it being available, and are accustomed to its output. It also > serves as a nice example for how to use the new command. I have little use for scripts/qmp/ myself. Since nothing there adds to my maintenance load appreciably, I don't mind keeping the scripts. qom-fuse is rather cute. > Do you want to review any code and specification now, or wait for me to send > V2 that deletes the error member? The changes will be minor. v1 should do for review. Thanks!
Re: [PATCH V1 0/6] fast qom tree get
Steven Sistare writes:
> On 4/9/2025 9:34 AM, Markus Armbruster wrote:
>> Steven Sistare writes:
>>> On 4/9/2025 3:39 AM, Markus Armbruster wrote:
Hi Steve, I apologize for the slow response.
Steve Sistare writes:
> Using qom-list and qom-get to get all the nodes and property values in a
> QOM tree can take multiple seconds because it requires 1000's of
> individual
> QOM requests. Some managers fetch the entire tree or a large subset
> of it when starting a new VM, and this cost is a substantial fraction of
> start up time.
"Some managers"... could you name one?
>>>
>>> My personal experience is with Oracle's OCI, but likely others could
>>> benefit.
>>
>> Peter Krempa tells us libvirt would benefit.
>>
> To reduce this cost, consider QAPI calls that fetch more information in
> each call:
> * qom-list-get: given a path, return a list of properties and values.
> * qom-list-getv: given a list of paths, return a list of properties
> and
> values for each path.
> * qom-tree-get: given a path, return all descendant nodes rooted at
> that
> path, with properties and values for each.
Libvirt developers, would you be interested in any of these?
> In all cases, a returned property is represented by ObjectPropertyValue,
> with fields name, type, value, and error. If an error occurs when reading
> a value, the value field is omitted, and the error message is returned in
> the
> the error field. Thus an error for one property will not cause a bulk
> fetch
> operation to fail.
Returning errors this way is highly unusual. Observation; I'm not
rejecting this out of hand. Can you elaborate a bit on why it's useful?
>>>
>>> It is considered an error to read some properties if they are not valid for
>>> the configuration. And some properties are write-only and return an error
>>> if they are read. Examples:
>>>
>>> legacy-i8042: >> readable> (str)
>>> legacy-memory: >> is not readable> (str)
>>> crash-information:
>>> (GuestPanicInformation)
>>>
>>> With conventional error handling, if any of these poison pills falls in the
>>> scope of a bulk get operation, the entire operation fails.
>>
>> I suspect many of these poison pills are design mistakes.
>>
>> If a property is not valid for the configuration, why does it exist?
>> QOM is by design dynamic. I wish it wasn't, but as long as it is
>> dynamic, I can't see why we should create properties we know to be
>> unusable.
>>
>> Why is reading crash-information an error when no crash occured? This
>> is the *normal* case. Errors are for the abnormal.
>>
>> Anyway, asking you to fix design mistakes all over the place wouldn't be
>> fair. So I'm asking you something else instead: do you actually need
>> the error information?
>
> I don't need the specific error message.
>
> I could return a boolean meaning "property not available" instead of returning
> the exact error message, as long as folks are OK with the output of the
> qom-tree
> script changing for these properties.
Let's put aside the qom-tree script for a moment.
In your patches, the queries return an object's properties as a list of
ObjectPropertyValue, defined as
{ 'struct': 'ObjectPropertyValue',
'data': { 'name': 'str',
'type': 'str',
'*value': 'any',
'*error': 'str' } }
As far as I understand, exactly one of @value and @error are present.
The list has no duplicates, i.e. no two elements have the same value of
"name".
Say we're interested in property "foo". Three cases:
* The list has an element with "name": "foo", and the element has member
"value": the property exists and "value" has its value.
* The list has an element with "name": "foo", and the element does not
have member "value": the property exists, but its value cannot be
gotten; member "error" has the error message.
* The list has no element with "name": "foo": the property does not
exist.
If we simply drop ObjectPropertyValue member @error, we lose 'member
"error" has the error message'. That's all.
If a need for more error information should arise later, we could add
member @error. Or something else entirely. Or tell people to qom-get
any properties qom-tree-get couldn't get for error information. My
point is: dropping @error now does not tie our hands as far as I can
tell.
Back to qom-tree. I believe this script is a development aid that
exists because qom-get is painful to use for humans. Your qom-tree
command would completely obsolete it. I wouldn't worry about it.
If you think I'm wrong there, please speak up!
Re: [PATCH V1 0/6] fast qom tree get
Steven Sistare writes: > On 4/9/2025 3:39 AM, Markus Armbruster wrote: >> Hi Steve, I apologize for the slow response. >> >> Steve Sistare writes: >> >>> Using qom-list and qom-get to get all the nodes and property values in a >>> QOM tree can take multiple seconds because it requires 1000's of individual >>> QOM requests. Some managers fetch the entire tree or a large subset >>> of it when starting a new VM, and this cost is a substantial fraction of >>> start up time. >> >> "Some managers"... could you name one? > > My personal experience is with Oracle's OCI, but likely others could benefit. Peter Krempa tells us libvirt would benefit. >>> To reduce this cost, consider QAPI calls that fetch more information in >>> each call: >>>* qom-list-get: given a path, return a list of properties and values. >>>* qom-list-getv: given a list of paths, return a list of properties and >>> values for each path. >>>* qom-tree-get: given a path, return all descendant nodes rooted at that >>> path, with properties and values for each. >> >> Libvirt developers, would you be interested in any of these? >> >>> In all cases, a returned property is represented by ObjectPropertyValue, >>> with fields name, type, value, and error. If an error occurs when reading >>> a value, the value field is omitted, and the error message is returned in >>> the >>> the error field. Thus an error for one property will not cause a bulk fetch >>> operation to fail. >> >> Returning errors this way is highly unusual. Observation; I'm not >> rejecting this out of hand. Can you elaborate a bit on why it's useful? > > It is considered an error to read some properties if they are not valid for > the configuration. And some properties are write-only and return an error > if they are read. Examples: > >legacy-i8042: > (str) >legacy-memory: not readable> (str) >crash-information: (GuestPanicInformation) > > With conventional error handling, if any of these poison pills falls in the > scope of a bulk get operation, the entire operation fails. I suspect many of these poison pills are design mistakes. If a property is not valid for the configuration, why does it exist? QOM is by design dynamic. I wish it wasn't, but as long as it is dynamic, I can't see why we should create properties we know to be unusable. Why is reading crash-information an error when no crash occured? This is the *normal* case. Errors are for the abnormal. Anyway, asking you to fix design mistakes all over the place wouldn't be fair. So I'm asking you something else instead: do you actually need the error information? [...]
Re: [PATCH v3 1/2] qapi: synchronize jobs and block-jobs documentation
Vladimir Sementsov-Ogievskiy writes: > Actualize documentation and synchronize it for commands which actually > call the same functions internally. > > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Markus Armbruster
Re: [PATCH V1 0/6] fast qom tree get
Hi Steve, I apologize for the slow response. Steve Sistare writes: > Using qom-list and qom-get to get all the nodes and property values in a > QOM tree can take multiple seconds because it requires 1000's of individual > QOM requests. Some managers fetch the entire tree or a large subset > of it when starting a new VM, and this cost is a substantial fraction of > start up time. "Some managers"... could you name one? > To reduce this cost, consider QAPI calls that fetch more information in > each call: > * qom-list-get: given a path, return a list of properties and values. > * qom-list-getv: given a list of paths, return a list of properties and > values for each path. > * qom-tree-get: given a path, return all descendant nodes rooted at that > path, with properties and values for each. Libvirt developers, would you be interested in any of these? > In all cases, a returned property is represented by ObjectPropertyValue, > with fields name, type, value, and error. If an error occurs when reading > a value, the value field is omitted, and the error message is returned in the > the error field. Thus an error for one property will not cause a bulk fetch > operation to fail. Returning errors this way is highly unusual. Observation; I'm not rejecting this out of hand. Can you elaborate a bit on why it's useful? > To evaluate each method, I modified scripts/qmp/qom-tree to use the method, > verified all methods produce the same output, and timed each using: > > qemu-system-x86_64 -display none \ > -chardev socket,id=monitor0,path=/tmp/vm1.sock,server=on,wait=off \ > -mon monitor0,mode=control & > > time qom-tree -s /tmp/vm1.sock > /dev/null Cool! > I only measured once per method, but the variation is low after a warm up run. > The 'real - user - sys' column is a proxy for QEMU CPU time. > > method real(s) user(s) sys(s) (real - user - sys)(s) > qom-list / qom-get 2.048 0.932 0.057 1.059 > qom-list-get 0.402 0.230 0.029 0.143 > qom-list-getv0.200 0.132 0.015 0.053 > qom-tree-get 0.143 0.123 0.012 0.008 > > qom-tree-get is the clear winner, reducing elapsed time by a factor of 14X, > and reducing QEMU CPU time by 132X. > > qom-list-getv is slower when fetching the entire tree, but can beat > qom-tree-get when only a subset of the tree needs to be fetched (not shown). > > qom-list-get is shown for comparison only, and is not included in this series. If we have qom-list-getv, then qom-list-get is not worth having.
Management applications and CPU feature flags (was: [PATCH V1 0/6] fast qom tree get)
Daniel P. Berrangé writes:
> On Wed, Apr 09, 2025 at 09:58:13AM +0200, Peter Krempa via Devel wrote:
>> On Wed, Apr 09, 2025 at 09:39:02 +0200, Markus Armbruster via Devel wrote:
>> > Hi Steve, I apologize for the slow response.
>> >
>> > Steve Sistare writes:
>> >
>> > > Using qom-list and qom-get to get all the nodes and property values in a
>> > > QOM tree can take multiple seconds because it requires 1000's of
>> > > individual
>> > > QOM requests. Some managers fetch the entire tree or a large subset
>> > > of it when starting a new VM, and this cost is a substantial fraction of
>> > > start up time.
>> >
>> > "Some managers"... could you name one?
>>
>> libvirt is at ~500 qom-get calls during an average startup ...
>>
>> > > To reduce this cost, consider QAPI calls that fetch more information in
>> > > each call:
>> > > * qom-list-get: given a path, return a list of properties and values.
>> > > * qom-list-getv: given a list of paths, return a list of properties and
>> > > values for each path.
>> > > * qom-tree-get: given a path, return all descendant nodes rooted at
>> > > that
>> > > path, with properties and values for each.
>> >
>> > Libvirt developers, would you be interested in any of these?
>>
>> YES!!!
>
> Not neccessarily, see below...
>
>>
>> The getter with value could SO MUCH optimize the startup sequence of a
>> VM where libvirt needs to probe CPU flags:
>>
>> (note the 'id' field in libvirt's monitor is sequential)
>>
>> buf={"execute":"qom-get","arguments":{"path":"/machine/unattached/device[0]","property":"realized"},"id":"libvirt-8"}
>> buf={"execute":"qom-get","arguments":{"path":"/machine/unattached/device[0]","property":"hotplugged"},"id":"libvirt-9"}
>> buf={"execute":"qom-get","arguments":{"path":"/machine/unattached/device[0]","property":"hotpluggable"},"id":"libvirt-10"}
>>
>> [...]
>>
>> buf={"execute":"qom-get","arguments":{"path":"/machine/unattached/device[0]","property":"hv-apicv"},"id":"libvirt-470"}
>> buf={"execute":"qom-get","arguments":{"path":"/machine/unattached/device[0]","property":"xd"},"id":"libvirt-471"}
>> buf={"execute":"qom-get","arguments":{"path":"/machine/unattached/device[0]","property":"sse4_1"},"id":"libvirt-472"}
>> buf={"execute":"qom-get","arguments":{"path":"/machine/unattached/device[0]","property":"unavailable-features"},"id":"libvirt-473"}
>>
>> First and last line's timestamps:
>>
>> 2025-04-08 14:44:28.882+: 1481190: info : qemuMonitorIOWrite:340 :
>> QEMU_MONITOR_IO_WRITE: mon=0x7f4678048360
>> buf={"execute":"qom-get","arguments":{"path":"/machine/unattached/device[0]","property":"realized"},"id":"libvirt-8"}
>>
>> 2025-04-08 14:44:29.149+: 1481190: info : qemuMonitorIOWrite:340 :
>> QEMU_MONITOR_IO_WRITE: mon=0x7f4678048360
>> buf={"execute":"qom-get","arguments":{"path":"/machine/unattached/device[0]","property":"unavailable-features"},"id":"libvirt-473"}
>>
>> Libvirt spent ~170 ms probing cpu flags.
>
> One thing I would point out is that qom-get can be considered an
> "escape hatch" to get information when no better QMP command exists.
> In this case, libvirt has made the assumption that every CPU feature
> is a QOM property.
>
> Adding qom-list-get doesn't appreciably change that, just makes the
> usage more efficient.
>
> Considering the bigger picture QMP design, when libvirt is trying to
> understand QEMU's CPU feature flag expansion, I would ask why we don't
> have something like a "query-cpu" command to tell us the current CPU
> expansion, avoiding the need for poking at QOM properties directly.
How do the existing query-cpu-FOO fall short of what management
applications such as libvirt needs?
Re: Management applications and CPU feature flags
Daniel P. Berrangé writes:
> On Fri, Apr 11, 2025 at 12:40:46PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé writes:
>>
>> > On Wed, Apr 09, 2025 at 09:58:13AM +0200, Peter Krempa via Devel wrote:
>> >> On Wed, Apr 09, 2025 at 09:39:02 +0200, Markus Armbruster via Devel wrote:
>> >> > Hi Steve, I apologize for the slow response.
>> >> >
>> >> > Steve Sistare writes:
>> >> >
>> >> > > Using qom-list and qom-get to get all the nodes and property values
>> >> > > in a
>> >> > > QOM tree can take multiple seconds because it requires 1000's of
>> >> > > individual
>> >> > > QOM requests. Some managers fetch the entire tree or a large subset
>> >> > > of it when starting a new VM, and this cost is a substantial fraction
>> >> > > of
>> >> > > start up time.
>> >> >
>> >> > "Some managers"... could you name one?
>> >>
>> >> libvirt is at ~500 qom-get calls during an average startup ...
>> >>
>> >> > > To reduce this cost, consider QAPI calls that fetch more information
>> >> > > in
>> >> > > each call:
>> >> > > * qom-list-get: given a path, return a list of properties and
>> >> > > values.
>> >> > > * qom-list-getv: given a list of paths, return a list of properties
>> >> > > and
>> >> > > values for each path.
>> >> > > * qom-tree-get: given a path, return all descendant nodes rooted at
>> >> > > that
>> >> > > path, with properties and values for each.
>> >> >
>> >> > Libvirt developers, would you be interested in any of these?
>> >>
>> >> YES!!!
>> >
>> > Not neccessarily, see below...
>> >
>> >>
>> >> The getter with value could SO MUCH optimize the startup sequence of a
>> >> VM where libvirt needs to probe CPU flags:
>> >>
>> >> (note the 'id' field in libvirt's monitor is sequential)
>> >>
>> >> buf={"execute":"qom-get","arguments":{"path":"/machine/unattached/device[0]","property":"realized"},"id":"libvirt-8"}
>> >> buf={"execute":"qom-get","arguments":{"path":"/machine/unattached/device[0]","property":"hotplugged"},"id":"libvirt-9"}
>> >> buf={"execute":"qom-get","arguments":{"path":"/machine/unattached/device[0]","property":"hotpluggable"},"id":"libvirt-10"}
>> >>
>> >> [...]
>> >>
>> >> buf={"execute":"qom-get","arguments":{"path":"/machine/unattached/device[0]","property":"hv-apicv"},"id":"libvirt-470"}
>> >> buf={"execute":"qom-get","arguments":{"path":"/machine/unattached/device[0]","property":"xd"},"id":"libvirt-471"}
>> >> buf={"execute":"qom-get","arguments":{"path":"/machine/unattached/device[0]","property":"sse4_1"},"id":"libvirt-472"}
>> >> buf={"execute":"qom-get","arguments":{"path":"/machine/unattached/device[0]","property":"unavailable-features"},"id":"libvirt-473"}
>> >>
>> >> First and last line's timestamps:
>> >>
>> >> 2025-04-08 14:44:28.882+: 1481190: info : qemuMonitorIOWrite:340 :
>> >> QEMU_MONITOR_IO_WRITE: mon=0x7f4678048360
>> >> buf={"execute":"qom-get","arguments":{"path":"/machine/unattached/device[0]","property":"realized"},"id":"libvirt-8"}
>> >>
>> >> 2025-04-08 14:44:29.149+: 1481190: info : qemuMonitorIOWrite:340 :
>> >> QEMU_MONITOR_IO_WRITE: mon=0x7f4678048360
>> >> buf={"execute":"qom-get","arguments":{"path":"/machine/unattached/device[0]","property":"unavailable-features"},"id":"libvirt-473"}
>> >>
>> >> Libvirt spent ~170 ms probing cpu flags.
>> >
>> > One thing I would point out is that qom-get can be considered an
>> > "escape hatch" to get information when no better QMP command exists.
>> > In this case, libvirt has made the assumption that every CPU feature
>> > is a QOM property.
>> >
>> > Adding qom-list-get doesn't appreciably change that, just makes the
>> > usage more efficient.
>> >
>> > Considering the bigger picture QMP design, when libvirt is trying to
>> > understand QEMU's CPU feature flag expansion, I would ask why we don't
>> > have something like a "query-cpu" command to tell us the current CPU
>> > expansion, avoiding the need for poking at QOM properties directly.
>>
>> How do the existing query-cpu-FOO fall short of what management
>> applications such as libvirt needs?
>
> It has been along while since I looked at them, but IIRC they were
> returning static info about CPU models, whereas libvirt wanted info
> on the currently requested '-cpu ARGS'
Libvirt developers, please work with us on design of new commands or
improvements to existing ones to better meet libvirt's needs in this
area.
Re: [RFC PATCH 0/3] single-binary: make QAPI generated files common
Pierrick Bouvier writes:
> On 4/25/25 08:38, Markus Armbruster wrote:
>> Pierrick Bouvier writes:
>>
>>> Note: This RFC was posted to trigger a discussion around this topic, and
>>> it's
>>> not expected to merge it as it is.
>>>
>>> Context
>>> ===
>>>
>>> Linaro is working towards heterogeneous emulation, mixing several
>>> architectures
>>> in a single QEMU process. The first prerequisite is to be able to build
>>> such a
>>> binary, which we commonly name "single-binary" in our various series.
>>> An (incomplete) list of series is available here:
>>> https://patchew.org/search?q=project%3AQEMU+single-binary
>>>
>>> We don't expect to change existing command line interface or any observable
>>> behaviour, it should be identical to existing binaries. If anyone notices a
>>> difference, it will be a bug.
>>
>> Define "notice a difference" :) More on that below.
>>
>
> Given a single-binary *named* exactly like an existing qemu-system-X
> binary, any user or QEMU management layer should not be able to
> distinguish it from the real qemu-system-X binary.
>
> The new potential things will be:
> - introduction of an (optional) -target option, which allows to
> override/disambiguate default target detected.
> - potentially more boards/cpus/devices visible, once we start developing
> heterogeneous emulation. See it as a new CONFIG_{new_board} present.
>
> Out of that, once the current target is identified, based on argv[0],
> there should be absolutely no difference, whether in the behaviour, UI,
> command line, or the monitor interfaces.
Letting argv[0] affect behavior is problematic. See prior discussion:
Subject: Re: [RFC PATCH 04/18] qemu: Introduce 'qemu/legacy_binary_info.h'
Message-ID: <87bjttzh3b@pond.sub.org>
https://lore.kernel.org/qemu-devel/87bjttzh3b@pond.sub.org/
> Maybe (i.e. probably) one day people will be interested to create a new
> shiny command line for heteregenous scenarios, but for now, this is
> *not* the goal we pursue. We just want to be able to manually define a
> board mixing two different cpu architectures, without reinventing all
> the wheels coming with that. Once everything is ready (and not before),
> it will be a good time to revisit the command line interface to reflect
> this. Definitely a small task compared to all we have left to do now.
>
> Finally, even if we decide to do those changes, I think they should be
> reflected on both existing binaries and the new single-binary. It would
> be a mistake to create "yet another" way to use QEMU, just because we
> have N architectures available instead of one.
>
>>> The first objective we target is to combine qemu-system-arm and
>>> qemu-system-aarch64 in a single binary, showing that we can build and link
>>> such
>>> a thing. While being useless from a feature point of view, it allows us to
>>> make
>>> good progress towards the goal, and unify two "distinct" architectures, and
>>> gain
>>> experience on problems met.
>>
>> Makes sense to me.
>>
>>> Our current approach is to remove compilation units duplication to be able
>>> to
>>> link all object files together. One of the concerned subsystem is QAPI.
>>>
>>> QAPI
>>>
>>>
>>> QAPI generated files contain conditional clauses to define various
>>> structures,
>>> enums, and commands only for specific targets. This forces files to be
>>> compiled for every target.
>>
>> To be precise: conditionals that use macros restricted to
>> target-specific code, i.e. the ones poisoned by exec/poison.h. Let's
>> call them target-specific QAPI conditionals.
>>
>> The QAPI generator is blissfully unaware of all this.
>>
>
> Indeed, the only thing QAPI generaor is aware of is that it's a compile
> time definition, since it implements those with #if, compared to a
> runtime check.
QAPI conditionals are designed to provide build-time configuration. In
C, we use #if for that. QAPI conditionals are merely a slight
abstraction of C #if. Used to be even slighter until merge commit
c83fcfaf8a5.
Two kinds of #if conditions in QEMU C code:
1. True build-time configuration, i.e. control what's being built based
on what the host can do and what the user wants, as figured out by
configure. These conditions are the same for the entire build.
2. Specializing target-specific code. These conditions vary per target.
C code using them needs to be compiled per target.
To get a single binary, we want to reduce use of the second kind as much
as practical. This can involve replacing compile-time conditionals by
run-time checks. Remaining target-specific code needs to adjusted so
target-specific code for multiple targets can coexist in the single
binary.
Trouble is some uses of the second kind are in QAPI conditionals. I can
see three options:
(1) Drop these conditionals.
(2) Replace them by run-time checks.
(3) Have target-specific QAPI-generated code for multiple targets
coexist in the single binary.
As far as I can tell,
Re: [PATCH v2 2/2] qapi/block-core: derpecate some block-job- APIs
Typo in subject, make it "deprecate". Vladimir Sementsov-Ogievskiy writes: > For change, pause, resume, complete, dismiss and finalize actions > corresponding job- and block-job commands are almost equal. The > difference is in find_block_job_locked() vs find_job_locked() > functions. What's different? > > 1. find_block_job_locked() do check, is found job a block-job. This OK >when moving to more generic API, no needs to document this change. > > 2. find_block_job_locked() reports DeviceNotActive on failure, when >find_job_locked() reports GenericError. So, lets document this >difference in deprecated.txt. Still, for dismiss and finalize errors >are not documented at all, so be silent in deprecated.txt as well. > > ACKed-by: Peter Krempa > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Markus Armbruster
Re: [RFC PATCH 0/3] single-binary: make QAPI generated files common
Pierrick Bouvier writes:
> Note: This RFC was posted to trigger a discussion around this topic, and it's
> not expected to merge it as it is.
>
> Context
> ===
>
> Linaro is working towards heterogeneous emulation, mixing several
> architectures
> in a single QEMU process. The first prerequisite is to be able to build such a
> binary, which we commonly name "single-binary" in our various series.
> An (incomplete) list of series is available here:
> https://patchew.org/search?q=project%3AQEMU+single-binary
>
> We don't expect to change existing command line interface or any observable
> behaviour, it should be identical to existing binaries. If anyone notices a
> difference, it will be a bug.
Define "notice a difference" :) More on that below.
> The first objective we target is to combine qemu-system-arm and
> qemu-system-aarch64 in a single binary, showing that we can build and link
> such
> a thing. While being useless from a feature point of view, it allows us to
> make
> good progress towards the goal, and unify two "distinct" architectures, and
> gain
> experience on problems met.
Makes sense to me.
> Our current approach is to remove compilation units duplication to be able to
> link all object files together. One of the concerned subsystem is QAPI.
>
> QAPI
>
>
> QAPI generated files contain conditional clauses to define various structures,
> enums, and commands only for specific targets. This forces files to be
> compiled for every target.
To be precise: conditionals that use macros restricted to
target-specific code, i.e. the ones poisoned by exec/poison.h. Let's
call them target-specific QAPI conditionals.
The QAPI generator is blissfully unaware of all this.
The build system treats QAPI modules qapi/*-target.json as
target-specific. The .c files generated for them are compiled per
target. See qapi/meson.build.
Only such target-specific modules can can use target-specific QAPI
conditionals. Use in target-independent modules will generate C that
won't compile.
Poisoned macros used in qapi/*-target.json:
CONFIG_KVM
TARGET_ARM
TARGET_I386
TARGET_LOONGARCH64
TARGET_MIPS
TARGET_PPC
TARGET_RISCV
TARGET_S390X
>What we try to do here is to build them only once
> instead.
You're trying to eliminate target-specific QAPI conditionals. Correct?
> In the past, we identied that the best approach to solve this is to expose
> code
> for all targets (thus removing all #if clauses), and stub missing
> symbols for concerned targets.
This affects QAPI/QMP introspection, i.e. the value of query-qmp-schema.
Management applications can no longer use introspection to find out
whether target-specific things are available.
For instance, query-cpu-definitions is implemented for targets arm,
i386, loongarch, mips, ppc, riscv, and s390x. It initially was for
fewer targets, and more targets followed one by one. Still more may
follow in the future. Right now, management applications can use
introspection to find out whether it is available. That stops working
when you make it available for all targets, stubbed out for the ones
that don't (yet) implement it.
Management applications may have to be adjusted for this.
This is not an attempt to shoot down your approach. I'm merely
demonstrating limitations of your promise "if anyone notices a
difference, it will be a bug."
Now, we could get really fancy and try to keep introspection the same by
applying conditionals dynamically somehow. I.e. have the single binary
return different introspection values depending on the actual guest's
target.
This requires fixing the target before introspection. Unless this is
somehow completely transparent (wrapper scripts, or awful hacks based on
the binary's filename, perhaps), management applications may have to be
adjusted to actually do that.
Applies not just to introspection. Consider query-cpu-definitions
again. It currently returns CPU definitions for *the* target. What
would a single binary's query-cpu-definitions return? The CPU
definitions for *all* its targets? Management applications then receive
CPUs that won't work, which may upset them. To avoid noticable
difference, we again have to fix the target before we look.
Of course, "fixing the target" stops making sense once we move to
heterogeneous machines with multiple targets.
> This series build QAPI generated code once, by removing all TARGET_{arch} and
> CONFIG_KVM clauses. What it does *not* at the moment is:
> - prevent target specific commands to be visible for all targets
> (see TODO comment on patch 2 explaining how to address this)
> - nothing was done to hide all this from generated documentation
For better or worse, generated documentation always contains everything.
An argument could be made for stripping out documentation for the stuff
that isn't included in this build.
> From what I understood, the only thing that matters is to limit qmp commands
> visi
Re: [PATCH V1 0/6] fast qom tree get
Steven Sistare writes: > On 4/9/2025 3:39 AM, Markus Armbruster wrote: >> Hi Steve, I apologize for the slow response. >> >> Steve Sistare writes: >> >>> Using qom-list and qom-get to get all the nodes and property values in a >>> QOM tree can take multiple seconds because it requires 1000's of individual >>> QOM requests. Some managers fetch the entire tree or a large subset >>> of it when starting a new VM, and this cost is a substantial fraction of >>> start up time. >> >> "Some managers"... could you name one? > > My personal experience is with Oracle's OCI, but likely others could benefit. Elsewhere in this thread, we examined libvirt's use qom-get. Its use of qom-get is also noticably slow, and your work could speed it up. However, most of its use is for working around QMP interface shortcomings around probing CPU flags. Addressing these would help it even more. This makes me wonder what questions Oracle's OCI answers with the help of qom-get. Can you briefly describe them? Even if OCI would likewise be helped more by better QMP queries, your fast qom tree get work might still be useful.
Re: [RFC PATCH 0/3] single-binary: make QAPI generated files common
Peter Krempa writes: > On Fri, Apr 25, 2025 at 17:38:44 +0200, Markus Armbruster via Devel wrote: >> Pierrick Bouvier writes: > > [...] > >> To be precise: conditionals that use macros restricted to >> target-specific code, i.e. the ones poisoned by exec/poison.h. Let's >> call them target-specific QAPI conditionals. >> >> The QAPI generator is blissfully unaware of all this. >> >> The build system treats QAPI modules qapi/*-target.json as >> target-specific. The .c files generated for them are compiled per >> target. See qapi/meson.build. >> >> Only such target-specific modules can can use target-specific QAPI >> conditionals. Use in target-independent modules will generate C that >> won't compile. >> >> Poisoned macros used in qapi/*-target.json: >> >> CONFIG_KVM >> TARGET_ARM >> TARGET_I386 >> TARGET_LOONGARCH64 >> TARGET_MIPS >> TARGET_PPC >> TARGET_RISCV >> TARGET_S390X Commands and events: CPU introspection: query-cpu-model-baseline, query-cpu-model-comparison, query-cpu-model-expansion, query-cpu-definitions S390 KVM CPU stuff: set-cpu-topology, CPU_POLARIZATION_CHANGE, query-s390x-cpu-polarization. GIC: query-gic-capabilities SEV: query-sev, query-sev-launch-measure, query-sev-capabilities, sev-inject-launch-secret, query-sev-attestation-report SGX: query-sgx, query-sgx-capabilities Xen: xen-event-list, xen-event-inject An odd duck: rtc-reset-reinjection > I've had a look at what bits of the QMP schema are depending on the > above defines which libvirt uses. > > In many cases libvirt could restrict the use of given command/property > to only supported architectures. We decided to simply probe the presence > of the command because it's convenient to not have to filter them any > more > > - query-gic-capabilities > - libvirt already calls this only for ARM guests based on the > definition > > - query-sev and friends > - libvirt uses presence of 'query-sev' to decide if the binary > supports it; patching in a platofrm check is possible although > inconvenient > > - query-sgx and friends > - similar to sev > > -query-cpu-definitions and friends > - see below Large subset of my list. >> >What we try to do here is to build them only >> > once >> instead. >> >> You're trying to eliminate target-specific QAPI conditionals. Correct? >> >> > In the past, we identied that the best approach to solve this is to expose >> > code >> > for all targets (thus removing all #if clauses), and stub missing >> > symbols for concerned targets. >> >> This affects QAPI/QMP introspection, i.e. the value of query-qmp-schema. >> >> Management applications can no longer use introspection to find out >> whether target-specific things are available. > > Indeed and libvirt already uses this in few cases as noded above. > >> >> For instance, query-cpu-definitions is implemented for targets arm, >> i386, loongarch, mips, ppc, riscv, and s390x. It initially was for >> fewer targets, and more targets followed one by one. Still more may >> follow in the future. Right now, management applications can use >> introspection to find out whether it is available. That stops working >> when you make it available for all targets, stubbed out for the ones >> that don't (yet) implement it. >> >> Management applications may have to be adjusted for this. >> >> This is not an attempt to shoot down your approach. I'm merely >> demonstrating limitations of your promise "if anyone notices a >> difference, it will be a bug." >> >> Now, we could get really fancy and try to keep introspection the same by >> applying conditionals dynamically somehow. I.e. have the single binary >> return different introspection values depending on the actual guest's >> target. > > I wonder how this will work if libvirt is probing a binary. Libvirt does > not look at the filename. It can't because it can be a > user-specified/compiled binary, override script, or a distro that chose > to rename the binary. > > The second thing that libvirt does after 'query-version' is > 'query-target'. > > So what should libvirt do once multiple targets are supported? > > How do we query CPUs for each of the supported targets? > > Will the result be the same if we query them one at a time or all at > once? Pierrick's stated goal is to have no noticabl
Re: [RFC PATCH 0/3] single-binary: make QAPI generated files common
Pierrick Bouvier writes:
> On 4/28/25 4:07 AM, Markus Armbruster wrote:
>> Peter Krempa writes:
>>
>>> So what should libvirt do once multiple targets are supported?
>>>
>>> How do we query CPUs for each of the supported targets?
>>>
>
> It's kind of a similar question we have to solve now with QEMU code.
> What happens when a symbol is duplicated, and available only for several
> targets?
>
> In this case, we found various approaches to solve this:
> - unify this symbol for all targets (single implementation)
> - unify all targets to provide this symbol (multiple impl, all targets)
> - rename symbols adding {arch} suffix, so it's disambiguated by name
> - create a proper interface which an available function (multiple impl,
> selective targets)
>
> In the case of query-cpu-definitions, my intuition is that we want to
> have a single implementation, and that we return *all* the cpus, merging
> all architectures. In the end, we (and libvirt also) should think out of
> the "target" box. It's an implementation detail, based on the fact QEMU
> had 'targets' associated to various binaries for a long time and not a
> concept that should leak into all consumers.
>
>>> Will the result be the same if we query them one at a time or all at
>>> once?
>>
>> Pierrick's stated goal is to have no noticable differences between the
>> single binary and the qemu-system- it covers. This is obviously
>> impossible if we can interact with the single binary before the target
>> is fixed.
>>
>
> Right.
> At this point, we can guarantee the target will be fixed before anything
> else, at the start of main(). It's obviously an implementation choice,
> but to be honest, I don't see what we would gain from having a "null"
> default QEMU target, unable to emulate anything.
>
This requires fixing the target before introspection. Unless this is
somehow completely transparent (wrapper scripts, or awful hacks based on
the binary's filename, perhaps), management applications may have to be
adjusted to actually do that.
>>>
>>> As noted filename will not work. Users can specify any filename and
>>> create override scripts or rename the binary.
>>
>> True.
>>
>
> I would prefer to not open this pandora box on this thread, but don't
> worry, the best will be done to support all those cases, including
> renaming the binary, allowing any prefix, suffix, as long as name stays
> unambiguous. If you rename it to qemu-ok, how can you expect anything?
>
> We can provide the possibility to have a "default" target set at compile
> time, for distributors creating their own specific QEMU binaries. But in
> the context of classical software distribution, it doesn't make any sense.
I don't wish to derail this thread, but we've been dancing around the
question of how to best fix the target for some time. I think we should
talk about it for real.
Mind, this is not an objection to your larger "single binary" idea. It
could be only if it was an intractable problem, but I don't think it is.
You want the single binary you're trying to create to be a drop-in
replacement for per-target binaries.
"Drop-in replacement" means existing usage continues to work.
Additional interfaces are not a problem.
To achieve "drop-in replacement", the target needs to be fixed
automatically, and before the management application can further
interact with it.
If I understand you correctly, you're proposing to use argv[0] for that,
roughly like this: assume it's qemu-system-, extract
first thing in main(), done.
What if it's not named that way? If I understand you correctly, you're
proposing to fall back to a compiled-in default target.
I don't think this is going to fly.
Developers rename the binary all the time, and expect this not to change
behavior. For instance, I routinely rename qemu-FOO to qemu-FOO.old or
qemu-FOO.COMMIT-HASH to let me compare behavior easily.
We could relax the assumption to support such renames. Developers then
need to be aware of what renames are supported. Meh.
The more we relax the pattern, the likelier surprising behavior becomes.
We could mitigate surprises by eliminating the built-in default target.
Users invoke their binaries with their own names, too. If Joe R. User
finds qemu-system- too much to type, and creates a
symlink named q to it, more power to him!
Distributions have packaged renamed binaries. qemu-kvm has been used
quite widely.
In neither of these cases, relaxing the pattern helps.
The least bad solution I can see so far is a new option -target.
Instead of turning the target-specific binaries into links to / copies
of the single binary, they become wrappers that pass -target as the
first option. We need to make sure this option is honored in time then,
which should be easy enough.
If you invoke the single binary directly, you need to pass -target
yourself. If you don't to pass it, or pass it late in the command line,
you open up a window for interaction with indeter
Re: [PATCH V1 0/6] fast qom tree get
Steven Sistare writes: > On 4/28/2025 4:04 AM, Markus Armbruster wrote: >> Steven Sistare writes: >> >>> On 4/9/2025 3:39 AM, Markus Armbruster wrote: Hi Steve, I apologize for the slow response. Steve Sistare writes: > Using qom-list and qom-get to get all the nodes and property values in a > QOM tree can take multiple seconds because it requires 1000's of > individual > QOM requests. Some managers fetch the entire tree or a large subset > of it when starting a new VM, and this cost is a substantial fraction of > start up time. "Some managers"... could you name one? >>> >>> My personal experience is with Oracle's OCI, but likely others could >>> benefit. >> >> Elsewhere in this thread, we examined libvirt's use qom-get. Its use of >> qom-get is also noticably slow, and your work could speed it up. >> However, most of its use is for working around QMP interface >> shortcomings around probing CPU flags. Addressing these would help it >> even more. >> >> This makes me wonder what questions Oracle's OCI answers with the help >> of qom-get. Can you briefly describe them? >> >> Even if OCI would likewise be helped more by better QMP queries, your >> fast qom tree get work might still be useful. > > We already optimized our queries as a first step, but what remains is still > significant, which is why I submitted this RFE. I understand your motivation. I'd like to learn more on what OCI actually needs from QMP, to be able to better serve it and potentially other management applications.
Re: [RFC PATCH 0/3] single-binary: make QAPI generated files common
Pierrick Bouvier writes:
> On 4/25/25 11:21 PM, Markus Armbruster wrote:
>> Trouble is some uses of the second kind are in QAPI conditionals. I can
>> see three options:
>>
>> (1) Drop these conditionals.
>>
>> (2) Replace them by run-time checks.
>>
>> (3) Have target-specific QAPI-generated code for multiple targets
>> coexist in the single binary.
>>
>> As far as I can tell, your RFC series is an incomplete attempt at (2).
>>
>> I gather you considered (3), but you dislike it for its bloat and
>> possibly other reasons. I sympathize; the QAPI-generated code is plenty
>> bloated as it is, in good part to early design decisions (not mine).
>>
>> Your "no noticeable differences" goal precludes (1).
>>
>> Back to (2). In C, replacing compile-time conditionals by run-time
>> checks means replacing #if FOO by if (foo). Such a transformation isn't
>> possible in the QAPI schema. To make it possible, we need to evolve the
>> QAPI schema language.
>>
>> docs/devel/qapi-code-gen.rst describes what we have:
>>
>> COND = STRING
>> | { 'all: [ COND, ... ] }
>> | { 'any: [ COND, ... ] }
>> | { 'not': COND }
>>
>> []
>>
>> The C code generated for the definition will then be guarded by an #if
>> preprocessing directive with an operand generated from that condition:
>>
>> * STRING will generate defined(STRING)
>> * { 'all': [COND, ...] } will generate (COND && ...)
>> * { 'any': [COND, ...] } will generate (COND || ...)
>> * { 'not': COND } will generate !COND
>>
>> So, conditions are expression trees where the leaves are preprocessor
>> symbols and the inner nodes are operators.
>>
>> It's not quite obvious to me how to best evolve this to support run-time
>> checks.
>>
>
> After looking at the introspection code, I don't see any major blocker.
> We need to keep some of existing "if", as they are based on config-host,
> and should apply.
> We can introduce a new "available_if" (or any other name), which
> generates a runtime check when building the schema, or when serializing
> a struct.
>
> This way, by modifying the .json with:
> - if: 'TARGET_I386'
> + available_if: 'target_i386()'
>
> This way, we keep the possibility to have ifdef, and we can expose at
> runtime based on available_if. So we can keep the exact same schema we
> have today per target.
The name is ugly. Naming is hard. No need to worry about it right now.
Semantics of having both 'if' and 'available_if'? To work out an
answer, let's consider how to convert conditionals:
* 'if': STRING
If STRING is a target-specific macro, replace by 'available_if': PRED,
where PRED is the equivalent run-time predicate.
Else, no change.
* 'if': { 'all': [COND, ...] }
If COND contains only target-specific macros, replace by
'available_if': { 'all': [PRED, ...] }, where the PRED are the
equivalent run-time predicates.
If COND contains no target-specific macros, no change.
What if it contains both?
- If each COND contains either only target-specific macros, or no
target-specific macros, we could split the target-specific ones off
into an additional 'available_if'. This requires defining the
semantics of having both 'if' and 'available_if' as "both conditions
must be satisfied".
- What if this isn't the case?
* 'if' { 'any': [COND, ...] }
Similar, but to be able to split the COND we need "either condition
must be satisfied".
Even if we can make this work somehow, it would likely be a royal mess
to explain in qapi-code-gen.rst.
We currently don't have "mixed" conditionals. So we could sidestep the
problem: you can have either 'if' or 'available_if', but not both.
Feels like a cop out to me.
What if we move the "is dynamic" bit from the root of the conditional to
its leaves? So far, the leaves are macro names. What if we
additionally permit a function name?
Function name, not C expression, to not complicate generating code in
languages other than C too much.
Ignore the question of syntax for now, i.e. how to decide whether a leaf
is a macro or a function name.
>> Whatever we choose should support generating Rust and Go as well. Why?
>> Rust usage in QEMU is growing, and we'll likely need to generate some
>> Rust from the QAPI schema. Victor Toso has been working on Go bindings
>> for use in Go QMP client software.
>>
>
> I don't see any blocker with that. If you mention generating Rust and Go
> from qapi json definitions, it's already dependent on C preprocessor
> because of ifdef constant. So it will have to be adapted anyway.
> Having the same function (target_i386()) name through different
> languages is not something hard to achieve.
I can't see concrete blockers at this time. I just wanted to make you
aware of the emerging need to support other languages.
[...]
>> QAPI was designed to be compile-time static. Revising such fundamental
>> design assumptions is always frau
Re: [PATCH] [for-10.1] qapi/block-core: derpecate some block-job- APIs
Vladimir Sementsov-Ogievskiy writes: > For change, pause, resume, complete, dismiss and finalize actions > corresponding job- and block-job commands are almost equal. The > difference is in find_block_job_locked() vs find_job_locked() > functions. What's different? > > 1. find_block_job_locked() do check, is found job a block-job. This OK >when moving to more generic API, no needs to document this change. > > 2. find_block_job_locked() reports DeviceNotActive on failure, when >find_job_locked() reports GenericError. So, lets document this >difference in deprecated.txt. Still, for dismiss and finalize errors >are not documented at all, so be silent in deprecated.txt as well. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > > Hi all! > > That's a continuation of my "[RFC 00/15] block job API"[1], exactly, the > simplest part of it - deprecating block-job-* commands which simply > duplicate job-* ones. > > Note that the old series was started with trying to introduce job-change > command as substitution to both block-job-change (which only can change > mirror copy-mode parameter), and block-job-set-speed. It was rather > complicated and controversial attempt, which latest implemenation was > "[PATCH v3 0/7] introduce job-change qmp command"[2]. > > In [2] Kevin noted, that we'd better follow existing blockdev-reopen > approach of changing options (i.e. specify all options) than introduce a > new one. But, on the other hand, now I'm afraid, that rewriting in > third tools simple call to (old good) block-job-set-speed into > job-change(_all_options_ + changed speed) is too much work just for > "having nice interface". And too much for the only two options we want > to change. > > So, let's just start from something more obvious. Finally, > we can simple keep block-job-set-speed and block-job-change as is, > as they (unlike other block-job-* commands) are not duplicated by > similar job-* commands. > > [1] > https://patchew.org/QEMU/20240313150907.623462-1-vsement...@yandex-team.ru/ > [2] > https://patchew.org/QEMU/20241002140616.561652-1-vsement...@yandex-team.ru/ Thank you for your efforts at making the interfaces cleaner, simpler, and less redundant. > docs/about/deprecated.rst | 31 +++ > qapi/block-core.json | 30 ++ > 2 files changed, 61 insertions(+) > > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > index e2b4f077d4..eed3356359 100644 > --- a/docs/about/deprecated.rst > +++ b/docs/about/deprecated.rst > @@ -148,6 +148,37 @@ options are removed in favor of using explicit > ``blockdev-create`` and > ``blockdev-add`` calls. See :doc:`/interop/live-block-operations` for > details. > > +``block-job-pause`` (since 10.1) > +''' > + > +Use ``job-pause`` instead. The only difference is that ``job-pause`` > +always reports GenericError on failure when ``block-job-pause`` reports > +DeviceNotActive when block-job is not found. block-job-pause's doc comment: # The operation will pause as soon as possible. No event is emitted # when the operation is actually paused. [...] job-pause's: # The job will pause as soon as possible, which means transitioning # into the PAUSED state if it was RUNNING, or into STANDBY if it was # READY. The corresponding JOB_STATUS_CHANGE event will be emitted. Either one of them is incorrect about event emission, or there is another difference. > + > +``block-job-resume`` (since 10.1) > + > + > +Use ``job-resume`` instead. The only difference is that ``job-resume`` > +always reports GenericError on failure when ``block-job-resume`` reports > +DeviceNotActive when block-job is not found. block-job-resume's doc comment: # This command also clears the error status of the job. Nothing like that job-resume's. Either one of them is incorrect / incomplete about the error status clearance, or there is another difference. > + > +``block-job-complete`` (since 10.1) > +'' > + > +Use ``job-complete`` instead. The only difference is that ``job-complete`` > +always reports GenericError on failure when ``block-job-complete`` reports > +DeviceNotActive when block-job is not found. block-job-complete's doc comment: # Manually trigger completion of an active background block operation. # This is supported for drive mirroring, where it also switches the # device to write to the target path only. The ability to complete is # signaled with a BLOCK_JOB_READY event. # # This command completes an active background block operation # synchronously. The ordering of this command's return with the # BLOCK_JOB_COMPLETED event is not defined. Note that if an I/O error # occurs during the processing of this command: 1) the command itself # will fail; 2) the error will be processed according to the # rerror/werror arguments that we
Re: [PATCH] [for-10.1] qapi/block-core: derpecate some block-job- APIs
Vladimir Sementsov-Ogievskiy writes: > On 04.04.25 09:20, Markus Armbruster wrote: >> Vladimir Sementsov-Ogievskiy writes: [...] >>> + >>> +``block-job-finalize`` (since 10.1) >>> +'' >>> + >>> +Use ``job-finalize`` instead. >>> + >> >> block-job-finalize's doc comment: >> >> # Once a job that has manual=true reaches the pending state, it can be >> # instructed to finalize any graph changes and do any necessary >> # cleanup via this command. [...] >> >> There is no member @manual anywhere in the QAPI schema. I figure this >> should be @auto-finalize. >> >> job-finalize's doc comment: >> >> # Instructs all jobs in a transaction (or a single job if it is not >> # part of any transaction) to finalize any graph changes and do any >> # necessary cleanup. This command requires that all involved jobs are >> # in the PENDING state. >> >> Nothing on @auto-finalize. >> >> @auto-finalize defaults to true for jobs that support controlling it. >> These are exactly the ones that support @auto-dismiss. >> >> I figure @auto-dismiss is always false for the other jobs, but that >> doesn't seem to be documented anywhere. >> >> The only other bits related to @auto-dismiss and @auto-finalize seem to >> be two states in JobStatus: >> >> # @pending: The job has finished its work, but has finalization steps >> # that it needs to make prior to completing. These changes will >> # require manual intervention via @job-finalize if auto-finalize >> # was set to false. These pending changes may still fail. >> [...] >> # @concluded: The job has finished all work. If auto-dismiss was set >> # to false, the job will remain in the query list until it is >> # dismissed via @job-dismiss. >> >> >> Is it possible to observe @concluded via QMP when @auto-dismiss is on? > > Seems not. > >> >> What about @pending? > > Hmm probably, if we have a transaction of several jobs (actually only backups > may be joined to transactions), where some have auto-finalize and some not, > the whole transaction would be pending, including jobs that has > auto-finalize=true. Still, it's a strange case. So, auto-finalize=true is silently ignored when another job in the same transaction has auto-finalize=false? >> Aside: getting rid of auto-dismiss and auto-finalize some day would be >> nice. >> > > Hmm.. You mean, deprecated "true" value, and finally drop the fields, making > "false" the default? May be. May or may not be practical. > I'll resend, with additional patch to touch-up the documentation. Thanks!
Re: [RFC PATCH 0/3] single-binary: make QAPI generated files common
Daniel P. Berrangé writes:
> On Tue, Apr 29, 2025 at 09:43:24AM +0200, Markus Armbruster wrote:
>> Pierrick Bouvier writes:
>>
>> > After looking at the introspection code, I don't see any major blocker.
>> > We need to keep some of existing "if", as they are based on config-host,
>> > and should apply.
>> > We can introduce a new "available_if" (or any other name), which
>> > generates a runtime check when building the schema, or when serializing
>> > a struct.
>> >
>> > This way, by modifying the .json with:
>> > - if: 'TARGET_I386'
>> > + available_if: 'target_i386()'
>> >
>> > This way, we keep the possibility to have ifdef, and we can expose at
>> > runtime based on available_if. So we can keep the exact same schema we
>> > have today per target.
>>
>> The name is ugly. Naming is hard. No need to worry about it right now.
>>
>> Semantics of having both 'if' and 'available_if'? To work out an
>> answer, let's consider how to convert conditionals:
>>
>> * 'if': STRING
>>
>> If STRING is a target-specific macro, replace by 'available_if': PRED,
>> where PRED is the equivalent run-time predicate.
>>
>> Else, no change.
>>
>> * 'if': { 'all': [COND, ...] }
>>
>> If COND contains only target-specific macros, replace by
>> 'available_if': { 'all': [PRED, ...] }, where the PRED are the
>> equivalent run-time predicates.
>>
>> If COND contains no target-specific macros, no change.
>>
>> What if it contains both?
>>
>> - If each COND contains either only target-specific macros, or no
>> target-specific macros, we could split the target-specific ones off
>> into an additional 'available_if'. This requires defining the
>> semantics of having both 'if' and 'available_if' as "both conditions
>> must be satisfied".
>>
>> - What if this isn't the case?
>>
>> * 'if' { 'any': [COND, ...] }
>>
>> Similar, but to be able to split the COND we need "either condition
>> must be satisfied."
>>
>> Even if we can make this work somehow, it would likely be a royal mess
>> to explain in qapi-code-gen.rst.
>>
>> We currently don't have "mixed" conditionals. So we could sidestep the
>> problem: you can have either 'if' or 'available_if', but not both.
>> Feels like a cop out to me.
>>
>> What if we move the "is dynamic" bit from the root of the conditional to
>> its leaves? So far, the leaves are macro names. What if we
>> additionally permit a function name?
>>
>> Function name, not C expression, to not complicate generating code in
>> languages other than C too much.
>>
>> Ignore the question of syntax for now, i.e. how to decide whether a leaf
>> is a macro or a function name.
>
> I wonder if any of this is worth the pain in practice.
Fair!
Pierrick's stated goal is "no noticable differences". We've explored
what this means. We can also explore changes that would help us reach
the goal more easily, and maybe even consider setting a less ambitious
goal.
> Looking at the QAPI schema, we apply TARGET_ conditions either to
> commands, or to structs/enums that are used in args/return of commands.
> We don't conditionalize individual fields, etc.
With one exception noted by Pierrick.
> I tried to query our schema with 'jq' (incidentally rather tedious
> because of our JSON-but-not-JSON language[1]). If I select only
> commands we get:
>
> query-cpu-definitions => currently many arches
> query-cpu-model-expansion => currently many arches
> query-cpu-model-baseline => currently s390x only
> query-cpu-model-comparison => currently s390x only
> query-s390x-cpu-polarization => inherently s390x only
> query-gic-capabilities => inherently arm only
> query-sev => inherently x86 only
> query-sev-attestation-report => inherently x86 only
> query-sev-capabilities => inherently x86 only
> query-sev-launch-measure => inherently x86 only
> query-sgx => inherently x86 only
> query-sgx-capabilities => inherently x86 only
> rtc-reset-reinjection => inherently x86 only
> set-cpu-topology => inherently s390x only
> sev-inject-launch-secret => inherently x86 only
> xen-event-inject => currently x86 only
> xen-event-list => currently x86 only
>
> The two Xen commands are currently limited to x86, but if we ever extended
> Xen to arm, possibly they would make sense. IOW, conceptually a target
> conditional might be useful in future.
>
> The CPU model commands are the ones where having the target conditions
> visible in schema appears to add value, in that they'll allow a mgmt
> app to detect when we extend any of them to cover new architectures.
>
>
> Libvirt (and other mgmt apps) want to query the schema to see if commands
> exist in the QEMU they're using, before trying to invoke them. To some
> degree this is just a "nice to have" that improves error reporting/detection.
Schema introspection is more robust,
Re: [PATCH V2 1/5] qom: qom-tree-get
Markus Armbruster writes: > Steven Sistare writes: > >> On 7/4/2025 8:22 AM, Markus Armbruster wrote: >>> Steve Sistare writes: >>> Define the qom-tree-get QAPI command, which fetches an entire tree of properties and values with a single QAPI call. This is much faster than using qom-list plus qom-get for every node and property of the tree. See qom.json for details. Signed-off-by: Steve Sistare --- qapi/qom.json | 56 ++ qom/qom-qmp-cmds.c | 72 ++ 2 files changed, 128 insertions(+) diff --git a/qapi/qom.json b/qapi/qom.json index 28ce24c..94662ad 100644 --- a/qapi/qom.json +++ b/qapi/qom.json [...] ## +# @qom-tree-get: +# +# This command returns a tree of objects and their properties, +# rooted at the specified path. +# +# @path: The absolute or partial path within the object model, as +# described in @qom-get +# +# Errors: +# - If path is not valid or is ambiguous, returns an error. >>> >>> By convention, we use "If , , where is a >>> member of QapiErrorClass. >> >> OK. I was following the minimal Errors examples from this same file. > > Yup. I'll clean them up. I changed my mind. Omitting ", " is fairly common, actually. I don't feel like chasing down the actual error classes. Moreover, documenting error classes we don't want people to use seems counterproductive. Feel free to just delete ", returns an error." and call it a day. [...]
Re: [PATCH V2 4/5] qom: qom-list-getv
Steven Sistare writes:
> On 7/4/2025 8:22 AM, Markus Armbruster wrote:
>> Steve Sistare writes:
>>
>>> Define the qom-list-getv command, which fetches all the properties and
>>> values for a list of paths. This is faster than qom-tree-get when
>>> fetching a subset of the QOM tree. See qom.json for details.
>>>
>>> Signed-off-by: Steve Sistare
>>> ---
>>> qapi/qom.json | 34 ++
>>> qom/qom-qmp-cmds.c | 40
>>> 2 files changed, 74 insertions(+)
>>>
>>> diff --git a/qapi/qom.json b/qapi/qom.json
>>> index 94662ad..dc710d6 100644
>>> --- a/qapi/qom.json
>>> +++ b/qapi/qom.json
>>> @@ -62,6 +62,16 @@
>>> '*value': 'any' } }
>>>
>>> ##
>>> +# @ObjectPropertiesValues:
>>> +#
>>> +# @properties: a list of properties.
>>> +#
>>> +# Since 10.1
>>> +##
>>> +{ 'struct': 'ObjectPropertiesValues',
>>> + 'data': { 'properties': [ 'ObjectPropertyValue' ] }}
>>> +
>>> +##
>>> # @ObjectNode:
>>> #
>>> # @name: the name of the node
>>> @@ -158,6 +168,30 @@
>>> 'allow-preconfig': true }
>>>
>>> ##
>>> +# @qom-list-getv:
>>> +#
>>> +# This command returns a list of properties and their values for
>>> +# each object path in the input list.
>>
>> Imperative mood, please: "Return a list of ..."
>
> OK. (I followed the style of qom-get and qom-list).
Yup. We have a few more elsewhere. I'll clean them up.
[...]
Re: [PATCH V2 1/5] qom: qom-tree-get
Steven Sistare writes:
> On 7/4/2025 8:22 AM, Markus Armbruster wrote:
>> Steve Sistare writes:
>>
>>> Define the qom-tree-get QAPI command, which fetches an entire tree of
>>> properties and values with a single QAPI call. This is much faster
>>> than using qom-list plus qom-get for every node and property of the
>>> tree. See qom.json for details.
>>>
>>> Signed-off-by: Steve Sistare
>>> ---
>>> qapi/qom.json | 56 ++
>>> qom/qom-qmp-cmds.c | 72
>>> ++
>>> 2 files changed, 128 insertions(+)
>>>
>>> diff --git a/qapi/qom.json b/qapi/qom.json
>>> index 28ce24c..94662ad 100644
>>> --- a/qapi/qom.json
>>> +++ b/qapi/qom.json
>>> @@ -46,6 +46,38 @@
>>> '*default-value': 'any' } }
>>>
>>> ##
>>> +# @ObjectPropertyValue:
>>> +#
>>> +# @name: the name of the property
>>> +#
>>> +# @type: the type of the property, as described in @ObjectPropertyInfo
>>
>> That description is crap. In part because what it tries to describe is
>> crap. Neither is this patch's problem.
>>
>>> +#
>>> +# @value: the value of the property. Omitted if cannot be read.
>>
>> Suggest "Absent when the property cannot be read."
>
> OK.
>
>>> +#
>>> +# Since 10.1
>>> +##
>>> +{ 'struct': 'ObjectPropertyValue',
>>> + 'data': { 'name': 'str',
>>> +'type': 'str',
>>> +'*value': 'any' } }
>>
>> ObjectPropertyValue suggests this describes a property's value.
>
> I would agree with you if the name included "info" or "desc", but it
> does not. To me, "ObjectPropertyValue" says this is an object's
> property and value. But it's subjective.
Naming is hard.
> Perhaps: ObjectPropertyWithValue
I'd be tempted by ObjectProperty if it wasn't already taken by
qom/object.h.
Let's converge on the code, and maybe revisit naming at the end.
>> It does
>> not. It includes the name, i.e. it describes the *property*.
>>
>> So does ObjectPropertyInfo.
>>
>> The two overlap: both habe name and type. Only ObjectPropertyValue has
>> the current value. Only ObjectPropertyInfo has the default value and
>> description (I suspect the latter is useless in practice).
>>
>> ObjectPropertyInfo is used with qom-list and qom-list-properties.
>>
>> qom-list takes a QOM path, like your qom-tree-get and qom-list-getv.
>> I'd expect your commands to supersede qom-list in practice.
>>
>> qom-list-properties is unlike your qom-tree-get and qom-list-getv: it
>> takes a type name. It's unreliable for non-abstract types: it can miss
>> dynamically created properties.
>>
>> Let's ignore all this for now.
>>
>>> +
>>> +##
>>> +# @ObjectNode:
>>> +#
>>> +# @name: the name of the node
>>> +#
>>> +# @children: child nodes
>>> +#
>>> +# @properties: properties of the node
>>> +#
>>> +# Since 10.1
>>> +##
>>> +{ 'struct': 'ObjectNode',
>>> + 'data': { 'name': 'str',
>>> +'children': [ 'ObjectNode' ],
>>> +'properties': [ 'ObjectPropertyValue' ] }}
>>> +
>>> +##
>>> # @qom-list:
>>> #
>>> # This command will list any properties of a object given a path in
>>> @@ -126,6 +158,30 @@
>>> 'allow-preconfig': true }
>>>
>>> ##
>>> +# @qom-tree-get:
>>> +#
>>> +# This command returns a tree of objects and their properties,
>>> +# rooted at the specified path.
>>> +#
>>> +# @path: The absolute or partial path within the object model, as
>>> +# described in @qom-get
>>> +#
>>> +# Errors:
>>> +# - If path is not valid or is ambiguous, returns an error.
>>
>> By convention, we use "If , , where is a
>> member of QapiErrorClass.
>
> OK. I was following the minimal Errors examples from this same file.
Yup. I'll clean them up.
>> What are the possible error classes? As far as I can tell:
>>
>> - If path is ambiguous, GenericError
>> - If path cannot be resolved, DeviceNotFound
>>
>> However, use of error classes other than GenericError is strongly
>> discouraged (see error_set() in qapi/error.h).
>>
>> Is the ability to distinguish between these two errors useful?
>>
>> Existing related commands such as qom-get also use DeviceNotFound.
>> Entirely undocumented, exact error conditions unclear. Awesome.
>>
>> Libvirt seems to rely on this undocumented behavior: I can see code
>> checking for DeviceNotFound. Hyrum's law strikes.
>>
>> qom-get fails with DeviceNotFound in both of the above cases. It fails
>> with GenericError when @property doesn't exist or cannot be read. Your
>> qom-tree-get fails differently. Awesome again.
>>
>> Choices:
>>
>> 1. Leave errors undocumented and inconsistent.
>>
>> 2. Document errors for all related commands. Make the new ones as
>> consistent as we can.
>
> Ignoring qom-tree-get since we are dropping it.
>
> Do you prefer that qom-list-getv be consistent with qom-list (GenericError
> and DeviceNotFound, as created by the common subroutine qom_resolve_path),
> or only return GenericError with a customized message per be
Re: [PATCH v2 09/10] net: Add passt network backend
Laurent Vivier writes:
> On 24/06/2025 14:03, Daniel P. Berrangé wrote:
>> On Tue, Jun 24, 2025 at 01:55:20PM +0200, Markus Armbruster wrote:
>>> Laurent Vivier writes:
>>>
On 24/06/2025 10:16, Markus Armbruster wrote:
> Laurent Vivier writes:
>
>> This commit introduces support for passt as a new network backend.
>> passt is an unprivileged, user-mode networking solution that provides
>> connectivity for virtual machines by launching an external helper
>> process.
>>
>> The implementation reuses the generic stream data handling logic. It
>> launches the passt binary using GSubprocess, passing it a file
>> descriptor from a socketpair() for communication. QEMU connects to
>> the other end of the socket pair to establish the network data stream.
>>
>> The PID of the passt daemon is tracked via a temporary file to
>> ensure it is terminated when QEMU exits.
>>
>> Signed-off-by: Laurent Vivier
>
> [...]
>
>> diff --git a/qapi/net.json b/qapi/net.json
>> index 97ea1839813b..76d7654414f7 100644
>> --- a/qapi/net.json
>> +++ b/qapi/net.json
>> @@ -112,6 +112,125 @@
>> 'data': {
>>'str': 'str' } }
>>+##
>> +# @NetDevPasstOptions:
>> +#
>> +# Unprivileged user-mode network connectivity using passt
>> +#
>> +# @path: path to passt binary
>
> I'd prefer a more descriptive name.
>
> Elsewhere in this file, we refer to programs like this:
>
> # @script: script to initialize the interface
> #
> # @downscript: script to shut down the interface
>
> passt isn't a script, of course.
>
> I don't know, perhaps
>
> # @passt-filename: the passt program to run.
>
> or even
>
> # @passt: Filename of the passt program to run.
>
>> +#
>> +# @quiet: don't print informational messages
>
> What does the printing? A peek at the code I snipped suggests this flag
> is passed to the passt binary as --quiet. Correct?
>
>> +#
>> +# @debug: be verbose
>> +#
>> +# @trace: extra verbose
>
> Likewise for these two.
>
>> +#
>> +# @vhost-user: enable vhost-user
>>>
>>> [...]
>>>
>> +# @udp-ports: UDP ports to forward
>
> Is there anything in this struct that configures qemu-system-FOO itself,
> i.e. isn't just passed to passt?
>
Yes, all parameters are just passed to passt.
Do you think it's better not to add all these parameters to netdev backend
but only one
generic containing the passt command line parameters?
>>>
>>> I'm not sure.
>>>
>>> Thoughts from libvirt's perspective?
>>
>> We already have passt support in libvirt that leverages the existing
>> vhost-user netdev backend to connect up QEMU.
>>
>> I see this backend requires QEMU to be able to spawn the passt binary
>> itselfm, which is not something libvirt would allow via our security
>> confinement of QEMU. So that would rule out our ability to consume
>> this netdev backend, as currently written
>>
>> Is there anything QEMU can do with this passt netdev, that can't be
>> done via the vhost-user backend ? ie is this merely syntax sugar to
>> make it easier for humans launching QEMU, or is there some feature
>> / performance benefit ?
>
> The idea is only to allow user to run directly QEMU with passt in the same
> way it's done with the netdev user. There is no other benefit than the easier
> interface for humans.
>
> For instance, we want to run '-nic passt' as we can run '-nic user'.
Good to know! Put it into the commit message, please.
Hmm, it's good to know for management application developers, too. So
we should also put it into documentation, I guess. Where? Looking...
Oh, there's a section "Using passt as the user mode network stack"in
docs/system/devices/net.rst. That section clearly needs an update for
the new backend.
Back to the design question how to pass configuration via qemu-system to
the passt program with this new backend.
Your patch replicates passt command line options as optional members of
QAPI type NetDevPasstOptions. Any passt options not covered are
inaccessible.
Or rather inaccessible via QMP / HMP / CLI. You can still access them
by pointing @passt to a wrapper script.
This leads us to a possible alternative: make such a wrapper script the
*only* way to configure passt. This is like NetdevTapOptions @script
and @downscript. Mind, I'm not telling you it's a good idea, I'm merely
trying to map the solution space!
Instead of trying to make NetDevPasstOptions complete (so you need to
fall back to a wrapper script only when using a version of passt with
different options), we can limit it to a curated set of options. We'd
need to decide which ones :)
You pointed out yet another alternative: pass through the passt command
line directly. Two obvious ways: a single shell command str
Re: [PATCH V2 1/5] qom: qom-tree-get
Steve Sistare writes:
> Define the qom-tree-get QAPI command, which fetches an entire tree of
> properties and values with a single QAPI call. This is much faster
> than using qom-list plus qom-get for every node and property of the
> tree. See qom.json for details.
>
> Signed-off-by: Steve Sistare
> ---
> qapi/qom.json | 56 ++
> qom/qom-qmp-cmds.c | 72
> ++
> 2 files changed, 128 insertions(+)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 28ce24c..94662ad 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -46,6 +46,38 @@
> '*default-value': 'any' } }
>
> ##
> +# @ObjectPropertyValue:
> +#
> +# @name: the name of the property
> +#
> +# @type: the type of the property, as described in @ObjectPropertyInfo
That description is crap. In part because what it tries to describe is
crap. Neither is this patch's problem.
> +#
> +# @value: the value of the property. Omitted if cannot be read.
Suggest "Absent when the property cannot be read."
> +#
> +# Since 10.1
> +##
> +{ 'struct': 'ObjectPropertyValue',
> + 'data': { 'name': 'str',
> +'type': 'str',
> +'*value': 'any' } }
ObjectPropertyValue suggests this describes a property's value. It does
not. It includes the name, i.e. it describes the *property*.
So does ObjectPropertyInfo.
The two overlap: both habe name and type. Only ObjectPropertyValue has
the current value. Only ObjectPropertyInfo has the default value and
description (I suspect the latter is useless in practice).
ObjectPropertyInfo is used with qom-list and qom-list-properties.
qom-list takes a QOM path, like your qom-tree-get and qom-list-getv.
I'd expect your commands to supersede qom-list in practice.
qom-list-properties is unlike your qom-tree-get and qom-list-getv: it
takes a type name. It's unreliable for non-abstract types: it can miss
dynamically created properties.
Let's ignore all this for now.
> +
> +##
> +# @ObjectNode:
> +#
> +# @name: the name of the node
> +#
> +# @children: child nodes
> +#
> +# @properties: properties of the node
> +#
> +# Since 10.1
> +##
> +{ 'struct': 'ObjectNode',
> + 'data': { 'name': 'str',
> +'children': [ 'ObjectNode' ],
> +'properties': [ 'ObjectPropertyValue' ] }}
> +
> +##
> # @qom-list:
> #
> # This command will list any properties of a object given a path in
> @@ -126,6 +158,30 @@
>'allow-preconfig': true }
>
> ##
> +# @qom-tree-get:
> +#
> +# This command returns a tree of objects and their properties,
> +# rooted at the specified path.
> +#
> +# @path: The absolute or partial path within the object model, as
> +# described in @qom-get
> +#
> +# Errors:
> +# - If path is not valid or is ambiguous, returns an error.
By convention, we use "If , , where is a
member of QapiErrorClass.
What are the possible error classes? As far as I can tell:
- If path is ambiguous, GenericError
- If path cannot be resolved, DeviceNotFound
However, use of error classes other than GenericError is strongly
discouraged (see error_set() in qapi/error.h).
Is the ability to distinguish between these two errors useful?
Existing related commands such as qom-get also use DeviceNotFound.
Entirely undocumented, exact error conditions unclear. Awesome.
Libvirt seems to rely on this undocumented behavior: I can see code
checking for DeviceNotFound. Hyrum's law strikes.
qom-get fails with DeviceNotFound in both of the above cases. It fails
with GenericError when @property doesn't exist or cannot be read. Your
qom-tree-get fails differently. Awesome again.
Choices:
1. Leave errors undocumented and inconsistent.
2. Document errors for all related commands. Make the new ones as
consistent as we can.
> +# - If a property cannot be read, the value field is omitted in
> +# the corresponding @ObjectPropertyValue.
This is not an error, and therefore doesn't belong here.
ObjectPropertyValue's documentation also mentions it. Good enough?
> +#
> +# Returns: A tree of @ObjectNode. Each node contains its name, list
> +# of properties, and list of child nodes.
Hmm.
A struct Object has no name. Only properties have a name.
An ObjectNode has a name, and an ObjectPropertyValue has a name.
I may get back to this in a later message.
> +#
> +# Since 10.1
> +##
> +{ 'command': 'qom-tree-get',
> + 'data': { 'path': 'str' },
> + 'returns': 'ObjectNode',
> + 'allow-preconfig': true }
> +
> +##
> # @qom-set:
> #
> # This command will set a property from a object model path.
> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> index 293755f..b876681 100644
> --- a/qom/qom-qmp-cmds.c
> +++ b/qom/qom-qmp-cmds.c
> @@ -69,6 +69,78 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path,
> Error **errp)
> return props;
> }
>
> +static void qom_list_add_property_value(Object *obj, ObjectProperty *prop,
> +
Re: [PATCH V2 4/5] qom: qom-list-getv
Steve Sistare writes:
> Define the qom-list-getv command, which fetches all the properties and
> values for a list of paths. This is faster than qom-tree-get when
> fetching a subset of the QOM tree. See qom.json for details.
>
> Signed-off-by: Steve Sistare
> ---
> qapi/qom.json | 34 ++
> qom/qom-qmp-cmds.c | 40
> 2 files changed, 74 insertions(+)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 94662ad..dc710d6 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -62,6 +62,16 @@
> '*value': 'any' } }
>
> ##
> +# @ObjectPropertiesValues:
> +#
> +# @properties: a list of properties.
> +#
> +# Since 10.1
> +##
> +{ 'struct': 'ObjectPropertiesValues',
> + 'data': { 'properties': [ 'ObjectPropertyValue' ] }}
> +
> +##
> # @ObjectNode:
> #
> # @name: the name of the node
> @@ -158,6 +168,30 @@
>'allow-preconfig': true }
>
> ##
> +# @qom-list-getv:
> +#
> +# This command returns a list of properties and their values for
> +# each object path in the input list.
Imperative mood, please: "Return a list of ..."
> +#
> +# @paths: The absolute or partial path for each object, as described
> +# in @qom-get
> +#
> +# Errors:
> +# - If any path is not valid or is ambiguous, returns an error.
> +# - If a property cannot be read, the value field is omitted in
> +# the corresponding @ObjectPropertyValue.
My comment on qom-tree-get's Errors: section applies.
> +#
> +# Returns: A list of @ObjectPropertiesValues. Each element contains
> +# the properties of the corresponding element in @paths.
Again, ObjectPropertiesValues is an unfortunate name.
> +#
> +# Since 10.1
> +##
> +{ 'command': 'qom-list-getv',
> + 'data': { 'paths': [ 'str' ] },
> + 'returns': [ 'ObjectPropertiesValues' ],
> + 'allow-preconfig': true }
> +
> +##
> # @qom-tree-get:
> #
> # This command returns a tree of objects and their properties,
I find this command *much* simpler than qom-tree-get.
qom-list-getv treats all properties the same. References, whether they
are children and links, are the same: a QOM path.
qom-tree-get separates properties into children and non-children.
Children become nested ObjectNodes, links remain QOM paths.
> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> index b876681..1f05956 100644
> --- a/qom/qom-qmp-cmds.c
> +++ b/qom/qom-qmp-cmds.c
> @@ -90,6 +90,46 @@ static void qom_list_add_property_value(Object *obj,
> ObjectProperty *prop,
> }
> }
>
> +static ObjectPropertyValueList *qom_get_property_value_list(const char *path,
> +Error **errp)
> +{
> +Object *obj;
> +ObjectProperty *prop;
> +ObjectPropertyIterator iter;
> +ObjectPropertyValueList *props = NULL;
> +
> +obj = qom_resolve_path(path, errp);
> +if (obj == NULL) {
> +return NULL;
> +}
> +
> +object_property_iter_init(&iter, obj);
> +while ((prop = object_property_iter_next(&iter))) {
> +qom_list_add_property_value(obj, prop, &props);
> +}
> +
> +return props;
> +}
> +
> +ObjectPropertiesValuesList *qmp_qom_list_getv(strList *paths, Error **errp)
> +{
> +ObjectPropertiesValuesList *head = NULL, **tail = &head;
> +
> +for ( ; paths ; paths = paths->next) {
I'd prefer a separate variable:
for (tail = paths; tail; tail = tail->next) {
> +ObjectPropertiesValues *item = g_new0(ObjectPropertiesValues, 1);
> +
> +QAPI_LIST_APPEND(tail, item);
> +
> +item->properties = qom_get_property_value_list(paths->value, errp);
> +if (!item->properties) {
> +qapi_free_ObjectPropertiesValuesList(head);
> +return NULL;
> +}
> +}
> +
> +return head;
> +}
> +
> static ObjectNode *qom_tree_get(const char *path, Error **errp)
> {
> Object *obj;
The implementation is simpler than qom-tree's, too.
Re: [PATCH V2 0/5] fast qom tree get
Steve Sistare writes: > Using qom-list and qom-get to get all the nodes and property values in a > QOM tree can take multiple seconds because it requires 1000's of individual > QOM requests. Some managers fetch the entire tree or a large subset > of it when starting a new VM, and this cost is a substantial fraction of > start up time. > > To reduce this cost, consider QAPI calls that fetch more information in > each call: > * qom-list-get: given a path, return a list of properties and values. > * qom-list-getv: given a list of paths, return a list of properties and > values for each path. > * qom-tree-get: given a path, return all descendant nodes rooted at that > path, with properties and values for each. > > In all cases, a returned property is represented by ObjectPropertyValue, > with fields name, type, and value. If an error occurs when reading a value > the value field is omitted. Thus an error for one property will not cause a > bulk fetch operation to fail. > > To evaluate each method, I modified scripts/qmp/qom-tree to use the method, > verified all methods produce the same output, and timed each using: > > qemu-system-x86_64 -display none \ > -chardev socket,id=monitor0,path=/tmp/vm1.sock,server=on,wait=off \ > -mon monitor0,mode=control & > > time qom-tree -s /tmp/vm1.sock > /dev/null > > I only measured once per method, but the variation is low after a warm up run. > The 'real - user - sys' column is a proxy for QEMU CPU time. > > method real(s) user(s) sys(s) (real - user - sys)(s) > qom-list / qom-get 2.048 0.932 0.057 1.059 > qom-list-get 0.402 0.230 0.029 0.143 > qom-list-getv0.200 0.132 0.015 0.053 > qom-tree-get 0.143 0.123 0.012 0.008 > > qom-tree-get is the clear winner, reducing elapsed time by a factor of 14X, > and reducing QEMU CPU time by 132X. > > qom-list-getv is slower when fetching the entire tree, but can beat > qom-tree-get when only a subset of the tree needs to be fetched (not shown). > qom-list-get is shown for comparison only, and is not included in this series. How badly do you need the additional performance qom-tree-get can give you in certain cases? I'm asking because I find qom-list-getv *much* simpler.
Re: [PATCH v2 18/24] qapi/migration: Deprecate capabilities commands
Fabiano Rosas writes:
> The concept of capabilities is being merged into the concept of
> parameters. From now on, the commands that handle capabilities are
> deprecated in favor of the commands that handle parameters.
>
> Affected commands:
>
> - migrate-set-capabilities
> - query-migrate-capabilities
>
> Signed-off-by: Fabiano Rosas
> ---
> docs/about/deprecated.rst | 12
> migration/migration-hmp-cmds.c | 6 ++
> qapi/migration.json| 16 ++--
> 3 files changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 42037131de..15474833ea 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -605,3 +605,15 @@ command documentation for details on the ``fdset`` usage.
>
> The ``zero-blocks`` capability was part of the block migration which
> doesn't exist anymore since it was removed in QEMU v9.1.
> +
> +``migrate-set-capabilities`` command (since 10.1)
> +'
> +
> +This command was deprecated. Use ``migrate-set-parameters`` instead
> +which now supports setting capabilities.
> +
> +``query-migrate-capabilities`` command (since 10.1)
> +'''
> +
> +This command was deprecated. Use ``query-migrate-parameters`` instead
> +which now supports querying capabilities.
Scratch "This command was deprecated."
Could also scratch "which now supports..." Up to you.
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index 8615340a6b..7f234d5aa8 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -229,6 +229,9 @@ void hmp_info_migrate_capabilities(Monitor *mon, const
> QDict *qdict)
> {
> MigrationCapabilityStatusList *caps, *cap;
>
> +warn_report("info migrate_capabilities is deprecated;"
> +" use info migrate_parameters instead");
> +
> caps = qmp_query_migrate_capabilities(NULL);
>
> if (caps) {
> @@ -616,6 +619,9 @@ void hmp_migrate_set_capability(Monitor *mon, const QDict
> *qdict)
> MigrationCapabilityStatus *value;
> int val;
>
> +warn_report("migrate_set_capability is deprecated;"
> +" use migrate_set_parameter instead");
> +
> val = qapi_enum_parse(&MigrationCapability_lookup, cap, -1, &err);
> if (val < 0) {
> goto end;
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 3d3f5624c5..c5e6ea1a2d 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -521,6 +521,11 @@
> #
> # @capabilities: json array of capability modifications to make
> #
> +# Features:
> +#
> +# @deprecated: This command is deprecated in favor of
> +# migrate-set-parameters.
For consistency with existing deprecation notes:
# @deprecated: This command is deprecated. Use migrate-set-parameters
# instead.
> +#
> # Since: 1.2
> #
> # .. qmp-example::
> @@ -530,7 +535,8 @@
> # <- { "return": {} }
> ##
> { 'command': 'migrate-set-capabilities',
> - 'data': { 'capabilities': ['MigrationCapabilityStatus'] } }
> + 'data': { 'capabilities': ['MigrationCapabilityStatus'] },
> + 'features': ['deprecated'] }
>
> ##
> # @query-migrate-capabilities:
> @@ -539,6 +545,11 @@
> #
> # Returns: @MigrationCapabilityStatus
> #
> +# Features:
> +#
> +# @deprecated: This command is deprecated in favor of
> +# query-migrate-parameters.
Likewise.
> +#
> # Since: 1.2
> #
> # .. qmp-example::
> @@ -554,7 +565,8 @@
> # {"state": false, "capability": "x-colo"}
> #]}
> ##
> -{ 'command': 'query-migrate-capabilities', 'returns':
> ['MigrationCapabilityStatus']}
> +{ 'command': 'query-migrate-capabilities', 'returns':
> ['MigrationCapabilityStatus'],
> + 'features': ['deprecated'] }
>
> ##
> # @MultiFDCompression:
With that:
Reviewed-by: Markus Armbruster
Re: [PATCH 2/4] qapi/migration: Deprecate migrate argument @detach
Peter Xu writes:
> On Wed, May 21, 2025 at 08:37:09AM +0200, Markus Armbruster wrote:
>> Argument @detach has always been ignored. Start the clock to get rid
>> of it.
>>
>> Cc: Peter Xu
>> Cc: Fabiano Rosas
>> Signed-off-by: Markus Armbruster
>> ---
>> docs/about/deprecated.rst | 5 +
>> qapi/migration.json | 18 +-
>> 2 files changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
>> index 9665bc6fcf..ef4ea84e69 100644
>> --- a/docs/about/deprecated.rst
>> +++ b/docs/about/deprecated.rst
>> @@ -179,6 +179,11 @@ Use ``job-dismiss`` instead.
>>
>> Use ``job-finalize`` instead.
>>
>> +``migrate`` argument ``detach`` (since 10.1)
>> +
>> +
>> +This argument has always been ignored.
>> +
>> ``query-migrationthreads`` (since 9.2)
>> ''
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index 8b9c53595c..ecd266f98e 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1660,6 +1660,10 @@
>> #
>> # @resume: resume one paused migration, default "off". (since 3.0)
>> #
>> +# Features:
>> +#
>> +# @deprecated: Argument @detach is deprecated.
>> +#
>> # Since: 0.14
>> #
>> # .. admonition:: Notes
>> @@ -1668,19 +1672,14 @@
>> #migration's progress and final result (this information is
>> #provided by the 'status' member).
>> #
>> -# 2. All boolean arguments default to false.
>
> There's one more boolean ("resume") exists, but probably not a huge
> deal.. All booleans if not mentioned should have a default-false semantics
> at least to me.
Its default remains documented. It's visible above :)
> Reviewed-by: Peter Xu
Thanks!
Re: [PATCH v2 09/10] net: Add passt network backend
Laurent Vivier writes:
> On 24/06/2025 10:16, Markus Armbruster wrote:
>> Laurent Vivier writes:
>>
>>> This commit introduces support for passt as a new network backend.
>>> passt is an unprivileged, user-mode networking solution that provides
>>> connectivity for virtual machines by launching an external helper process.
>>>
>>> The implementation reuses the generic stream data handling logic. It
>>> launches the passt binary using GSubprocess, passing it a file
>>> descriptor from a socketpair() for communication. QEMU connects to
>>> the other end of the socket pair to establish the network data stream.
>>>
>>> The PID of the passt daemon is tracked via a temporary file to
>>> ensure it is terminated when QEMU exits.
>>>
>>> Signed-off-by: Laurent Vivier
>>
>> [...]
>>
>>> diff --git a/qapi/net.json b/qapi/net.json
>>> index 97ea1839813b..76d7654414f7 100644
>>> --- a/qapi/net.json
>>> +++ b/qapi/net.json
>>> @@ -112,6 +112,125 @@
>>> 'data': {
>>> 'str': 'str' } }
>>>
>>> +##
>>> +# @NetDevPasstOptions:
>>> +#
>>> +# Unprivileged user-mode network connectivity using passt
>>> +#
>>> +# @path: path to passt binary
>>
>> I'd prefer a more descriptive name.
>>
>> Elsewhere in this file, we refer to programs like this:
>>
>> # @script: script to initialize the interface
>> #
>> # @downscript: script to shut down the interface
>>
>> passt isn't a script, of course.
>>
>> I don't know, perhaps
>>
>> # @passt-filename: the passt program to run.
>>
>> or even
>>
>> # @passt: Filename of the passt program to run.
>>
>>> +#
>>> +# @quiet: don't print informational messages
>>
>> What does the printing? A peek at the code I snipped suggests this flag
>> is passed to the passt binary as --quiet. Correct?
>>
>>> +#
>>> +# @debug: be verbose
>>> +#
>>> +# @trace: extra verbose
>>
>> Likewise for these two.
>>
>>> +#
>>> +# @vhost-user: enable vhost-user
[...]
>>> +# @udp-ports: UDP ports to forward
>>
>> Is there anything in this struct that configures qemu-system-FOO itself,
>> i.e. isn't just passed to passt?
>>
>
> Yes, all parameters are just passed to passt.
>
> Do you think it's better not to add all these parameters to netdev backend
> but only one
> generic containing the passt command line parameters?
I'm not sure.
Thoughts from libvirt's perspective?
Re: [RFC PATCH 0/3] single-binary: make QAPI generated files common
Pierrick Bouvier writes: [...] > I don't think we should think too much ahead for languages other than C, > for one, two, and even three reasons :) I agree that thinking ahead too much is a bad habit. So is thinking ahead too little :) > - First, it's already broken because we rely on ifdef that won't be > there in Rust or Go. I don't think it's broken. QAPI 'if' translates straightforwardly to C #if, but that doesn't mean it cannot be translated to conditional compilation / metaprogramming in other languages. In fact, the value of 'if' used to be C constant expressions suitable for use with #if, and we changed it to its current form specifically to enable Rust work, in merge commit c83fcfaf8a5. Marc-André's was trying to develop Rust bindings back then, and if I remember correctly this change was enough to let him implement 'if' with Rust. > - Second, it's code, we can just change it later if needed. True! > - Third, those json are consumed only by QEMU (right?), so we are free > to write/modify them as we want. Also true. > The only thing that must stay the same is what we expose to the consumer > in the schema, and which commands we expose in qemu. We may evolve the external interface as long as we honor our compatibility promise. You're aiming for "no change at all" there. I understand why that's desirable. But if it should turn out that a bit of compatible change simplifies the job, we can take the simpler route. [...]
Re: [PATCH 2/4] qapi/migration: Deprecate migrate argument @detach
Peter Krempa writes: > On Wed, May 21, 2025 at 09:46:10 +0200, Peter Krempa via Devel wrote: >> On Wed, May 21, 2025 at 08:37:09 +0200, Markus Armbruster via Devel wrote: >> > Argument @detach has always been ignored. Start the clock to get rid >> > of it. >> > >> > Cc: Peter Xu >> > Cc: Fabiano Rosas >> > Signed-off-by: Markus Armbruster >> > --- >> > docs/about/deprecated.rst | 5 + >> > qapi/migration.json | 18 +- >> > 2 files changed, 14 insertions(+), 9 deletions(-) >> > >> > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst >> > index 9665bc6fcf..ef4ea84e69 100644 >> > --- a/docs/about/deprecated.rst >> > +++ b/docs/about/deprecated.rst >> > @@ -179,6 +179,11 @@ Use ``job-dismiss`` instead. >> > >> > Use ``job-finalize`` instead. >> > >> > +``migrate`` argument ``detach`` (since 10.1) >> > +'''''''''''''''''''''''''''''''''''''''''''' >> > + >> > +This argument has always been ignored. >> >> Huh; libvirt is actually always specifying it for some reason. >> >> I'll post patches shortly getting rid of it completely in libvirt > > Patch 2/2 of > > https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/VPMPVPG5DSFORQODG4JGMZ2MTOJDQPPF/ > > addresses the above. I noticed that 'dump-guest-memory' does also have > 'detach' option but that one at least has effect, even when it's not > really useful (blocking monitor if not used). Another long-running task that should be a job... > On behalf of libvirt: > > Series: > > ACKed-by: Peter Krempa Thanks!
[PATCH 4/4] docs/about/removed-features: Move removal notes to tidy up order
The removal notes within a section are mostly in version order. Move the few that aren't so they are. Signed-off-by: Markus Armbruster --- docs/about/removed-features.rst | 60 - 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst index 92b5ba6218..4819cb4665 100644 --- a/docs/about/removed-features.rst +++ b/docs/about/removed-features.rst @@ -162,6 +162,12 @@ specified with ``-mem-path`` can actually provide the guest RAM configured with The ``name`` parameter of the ``-net`` option was a synonym for the ``id`` parameter, which should now be used instead. +RISC-V firmware not booted by default (removed in 5.1) +'' + +QEMU 5.1 changes the default behaviour from ``-bios none`` to ``-bios default`` +for the RISC-V ``virt`` machine and ``sifive_u`` machine. + ``-numa node,mem=...`` (removed in 5.1) ''' @@ -324,12 +330,6 @@ devices. Drives the board doesn't pick up can no longer be used with This option was undocumented and not used in the field. Use ``-device usb-ccid`` instead. -RISC-V firmware not booted by default (removed in 5.1) -'' - -QEMU 5.1 changes the default behaviour from ``-bios none`` to ``-bios default`` -for the RISC-V ``virt`` machine and ``sifive_u`` machine. - ``-no-quit`` (removed in 7.0) ' @@ -911,14 +911,6 @@ The RISC-V no MMU cpus have been removed. The two CPUs: ``rv32imacu-nommu`` and ``rv64imacu-nommu`` can no longer be used. Instead the MMU status can be specified via the CPU ``mmu`` option when using the ``rv32`` or ``rv64`` CPUs. -RISC-V 'any' CPU type ``-cpu any`` (removed in 9.2) -''' - -The 'any' CPU type was introduced back in 2018 and was around since the -initial RISC-V QEMU port. Its usage was always been unclear: users don't know -what to expect from a CPU called 'any', and in fact the CPU does not do anything -special that isn't already done by the default CPUs rv32/rv64. - ``compat`` property of server class POWER CPUs (removed in 6.0) ''' @@ -965,6 +957,14 @@ The CRIS architecture was pulled from Linux in 4.17 and the compiler was no longer packaged in any distro making it harder to run the ``check-tcg`` tests. +RISC-V 'any' CPU type ``-cpu any`` (removed in 9.2) +''' + +The 'any' CPU type was introduced back in 2018 and was around since the +initial RISC-V QEMU port. Its usage was always been unclear: users don't know +what to expect from a CPU called 'any', and in fact the CPU does not do anything +special that isn't already done by the default CPUs rv32/rv64. + System accelerators --- @@ -975,18 +975,18 @@ Userspace local APIC with KVM (x86, removed in 8.0) a local APIC. The ``split`` setting is supported, as is using ``-M kernel-irqchip=off`` when the CPU does not have a local APIC. -HAXM (``-accel hax``) (removed in 8.2) -'' - -The HAXM project has been retired (see https://github.com/intel/haxm#status). -Use "whpx" (on Windows) or "hvf" (on macOS) instead. - MIPS "Trap-and-Emulate" KVM support (removed in 8.0) The MIPS "Trap-and-Emulate" KVM host and guest support was removed from Linux in 2021, and is not supported anymore by QEMU either. +HAXM (``-accel hax``) (removed in 8.2) +'' + +The HAXM project has been retired (see https://github.com/intel/haxm#status). +Use "whpx" (on Windows) or "hvf" (on macOS) instead. + System emulator machines @@ -1044,16 +1044,6 @@ Aspeed ``swift-bmc`` machine (removed in 7.0) This machine was removed because it was unused. Alternative AST2500 based OpenPOWER machines are ``witherspoon-bmc`` and ``romulus-bmc``. -Aspeed ``tacoma-bmc`` machine (removed in 10.0) -''' - -The ``tacoma-bmc`` machine was removed because it didn't bring much -compared to the ``rainier-bmc`` machine. Also, the ``tacoma-bmc`` was -a board used for bring up of the AST2600 SoC that never left the -labs. It can be easily replaced by the ``rainier-bmc`` machine, which -was the actual final product, or by the ``ast2600-evb`` with some -tweaks. - ppc ``taihu`` machine (removed in 7.2) ' @@ -1084,6 +1074,16 @@ for all machine types using the PXA2xx and OMAP2 SoCs. We are also dropping the ``cheetah`` OMAP1 board, because we don't have any test images for it and don't know of anybody who does. +Aspeed ``tacoma-bmc`` machine (removed in 10.0) +''' + +The ``tacoma-bmc`` machi
[PATCH 1/4] docs/about: Belatedly document tightening of QMP device_add checking
Commit 4d8b0f0a9536 (v6.2.0) deprecate incorrectly typed device_add arguments. Commit be93fd53723c (qdev-monitor: avoid QemuOpts in QMP device_add) fixed them for v9.2.0, but neglected to update documentation. Do that now. Cc: Stefan Hajnoczi Signed-off-by: Markus Armbruster --- docs/about/deprecated.rst | 14 -- docs/about/removed-features.rst | 9 + 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 44d3427e98..9665bc6fcf 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -187,20 +187,6 @@ threads (for example, it only reports source side of multifd threads, without reporting any destination threads, or non-multifd source threads). For debugging purpose, please use ``-name $VM,debug-threads=on`` instead. -Incorrectly typed ``device_add`` arguments (since 6.2) -'' - -Due to shortcomings in the internal implementation of ``device_add``, QEMU -incorrectly accepts certain invalid arguments: Any object or list arguments are -silently ignored. Other argument types are not checked, but an implicit -conversion happens, so that e.g. string values can be assigned to integer -device properties or vice versa. - -This is a bug in QEMU that will be fixed in the future so that previously -accepted incorrect commands will return an error. Users should make sure that -all arguments passed to ``device_add`` are consistent with the documented -property types. - Host Architectures -- diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst index 063284d4f8..92b5ba6218 100644 --- a/docs/about/removed-features.rst +++ b/docs/about/removed-features.rst @@ -722,6 +722,15 @@ Use ``multifd-channels`` instead. Use ``multifd-compression`` instead. +Incorrectly typed ``device_add`` arguments (since 9.2) +'' + +Due to shortcomings in the internal implementation of ``device_add``, +QEMU used to incorrectly accept certain invalid arguments. Any object +or list arguments were silently ignored. Other argument types were not +checked, but an implicit conversion happened, so that e.g. string +values could be assigned to integer device properties or vice versa. + QEMU Machine Protocol (QMP) events -- -- 2.48.1
[PATCH 3/4] docs/about/deprecated: Move deprecation notes to tidy up order
The deprecation notes within a section are mostly in version order. Move the few that aren't so they are. Signed-off-by: Markus Armbruster --- docs/about/deprecated.rst | 32 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index ef4ea84e69..4715d1ede5 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -148,6 +148,14 @@ options are removed in favor of using explicit ``blockdev-create`` and ``blockdev-add`` calls. See :doc:`/interop/live-block-operations` for details. +``query-migrationthreads`` (since 9.2) +'' + +To be removed with no replacement, as it reports only a limited set of +threads (for example, it only reports source side of multifd threads, +without reporting any destination threads, or non-multifd source threads). +For debugging purpose, please use ``-name $VM,debug-threads=on`` instead. + ``block-job-pause`` (since 10.1) @@ -184,14 +192,6 @@ Use ``job-finalize`` instead. This argument has always been ignored. -``query-migrationthreads`` (since 9.2) -'' - -To be removed with no replacement, as it reports only a limited set of -threads (for example, it only reports source side of multifd threads, -without reporting any destination threads, or non-multifd source threads). -For debugging purpose, please use ``-name $VM,debug-threads=on`` instead. - Host Architectures -- @@ -522,14 +522,6 @@ PCIe passthrough shall be the mainline solution. CPU device properties ' -``pcommit`` on x86 (since 9.1) -^^ - -The PCOMMIT instruction was never included in any physical processor. -It was implemented as a no-op instruction in TCG up to QEMU 9.0, but -only with ``-cpu max`` (which does not guarantee migration compatibility -across versions). - ``pmu-num=n`` on RISC-V CPUs (since 8.2) @@ -539,6 +531,14 @@ be calculated with ``((2 ^ n) - 1) << 3``. The least significant three bits must be left clear. +``pcommit`` on x86 (since 9.1) +^^ + +The PCOMMIT instruction was never included in any physical processor. +It was implemented as a no-op instruction in TCG up to QEMU 9.0, but +only with ``-cpu max`` (which does not guarantee migration compatibility +across versions). + Backwards compatibility --- -- 2.48.1
[PATCH 2/4] qapi/migration: Deprecate migrate argument @detach
Argument @detach has always been ignored. Start the clock to get rid
of it.
Cc: Peter Xu
Cc: Fabiano Rosas
Signed-off-by: Markus Armbruster
---
docs/about/deprecated.rst | 5 +
qapi/migration.json | 18 +-
2 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 9665bc6fcf..ef4ea84e69 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -179,6 +179,11 @@ Use ``job-dismiss`` instead.
Use ``job-finalize`` instead.
+``migrate`` argument ``detach`` (since 10.1)
+
+
+This argument has always been ignored.
+
``query-migrationthreads`` (since 9.2)
''
diff --git a/qapi/migration.json b/qapi/migration.json
index 8b9c53595c..ecd266f98e 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1660,6 +1660,10 @@
#
# @resume: resume one paused migration, default "off". (since 3.0)
#
+# Features:
+#
+# @deprecated: Argument @detach is deprecated.
+#
# Since: 0.14
#
# .. admonition:: Notes
@@ -1668,19 +1672,14 @@
#migration's progress and final result (this information is
#provided by the 'status' member).
#
-# 2. All boolean arguments default to false.
-#
-# 3. The user Monitor's "detach" argument is invalid in QMP and
-#should not be used.
-#
-# 4. The uri argument should have the Uniform Resource Identifier
+# 2. The uri argument should have the Uniform Resource Identifier
#of default destination VM. This connection will be bound to
#default network.
#
-# 5. For now, number of migration streams is restricted to one,
+# 3. For now, number of migration streams is restricted to one,
#i.e. number of items in 'channels' list is just 1.
#
-# 6. The 'uri' and 'channels' arguments are mutually exclusive;
+# 4. The 'uri' and 'channels' arguments are mutually exclusive;
#exactly one of the two should be present.
#
# .. qmp-example::
@@ -1724,7 +1723,8 @@
{ 'command': 'migrate',
'data': {'*uri': 'str',
'*channels': [ 'MigrationChannel' ],
- '*detach': 'bool', '*resume': 'bool' } }
+ '*detach': { 'type': 'bool', 'features': [ 'deprecated' ] },
+ '*resume': 'bool' } }
##
# @migrate-incoming:
--
2.48.1
[PATCH 0/4] An overdue deprecation, a doc fix, a bit of cleanup
Markus Armbruster (4): docs/about: Belatedly document tightening of QMP device_add checking qapi/migration: Deprecate migrate argument @detach docs/about/deprecated: Move deprecation notes to tidy up order docs/about/removed-features: Move removal notes to tidy up order docs/about/deprecated.rst | 47 +- docs/about/removed-features.rst | 69 +++-- qapi/migration.json | 18 - 3 files changed, 67 insertions(+), 67 deletions(-) -- 2.48.1
Re: [PATCH 0/4] An overdue deprecation, a doc fix, a bit of cleanup
Queued.
Re: [PATCH 1/4] docs/about: Belatedly document tightening of QMP device_add checking
Eric Blake writes: > On Wed, May 21, 2025 at 08:37:08AM +0200, Markus Armbruster via Devel wrote: >> Commit 4d8b0f0a9536 (v6.2.0) deprecate incorrectly typed device_add > > deprecated Yes. Thank you! >> arguments. Commit be93fd53723c (qdev-monitor: avoid QemuOpts in QMP >> device_add) fixed them for v9.2.0, but neglected to update >> documentation. Do that now. >> >> Cc: Stefan Hajnoczi >> Signed-off-by: Markus Armbruster
Re: [PATCH] qapi: net/tap: deprecate vhostforce option
Vladimir Sementsov-Ogievskiy writes:
> On 30.08.25 11:17, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy writes:
>>
>>> This option simply duplicates the @vhost option since long ago
>>> (10 years!)
>>> commit 1e7398a140f7a6 ("vhost: enable vhost without without MSI-X").
>>
>> This isn't obvious to me.
>>
>> As far as I can see, their only use is in net_init_tap_one():
>>
>> if (tap->has_vhost ? tap->vhost :
>> vhostfdname || (tap->has_vhostforce && tap->vhostforce)) {
>>
>> Can you take this apart for me?
>
> Prior 1e7398a140f7a6, to enable vhost for some specific kind of guests
> (that don't have MSI-X support), you should hav set vhostforce=on
> (with vhost=on or unset).
>
> Since 1e7398a140f7a6, guest type doesn't matter, all guests are equal
> for vhost-enabling options logic.
>
> So we simply have redundant options:
>
> vhost=on / vhost=off : vhostforce ignored, doesn't make sense
>
> vhost unset : vhostforce counts, enabling vhost
>
> So you may enable vhost several ways:
> - vhost=on
> - vhostforce=on
> - vhost=on + vhostforce=on
> - and even vhost=on + vhostforce=off
>
> - they are all equal.
So @vhostforce doesn't quite duplicate @vhost: if they conflict, @vhost
silently takes precedence.
>>> Let's finally deprecate it.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>> ---
>>> docs/about/deprecated.rst | 7 +++
>>> qapi/net.json | 6 +-
>>> 2 files changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
>>> index d50645a071..d14cb37480 100644
>>> --- a/docs/about/deprecated.rst
>>> +++ b/docs/about/deprecated.rst
>>> @@ -516,6 +516,13 @@ Stream ``reconnect`` (since 9.2)
>>> The ``reconnect`` option only allows specifying second granularity
>>> timeouts,
>>> which is not enough for all types of use cases, use ``reconnect-ms``
>>> instead.
>>> +TAP ``vhostforce`` (since 10.2)
>>> +^^^
>>> +
>>> +The ``vhostforce`` option just duplicates the main ``vhost`` option.
>>> +Use ``vhost`` alone.
>>
>> Would "Use instead ``vhost`` instead" be clearer?
>
> I meant, that user should not use vhost=on + vhostforce=on anymore.
>
> My be just "Use ``vhost``", without "alone"/"instead"?
Suggest
The ``vhostforce`` option is redundant with the ``vhost`` option.
If they conflict, ``vhost`` takes precedence. Just use ``vhost``.
Thanks!
[...]
Re: [PATCH 02/33] vhost: drop backend_features field
Cc: libvirt
Vladimir Sementsov-Ogievskiy writes:
> This field is mostly unused and sometimes confusing (we even have
> a TODO-like comment to drop it). Let's finally do.
>
> The field is used to held VHOST_USER_F_PROTOCOL_FEATURES for vhost-user
> and/or VHOST_NET_F_VIRTIO_NET_HDR for vhost-net (which may be
> vhoust-user-net). But we can simply recalculte these two flags inplace
> from hdev->features, and from net-client for
> VHOST_NET_F_VIRTIO_NET_HDR.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
[...]
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 66be6afc88..9f9dd2d46d 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -100,16 +100,9 @@ struct vhost_dev {
> *
> * @features: available features provided by the backend
> * @acked_features: final negotiated features with front-end driver
> - *
> - * @backend_features: this is used in a couple of places to either
> - * store VHOST_USER_F_PROTOCOL_FEATURES to apply to
> - * VHOST_USER_SET_FEATURES or VHOST_NET_F_VIRTIO_NET_HDR. Its
> - * future use should be discouraged and the variable retired as
> - * its easy to confuse with the VirtIO backend_features.
I guess this is the TODO-like comment mentioned in the commit message.
> */
> uint64_t features;
> uint64_t acked_features;
> -uint64_t backend_features;
>
> /**
> * @protocol_features: is the vhost-user only feature set by
> diff --git a/qapi/virtio.json b/qapi/virtio.json
> index 9d652fe4a8..0aae77340d 100644
> --- a/qapi/virtio.json
> +++ b/qapi/virtio.json
> @@ -85,8 +85,6 @@
> #
> # @acked-features: vhost_dev acked_features
> #
> -# @backend-features: vhost_dev backend_features
> -#
> # @protocol-features: vhost_dev protocol_features
> #
> # @max-queues: vhost_dev max_queues
> @@ -106,7 +104,6 @@
> 'vq-index': 'int',
> 'features': 'VirtioDeviceFeatures',
> 'acked-features': 'VirtioDeviceFeatures',
> -'backend-features': 'VirtioDeviceFeatures',
> 'protocol-features': 'VhostDeviceProtocols',
> 'max-queues': 'uint64',
> 'backend-cap': 'uint64',
Incompatible change. We can do this because it's only visible in the
return value of x-query-virtio-status, which is unstable. Recommend to
note this in the commit message.
Acked-by: Markus Armbruster
Re: [PATCH v2] qapi: net/tap: deprecate vhostforce option
Vladimir Sementsov-Ogievskiy writes:
> This option doesn't make sense since long ago (10 years!)
> commit 1e7398a140f7a6 ("vhost: enable vhost without without MSI-X").
>
> Prior 1e7398a140f7a6, to enable vhost for some specific kind of guests
> (that don't have MSI-X support), you should have set vhostforce=on
> (with vhost=on or unset).
>
> Since 1e7398a140f7a6, guest type doesn't matter, all guests are equal
> for vhost-enabling options logic.
>
> The current logic is:
> vhost=on / vhost=off : vhostforce ignored, doesn't make sense
> vhost unset : vhostforce counts, enabling vhost
>
> Currently you may enable vhost several ways:
> - vhost=on
> - vhostforce=on
> - vhost=on + vhostforce=on
> - and even vhost=on + vhostforce=off
>
> - they are all equal.
>
> Let's finally deprecate the extra option.
>
> Also, fix @vhostforce documentation.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
>
> v2:
> - improve wording
> - add documentation fix
>
> docs/about/deprecated.rst | 7 +++
> qapi/net.json | 11 +--
> 2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index d50645a071..b17a5a41aa 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -516,6 +516,13 @@ Stream ``reconnect`` (since 9.2)
> The ``reconnect`` option only allows specifying second granularity timeouts,
> which is not enough for all types of use cases, use ``reconnect-ms`` instead.
>
> +TAP ``vhostforce`` (since 10.2)
> +^^^
> +
> +The ``vhostforce`` option is redundant with the ``vhost`` option.
> +If they conflict, ``vhost`` takes precedence. Just use ``vhost``.
> +
> +
> VFIO device options
> '''
>
> diff --git a/qapi/net.json b/qapi/net.json
> index 78bcc9871e..e89c7aff5f 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -346,13 +346,20 @@
> # @vhostfds: file descriptors of multiple already opened vhost net
> # devices
> #
> -# @vhostforce: vhost on for non-MSIX virtio guests
> +# @vhostforce: enable vhost-net network accelerator. Ignored when
> +#@vhost is set.
Two spaces between sentences for consistency, please.
> #
> # @queues: number of queues to be created for multiqueue capable tap
> #
> # @poll-us: maximum number of microseconds that could be spent on busy
> # polling for tap (since 2.7)
> #
> +# Features:
> +#
> +# @deprecated: Member @vhostforce is deprecated. The @vhostforce
> +#option is redundant with the @vhost option. If they conflict,
> +#@vhost takes precedence. Just use @vhost.
> +#
> # Since: 1.2
> ##
> { 'struct': 'NetdevTapOptions',
> @@ -369,7 +376,7 @@
> '*vhost': 'bool',
> '*vhostfd':'str',
> '*vhostfds': 'str',
> -'*vhostforce': 'bool',
> +'*vhostforce': { 'type': 'bool', 'features': [ 'deprecated' ] },
> '*queues': 'uint32',
> '*poll-us':'uint32'} }
Reviewed-by: Markus Armbruster
Re: [PATCH] qapi: net/tap: deprecate vhostforce option
Vladimir Sementsov-Ogievskiy writes:
> This option simply duplicates the @vhost option since long ago
> (10 years!)
> commit 1e7398a140f7a6 ("vhost: enable vhost without without MSI-X").
This isn't obvious to me.
As far as I can see, their only use is in net_init_tap_one():
if (tap->has_vhost ? tap->vhost :
vhostfdname || (tap->has_vhostforce && tap->vhostforce)) {
Can you take this apart for me?
> Let's finally deprecate it.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> docs/about/deprecated.rst | 7 +++
> qapi/net.json | 6 +-
> 2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index d50645a071..d14cb37480 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -516,6 +516,13 @@ Stream ``reconnect`` (since 9.2)
> The ``reconnect`` option only allows specifying second granularity timeouts,
> which is not enough for all types of use cases, use ``reconnect-ms`` instead.
>
> +TAP ``vhostforce`` (since 10.2)
> +^^^
> +
> +The ``vhostforce`` option just duplicates the main ``vhost`` option.
> +Use ``vhost`` alone.
Would "Use instead ``vhost`` instead" be clearer?
> +
> +
> VFIO device options
> '''
>
> diff --git a/qapi/net.json b/qapi/net.json
> index 78bcc9871e..d1216bb60a 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -353,6 +353,10 @@
> # @poll-us: maximum number of microseconds that could be spent on busy
> # polling for tap (since 2.7)
> #
> +# Features:
> +#
> +# @deprecated: Member @vhostforce is deprecated. Simply use @vhost.
@deprecated text is commonly of the form "FOO is deprecated. Use BAR
instead."
Recommend "Use @vhost instead."
> +#
> # Since: 1.2
> ##
> { 'struct': 'NetdevTapOptions',
> @@ -369,7 +373,7 @@
> '*vhost': 'bool',
> '*vhostfd':'str',
> '*vhostfds': 'str',
> -'*vhostforce': 'bool',
> +'*vhostforce': { 'type': 'bool', 'features': [ 'deprecated' ] },
> '*queues': 'uint32',
> '*poll-us':'uint32'} }
Re: [PATCH v4 07/10] qmp: add chardev-resize command
Filip Hejsek writes:
> On Mon, 2025-09-15 at 08:35 +0200, Markus Armbruster wrote:
>> Filip Hejsek writes:
>>
>> > On Fri, 2025-09-12 at 16:01 +0200, Markus Armbruster wrote:
>> > > Cc: libvirt
>> > >
>> > > Filip Hejsek writes:
>> > >
>> > > > From: Szymon Lukasz
>> > > >
>> > > > The managment software can use this command to notify QEMU about the
>> > > > size of the terminal connected to a chardev, QEMU can then forward this
>> > > > information to the guest if the chardev is connected to a virtio
>> > > > console
>> > > > device.
>> > > >
>> > > > Signed-off-by: Szymon Lukasz
>> > > > Suggested-by: Daniel P. Berrangé
>> > > > Signed-off-by: Filip Hejsek
>> > > > ---
>> > > > chardev/char.c | 14 ++
>> > > > qapi/char.json | 22 ++
>> > > > 2 files changed, 36 insertions(+)
>> > > >
>> > > > diff --git a/chardev/char.c b/chardev/char.c
>> > > > index
>> > > > b45d79cb9b57643827eb7479257fdda2cf6b0434..6e3ade98614c949be8041ec5905a490ff536dee9
>> > > > 100644
>> > > > --- a/chardev/char.c
>> > > > +++ b/chardev/char.c
>> > > > @@ -1269,6 +1269,20 @@ bool qmp_add_client_char(int fd, bool
>> > > > has_skipauth, bool skipauth,
>> > > > return true;
>> > > > }
>> > > >
>> > > > +void qmp_chardev_resize(const char *id, uint16_t cols, uint16_t rows,
>> > > > +Error **errp)
>> > > > +{
>> > > > +Chardev *chr;
>> > > > +
>> > > > +chr = qemu_chr_find(id);
>> > > > +if (chr == NULL) {
>> > > > +error_setg(errp, "Chardev '%s' not found", id);
>> > > > +return;
>> > > > +}
>> > > > +
>> > > > +qemu_chr_resize(chr, cols, rows);
>> > > > +}
>> > > > +
>> > > > /*
>> > > > * Add a timeout callback for the chardev (in milliseconds), return
>> > > > * the GSource object created. Please use this to add timeout hook for
>> > > > diff --git a/qapi/char.json b/qapi/char.json
>> > > > index
>> > > > f0a53f742c8bee24c377551803a864fd36ac78cf..0a26c5eee6b71bc5de127a91b253cc69a9fe8ce6
>> > > > 100644
>> > > > --- a/qapi/char.json
>> > > > +++ b/qapi/char.json
>> > > > @@ -874,6 +874,28 @@
>> > > > { 'command': 'chardev-send-break',
>> > > >'data': { 'id': 'str' } }
>> > > >
>> > > > +##
>> > > > +# @chardev-resize:
>> > >
>> > > This name doesn't tell me what is being resized. PATCH 04 uses
>> > > "winsize", which is better. The (losely) related SIGWINCH suggests
>> > > "window change" or "window size change". Below, you use "terminal
>> > > size".
>> >
>> > How about chardev-console-resize? That would match the name of the
>> > virtio event (VIRTIO_CONSOLE_RESIZE).
>>
>> Not bad. It could become slightly bad if we make devices other than
>> "consoles" make us of it. Would that be possible?
>
> I don't think the size has any meaning for devices that are not
> connected to a console, although the code does not care whether it
> actually is a console and simply has a size for every chardev.
Double-checking: the command works for any ChardevBackendKind, doesn't
it?
> I guess I could also rename it to chardev-window-resize
> or chardev-set-window-size. Let me know if you prefer one of these.
I think I'd prefer "window" or "terminal".
"resize" and "set size" suggest that the command initiates a size
change. Not true, it notifies of a size change. Maybe
"chardev-window-size-changed", "chardev-terminal-size-changed",
"chardev-window-resized", or "chardev-terminal-resized".
>> > > > +#
>> > > > +# Notifies a chardev about the current size of the terminal connected
>> > > > +# to this chardev.
>> > >
>> > > Yes, but what is it good for? Your commit message tells: "managment
>> > > software can use this command to notify QEMU about the size of the
>> > > terminal connected to a chardev, QEMU can then forward this information
>> > > to the guest if the chardev is connected to a virtio console device."
>> >
>> > How about:
>> >
>> >Notifies a chardev about the current size of the terminal connected
>> >to this chardev. The information will be forwarded to the guest if
>> >the chardev is connected to a virtio console device.
>>
>> Works for me.
>>
>> > > > +#
>> > > > +# @id: the chardev's ID, must exist
>> > > > +# @cols: the number of columns
>> > > > +# @rows: the number of rows
>> > >
>> > > Blank lines between the argument descriptions, bease.
>> > >
>> > > What's the initial size?
>> >
>> > 0x0
Another question... 'vc' chardevs accept optional @rows, @cols (see
ChardevVC). Is this the same size or something else?
>> A clearly invalid size. I guess it effectively means "unknown size".
>> Should we document that?
>
> Probably. 0x0 is I think also the default size in the Linux kernel, but
> I don't think the Linux kernel documents this.
How does 0 x 0 behave compared to a valid size like 80 x 24?
>Another question is if
> the 0x0 size should be propagated to the guest over virtio. I think it
> should be, although the vi
Re: [PATCH v4 07/10] qmp: add chardev-resize command
Cc: libvirt
Filip Hejsek writes:
> From: Szymon Lukasz
>
> The managment software can use this command to notify QEMU about the
> size of the terminal connected to a chardev, QEMU can then forward this
> information to the guest if the chardev is connected to a virtio console
> device.
>
> Signed-off-by: Szymon Lukasz
> Suggested-by: Daniel P. Berrangé
> Signed-off-by: Filip Hejsek
> ---
> chardev/char.c | 14 ++
> qapi/char.json | 22 ++
> 2 files changed, 36 insertions(+)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index
> b45d79cb9b57643827eb7479257fdda2cf6b0434..6e3ade98614c949be8041ec5905a490ff536dee9
> 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -1269,6 +1269,20 @@ bool qmp_add_client_char(int fd, bool has_skipauth,
> bool skipauth,
> return true;
> }
>
> +void qmp_chardev_resize(const char *id, uint16_t cols, uint16_t rows,
> +Error **errp)
> +{
> +Chardev *chr;
> +
> +chr = qemu_chr_find(id);
> +if (chr == NULL) {
> +error_setg(errp, "Chardev '%s' not found", id);
> +return;
> +}
> +
> +qemu_chr_resize(chr, cols, rows);
> +}
> +
> /*
> * Add a timeout callback for the chardev (in milliseconds), return
> * the GSource object created. Please use this to add timeout hook for
> diff --git a/qapi/char.json b/qapi/char.json
> index
> f0a53f742c8bee24c377551803a864fd36ac78cf..0a26c5eee6b71bc5de127a91b253cc69a9fe8ce6
> 100644
> --- a/qapi/char.json
> +++ b/qapi/char.json
> @@ -874,6 +874,28 @@
> { 'command': 'chardev-send-break',
>'data': { 'id': 'str' } }
>
> +##
> +# @chardev-resize:
This name doesn't tell me what is being resized. PATCH 04 uses
"winsize", which is better. The (losely) related SIGWINCH suggests
"window change" or "window size change". Below, you use "terminal
size".
> +#
> +# Notifies a chardev about the current size of the terminal connected
> +# to this chardev.
Yes, but what is it good for? Your commit message tells: "managment
software can use this command to notify QEMU about the size of the
terminal connected to a chardev, QEMU can then forward this information
to the guest if the chardev is connected to a virtio console device."
> +#
> +# @id: the chardev's ID, must exist
> +# @cols: the number of columns
> +# @rows: the number of rows
Blank lines between the argument descriptions, bease.
What's the initial size?
Do we need a way to query the size?
> +#
> +# Since: 10.2
> +#
> +# .. qmp-example::
> +#
> +# -> { "execute": "chardev-resize", "arguments": { "id": "foo", "cols":
> 80, "rows": 24 } }
> +# <- { "return": {} }
> +##
> +{ 'command': 'chardev-resize',
> + 'data': { 'id': 'str',
> +'cols': 'uint16',
> +'rows': 'uint16' } }
> +
> ##
> # @VSERPORT_CHANGE:
> #
Re: [PATCH v4 07/10] qmp: add chardev-resize command
Filip Hejsek writes:
> On Fri, 2025-09-12 at 16:01 +0200, Markus Armbruster wrote:
>> Cc: libvirt
>>
>> Filip Hejsek writes:
>>
>> > From: Szymon Lukasz
>> >
>> > The managment software can use this command to notify QEMU about the
>> > size of the terminal connected to a chardev, QEMU can then forward this
>> > information to the guest if the chardev is connected to a virtio console
>> > device.
>> >
>> > Signed-off-by: Szymon Lukasz
>> > Suggested-by: Daniel P. Berrangé
>> > Signed-off-by: Filip Hejsek
>> > ---
>> > chardev/char.c | 14 ++
>> > qapi/char.json | 22 ++
>> > 2 files changed, 36 insertions(+)
>> >
>> > diff --git a/chardev/char.c b/chardev/char.c
>> > index
>> > b45d79cb9b57643827eb7479257fdda2cf6b0434..6e3ade98614c949be8041ec5905a490ff536dee9
>> > 100644
>> > --- a/chardev/char.c
>> > +++ b/chardev/char.c
>> > @@ -1269,6 +1269,20 @@ bool qmp_add_client_char(int fd, bool has_skipauth,
>> > bool skipauth,
>> > return true;
>> > }
>> >
>> > +void qmp_chardev_resize(const char *id, uint16_t cols, uint16_t rows,
>> > +Error **errp)
>> > +{
>> > +Chardev *chr;
>> > +
>> > +chr = qemu_chr_find(id);
>> > +if (chr == NULL) {
>> > +error_setg(errp, "Chardev '%s' not found", id);
>> > +return;
>> > +}
>> > +
>> > +qemu_chr_resize(chr, cols, rows);
>> > +}
>> > +
>> > /*
>> > * Add a timeout callback for the chardev (in milliseconds), return
>> > * the GSource object created. Please use this to add timeout hook for
>> > diff --git a/qapi/char.json b/qapi/char.json
>> > index
>> > f0a53f742c8bee24c377551803a864fd36ac78cf..0a26c5eee6b71bc5de127a91b253cc69a9fe8ce6
>> > 100644
>> > --- a/qapi/char.json
>> > +++ b/qapi/char.json
>> > @@ -874,6 +874,28 @@
>> > { 'command': 'chardev-send-break',
>> >'data': { 'id': 'str' } }
>> >
>> > +##
>> > +# @chardev-resize:
>>
>> This name doesn't tell me what is being resized. PATCH 04 uses
>> "winsize", which is better. The (losely) related SIGWINCH suggests
>> "window change" or "window size change". Below, you use "terminal
>> size".
>
> How about chardev-console-resize? That would match the name of the
> virtio event (VIRTIO_CONSOLE_RESIZE).
Not bad. It could become slightly bad if we make devices other than
"consoles" make us of it. Would that be possible?
>> > +#
>> > +# Notifies a chardev about the current size of the terminal connected
>> > +# to this chardev.
>>
>> Yes, but what is it good for? Your commit message tells: "managment
>> software can use this command to notify QEMU about the size of the
>> terminal connected to a chardev, QEMU can then forward this information
>> to the guest if the chardev is connected to a virtio console device."
>
> How about:
>
>Notifies a chardev about the current size of the terminal connected
>to this chardev. The information will be forwarded to the guest if
>the chardev is connected to a virtio console device.
Works for me.
>> > +#
>> > +# @id: the chardev's ID, must exist
>> > +# @cols: the number of columns
>> > +# @rows: the number of rows
>>
>> Blank lines between the argument descriptions, bease.
>>
>> What's the initial size?
>
> 0x0
A clearly invalid size. I guess it effectively means "unknown size".
Should we document that?
>> Do we need a way to query the size?
>
> I don't think it is necessary. What would be the usecase for that?
I don't know, but it's my standard question when I see an interface to
set something without an interface to get it. Its purpose is to make us
think, not to make us at the get blindly.
>> > +#
>> > +# Since: 10.2
>> > +#
>> > +# .. qmp-example::
>> > +#
>> > +# -> { "execute": "chardev-resize", "arguments": { "id": "foo",
>> > "cols": 80, "rows": 24 } }
>> > +# <- { "return": {} }
>> > +##
>> > +{ 'command': 'chardev-resize',
>> > + 'data': { 'id': 'str',
>> > +'cols': 'uint16',
>> > +'rows': 'uint16' } }
>> > +
>> > ##
>> > # @VSERPORT_CHANGE:
>> > #
Re: [PATCH v4 07/10] qmp: add chardev-resize command
Filip Hejsek writes: > On Tue, 2025-09-16 at 15:07 +0200, Markus Armbruster wrote: >> Filip Hejsek writes: >> >> > On Mon, 2025-09-15 at 08:35 +0200, Markus Armbruster wrote: >> > > Filip Hejsek writes: >> > > >> > > > On Fri, 2025-09-12 at 16:01 +0200, Markus Armbruster wrote: >> > > > > Cc: libvirt >> > > > > >> > > > > Filip Hejsek writes: >> > > > > >> > > > > > From: Szymon Lukasz >> > > > > > >> > > > > > [...] >> > > > > > >> > > > > > +## >> > > > > > +# @chardev-resize: >> > > > > >> > > > > This name doesn't tell me what is being resized. PATCH 04 uses >> > > > > "winsize", which is better. The (losely) related SIGWINCH suggests >> > > > > "window change" or "window size change". Below, you use "terminal >> > > > > size". >> > > > >> > > > How about chardev-console-resize? That would match the name of the >> > > > virtio event (VIRTIO_CONSOLE_RESIZE). >> > > >> > > Not bad. It could become slightly bad if we make devices other than >> > > "consoles" make us of it. Would that be possible? >> > >> > I don't think the size has any meaning for devices that are not >> > connected to a console, although the code does not care whether it >> > actually is a console and simply has a size for every chardev. >> >> Double-checking: the command works for any ChardevBackendKind, doesn't >> it? > > Yes. For some (e.g. stdio) it will clash with builtin resize detection, > but it can still be used (last update wins). > > Maybe using the command should be prohibited for some device types? Depends on the command's intended use. In general, failing a command is better than silently not doing what it commands. If the command is "tell the guest the window size changed", and we can't, then failing the command is better than silently doing nothing. Howver, other considerations can trump this. If we want to use this command on any window size change without having to know the device type, having it silently do nothing for device types that don't support it can be more convenient. Fine as long as the behavior is clearly documented. >> > I guess I could also rename it to chardev-window-resize >> > or chardev-set-window-size. Let me know if you prefer one of these. >> >> I think I'd prefer "window" or "terminal". >> >> "resize" and "set size" suggest that the command initiates a size >> change. Not true, it notifies of a size change. Maybe >> "chardev-window-size-changed", "chardev-terminal-size-changed", >> "chardev-window-resized", or "chardev-terminal-resized". > > OK, then I'll use "chardev-window-size-changed". > >> > > > > >> > > > >> [...] >> Another question... 'vc' chardevs accept optional @rows, @cols (see >> ChardevVC). Is this the same size or something else? > > Well, yes and no. @cols + @rows control the actual size of the console > screen buffer, while the chardev size is only used to inform the guest > about the size. @cols and @rows can also be unset, in which case the > size will be determined automatically from display and font size. Thanks! > This patch series does not yet implement size propagation for the 'vc' > device. I have WIP patches for that, but there is something I'm not > sure how to do, so I will likely send an RFC first. Would it make sense to mention this gap in a commit message? >> > > A clearly invalid size. I guess it effectively means "unknown size". >> > > Should we document that? >> > >> > Probably. 0x0 is I think also the default size in the Linux kernel, but >> > I don't think the Linux kernel documents this. >> >> How does 0 x 0 behave compared to a valid size like 80 x 24? > > In these patches it is not treated specially (apart from being the > default). I think the Linux kernel doesn't treat it specially either. > Terminal programs generally interpret it as unknown size and use other > methods to obtain the size like environment variables, the terminfo > database, or defaulting to 80x24. Example: > >$ python -c 'import termios; termios.tcsetwinsize(0, (0,0))' >$ tput cols >80 Do you think working this into the documentation would be useful? >> [...] >> > > > > Do we need a way to query the size? >> > > > >> > > > I don't think it is necessary. What would be the usecase for that? >> > > >> > > I don't know, but it's my standard question when I see an interface to >> > > set something without an interface to get it. Its purpose is to make us >> > > think, not to make us at the get blindly. >> > >> > I guess it might be useful for debugging. If the size is not propagated >> > correctly, one might query it to find out on which side the problem is. >> >> We have query-chardev. It doesn't return much. > > I'm not sure what you're implying. Shall I add the size there? If we could already query chardev configuration / state comprehensively, then I'd ask you to make it cover your new state bits, too. Since we don't, and there is no clear use case at this time, I'm not asking you to do that.
