Re: [libvirt] [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target
Cc'ing a few more people. Daniel Henrique Barboza <danielhb@linux.ibm.com> writes:
When issuing the qmp/hmp 'system_wakeup' command, what happens in a nutshell is:
- qmp_system_wakeup_request set runstate to RUNNING, sets a wakeup_reason and notify the event - in the main_loop, all vcpus are paused, a system reset is issued, all subscribers of wakeup_notifiers receives a notification, vcpus are then resumed and the wake up QAPI event is fired
Note that this procedure alone doesn't ensure that the guest will awake from SUSPENDED state - the subscribers of the wake up event must take action to resume the guest, otherwise the guest will simply reboot.
At this moment there are only two subscribers of the wake up event: one in hw/acpi/core.c and another one in hw/i386/xen/xen-hvm.c. This means that system_wakeup does not work as intended with other architectures.
However, only the presence of 'system_wakeup' is required for QGA to support 'guest-suspend-ram' and 'guest-suspend-hybrid' at this moment. This means that the user/management will expect to suspend the guest using one of those suspend commands and then resume execution using system_wakeup, regardless of the support offered in system_wakeup in the first place.
This patch adds a new flag called 'wakeup-suspend-support' in TargetInfo that allows the caller to query if the guest supports wake up from suspend via system_wakeup. It goes over the subscribers of the wake up event and, if it's empty, it assumes that the guest does not support wake up from suspend (and thus, pm-suspend itself).
This is the expected output of query-target when running a x86 guest:
{"execute" : "query-target"} {"return": {"arch": "x86_64", "wakeup-suspend-support": true}}
This is the output when running a pseries guest:
{"execute" : "query-target"} {"return": {"arch": "ppc64", "wakeup-suspend-support": false}}
Given that the TargetInfo structure is read-only, adding a new flag to it is backwards compatible. There is no need to deprecate the old TargetInfo format.
With this extra tool, management can avoid situations where a guest that does not have proper suspend/wake capabilities ends up in inconsistent state (e.g. https://github.com/open-power-host-os/qemu/issues/31).
Reported-by: Balamuruhan S <bala24@linux.vnet.ibm.com> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
Is query-target is the right place to carry this flag? v7 is rather late for this kind of question; my sincere apologies. The flag is true after qemu_register_wakeup_notifier(). Callers so far: * piix4_pm_realize() via acpi_pm1_cnt_init() This is the realize method of device PIIX4_PM. It's an optional onboard device (suppressed by -no-acpi) of machine types pc-i440fx-VERSION, pc-VERSION, malta. * pc_q35_init() via ich9_lpc_pm_init(), ich9_pm_init(), acpi_pm1_cnt_init() This is the initialization method of machine types pc-q35-VERSION. Note that -no-acpi is not honored. * vt82c686b_pm_realize() via acpi_pm1_cnt_init() This is the realize method of device VT82C686B_PM. It's an onboard device of machine type fulong2e. Again, -no-acpi is not honored. * xen_hvm_init() This one gets called with -accel xen. I suspect the actual callback xen_wakeup_notifier() doesn't actually make wakeup work, unlike the acpi_notify_wakeup() callback registered by the other callers. Issue#1: this calls into question your assumption that the existence of a wake-up notifier implies wake-up works. It still holds if --accel xen is only accepted together with other configuration that triggers registration of acpi_notify_wakeup(). Is it? Stefano, Anthony? Issue#2: the flag isn't a property of the target. Due to -no-acpi, it's not even a property of the machine type. If it was, query-machines would be the natural owner of the flag. Perhaps query-machines is still the proper owner. The value of wakeup-suspend-support would have to depend on -no-acpi for the machine types that honor it. Not ideal; I'd prefer MachineInfo to be static. Tolerable? I guess that's also a libvirt question.
On Fri, May 18, 2018 at 10:48:31AM +0200, Markus Armbruster wrote:
Cc'ing a few more people.
Daniel Henrique Barboza <danielhb@linux.ibm.com> writes:
When issuing the qmp/hmp 'system_wakeup' command, what happens in a nutshell is:
- qmp_system_wakeup_request set runstate to RUNNING, sets a wakeup_reason and notify the event - in the main_loop, all vcpus are paused, a system reset is issued, all subscribers of wakeup_notifiers receives a notification, vcpus are then resumed and the wake up QAPI event is fired
Note that this procedure alone doesn't ensure that the guest will awake from SUSPENDED state - the subscribers of the wake up event must take action to resume the guest, otherwise the guest will simply reboot.
At this moment there are only two subscribers of the wake up event: one in hw/acpi/core.c and another one in hw/i386/xen/xen-hvm.c. This means that system_wakeup does not work as intended with other architectures.
However, only the presence of 'system_wakeup' is required for QGA to support 'guest-suspend-ram' and 'guest-suspend-hybrid' at this moment. This means that the user/management will expect to suspend the guest using one of those suspend commands and then resume execution using system_wakeup, regardless of the support offered in system_wakeup in the first place.
This patch adds a new flag called 'wakeup-suspend-support' in TargetInfo that allows the caller to query if the guest supports wake up from suspend via system_wakeup. It goes over the subscribers of the wake up event and, if it's empty, it assumes that the guest does not support wake up from suspend (and thus, pm-suspend itself).
This is the expected output of query-target when running a x86 guest:
{"execute" : "query-target"} {"return": {"arch": "x86_64", "wakeup-suspend-support": true}}
This is the output when running a pseries guest:
{"execute" : "query-target"} {"return": {"arch": "ppc64", "wakeup-suspend-support": false}}
Given that the TargetInfo structure is read-only, adding a new flag to it is backwards compatible. There is no need to deprecate the old TargetInfo format.
With this extra tool, management can avoid situations where a guest that does not have proper suspend/wake capabilities ends up in inconsistent state (e.g. https://github.com/open-power-host-os/qemu/issues/31).
Reported-by: Balamuruhan S <bala24@linux.vnet.ibm.com> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
Is query-target is the right place to carry this flag? v7 is rather late for this kind of question; my sincere apologies. [...]
Issue#2: the flag isn't a property of the target. Due to -no-acpi, it's not even a property of the machine type. If it was, query-machines would be the natural owner of the flag.
Perhaps query-machines is still the proper owner. The value of wakeup-suspend-support would have to depend on -no-acpi for the machine types that honor it. Not ideal; I'd prefer MachineInfo to be static. Tolerable? I guess that's also a libvirt question.
It depends when libvirt is going to query it. Is it OK to only query it after the VM is already up and running? If it is, then we can simply expose it as a read-only property of the machine object. Or, if we don't want to rely on qom-get as a stable API, we can add a new query command (query-machine? query-power-management?) -- Eduardo
On 05/21/2018 03:14 PM, Eduardo Habkost wrote:
Issue#2: the flag isn't a property of the target. Due to -no-acpi, it's not even a property of the machine type. If it was, query-machines would be the natural owner of the flag.
Perhaps query-machines is still the proper owner. The value of wakeup-suspend-support would have to depend on -no-acpi for the machine types that honor it. Not ideal; I'd prefer MachineInfo to be static. Tolerable? I guess that's also a libvirt question. It depends when libvirt is going to query it. Is it OK to only query it after the VM is already up and running? If it is, then we can simply expose it as a read-only property of the machine object.
Or, if we don't want to rely on qom-get as a stable API, we can add a new query command (query-machine? query-power-management?)
In the first version this logic was included in a new query command called "query-wakeup-from-suspend-support": https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg00889.html In that review it was suggested that this logic could be a flag in either query-target or query-machines API. Before sending the v2 I sent the following comment: "After investigating, I think that it's simpler to hook the wakeup support info into TargetInfo than MachineInfo, given that the detection I'm using for this new property is based on the current runtime state. Hooking into MachineInfo would require to change the MachineClass to add a new property, then setting it up for the machines that have the wakeup support (only x86 so far). Definitely doable, but if we don't have any favorites between MachineInfo and TargetInfo I'd rather pick the simpler route. So, if no one objects, I'll rework this series by putting the logic inside query-target instead of a new API." Since no objection was made back then, this logic was put into query-target starting in v2. Still, I don't have any favorites though: query-target looks ok, query-machine looks ok and a new API looks ok too. It's all about what makes (more) sense in the management level, I think. danielhb
On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote:
On 05/21/2018 03:14 PM, Eduardo Habkost wrote:
Issue#2: the flag isn't a property of the target. Due to -no-acpi, it's not even a property of the machine type. If it was, query-machines would be the natural owner of the flag.
Perhaps query-machines is still the proper owner. The value of wakeup-suspend-support would have to depend on -no-acpi for the machine types that honor it. Not ideal; I'd prefer MachineInfo to be static. Tolerable? I guess that's also a libvirt question. It depends when libvirt is going to query it. Is it OK to only query it after the VM is already up and running? If it is, then we can simply expose it as a read-only property of the machine object.
Or, if we don't want to rely on qom-get as a stable API, we can add a new query command (query-machine? query-power-management?)
In the first version this logic was included in a new query command called "query-wakeup-from-suspend-support":
https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg00889.html
In that review it was suggested that this logic could be a flag in either query-target or query-machines API. Before sending the v2 I sent the following comment:
"After investigating, I think that it's simpler to hook the wakeup support info into TargetInfo than MachineInfo, given that the detection I'm using for this new property is based on the current runtime state. Hooking into MachineInfo would require to change the MachineClass to add a new property, then setting it up for the machines that have the wakeup support (only x86 so far). Definitely doable, but if we don't have any favorites between MachineInfo and TargetInfo I'd rather pick the simpler route.
So, if no one objects, I'll rework this series by putting the logic inside query-target instead of a new API."
Apologies for not noticing this series months ago. :(
Since no objection was made back then, this logic was put into query-target starting in v2. Still, I don't have any favorites though: query-target looks ok, query-machine looks ok and a new API looks ok too. It's all about what makes (more) sense in the management level, I think.
I understand the original objection from Eric: having to add a new command for every runtime flag we want to expose to the user looks wrong to me. However, extending query-machines and query-target looks wrong too, however. query-target looks wrong because this not a property of the target. query-machines is wrong because this is not a static property of the machine-type, but of the running machine instance. Can we have a new query command that could be an obvious container for simple machine capabilities that are not static? A name like "query-machine" would be generic enough for that, I guess. Markus, Eric, what do you think? -- Eduardo
Eduardo Habkost <ehabkost@redhat.com> writes:
On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote:
On 05/21/2018 03:14 PM, Eduardo Habkost wrote:
Issue#2: the flag isn't a property of the target. Due to -no-acpi, it's not even a property of the machine type. If it was, query-machines would be the natural owner of the flag.
Perhaps query-machines is still the proper owner. The value of wakeup-suspend-support would have to depend on -no-acpi for the machine types that honor it. Not ideal; I'd prefer MachineInfo to be static. Tolerable? I guess that's also a libvirt question. It depends when libvirt is going to query it. Is it OK to only query it after the VM is already up and running? If it is, then we can simply expose it as a read-only property of the machine object.
Or, if we don't want to rely on qom-get as a stable API, we can add a new query command (query-machine? query-power-management?)
In the first version this logic was included in a new query command called "query-wakeup-from-suspend-support":
https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg00889.html
In that review it was suggested that this logic could be a flag in either query-target or query-machines API. Before sending the v2 I sent the following comment:
"After investigating, I think that it's simpler to hook the wakeup support info into TargetInfo than MachineInfo, given that the detection I'm using for this new property is based on the current runtime state. Hooking into MachineInfo would require to change the MachineClass to add a new property, then setting it up for the machines that have the wakeup support (only x86 so far). Definitely doable, but if we don't have any favorites between MachineInfo and TargetInfo I'd rather pick the simpler route.
So, if no one objects, I'll rework this series by putting the logic inside query-target instead of a new API."
Apologies for not noticing this series months ago. :(
Seconded. Daniel, this (minor) mess is absolutely not your fault.
Since no objection was made back then, this logic was put into query-target starting in v2. Still, I don't have any favorites though: query-target looks ok, query-machine looks ok and a new API looks ok too. It's all about what makes (more) sense in the management level, I think.
I understand the original objection from Eric: having to add a new command for every runtime flag we want to expose to the user looks wrong to me.
Agreed.
However, extending query-machines and query-target looks wrong too, however. query-target looks wrong because this not a property of the target. query-machines is wrong because this is not a static property of the machine-type, but of the running machine instance.
Of the two, query-machines looks less wrong. Arguably, -no-acpi should not exist. It's an ad hoc flag that sneakily splits a few machine types into two variants, with and without ACPI. It's silently ignored for other machine types, even APCI-capable ones. If the machine type variants with and without ACPI were separate types, wakeup-suspend-support would be a static property of the machine type. However, "separate types" probably doesn't scale: I'm afraid we'd end up with an undesirable number of machine types. Avoiding that is exactly why we have machine types with configurable options. I suspect that's how ACPI should be configured (if at all). So, should we make -no-acpi sugar for a machine type parameter? And then deprecate -no-acpi for good measure?
Can we have a new query command that could be an obvious container for simple machine capabilities that are not static? A name like "query-machine" would be generic enough for that, I guess.
Having command names differ only in a single letter is awkward, but let's focus on things other than naming now, and use query-current-machine like a working title. query-machines is wrong because wakeup-suspend-support isn't static for some machine types. query-current-machine is also kind of wrong because wakeup-suspend-support *is* static for most machine types. Worse, a machine type property that is static for all machine types now could conceivably become dynamic when we add a machine type configuration knob. Would a way to tie the property to the configuration knob help? Something like wakeup-suspend-support taking values true (supported), false (not supported), and "acpi" (supported if machine type configuration knob "acpi" is switched on).
Markus, Eric, what do you think?
Haven't made up my mind, yet :)
On Wed, May 23, 2018 at 11:17:55AM +0200, Markus Armbruster wrote:
Eduardo Habkost <ehabkost@redhat.com> writes:
On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote: [...]
Since no objection was made back then, this logic was put into query-target starting in v2. Still, I don't have any favorites though: query-target looks ok, query-machine looks ok and a new API looks ok too. It's all about what makes (more) sense in the management level, I think.
I understand the original objection from Eric: having to add a new command for every runtime flag we want to expose to the user looks wrong to me.
Agreed.
However, extending query-machines and query-target looks wrong too, however. query-target looks wrong because this not a property of the target. query-machines is wrong because this is not a static property of the machine-type, but of the running machine instance.
Of the two, query-machines looks less wrong.
Arguably, -no-acpi should not exist. It's an ad hoc flag that sneakily splits a few machine types into two variants, with and without ACPI. It's silently ignored for other machine types, even APCI-capable ones.
If the machine type variants with and without ACPI were separate types, wakeup-suspend-support would be a static property of the machine type.
However, "separate types" probably doesn't scale: I'm afraid we'd end up with an undesirable number of machine types. Avoiding that is exactly why we have machine types with configurable options. I suspect that's how ACPI should be configured (if at all).
So, should we make -no-acpi sugar for a machine type parameter? And then deprecate -no-acpi for good measure?
I think we should.
Can we have a new query command that could be an obvious container for simple machine capabilities that are not static? A name like "query-machine" would be generic enough for that, I guess.
Having command names differ only in a single letter is awkward, but let's focus on things other than naming now, and use query-current-machine like a working title.
query-machines is wrong because wakeup-suspend-support isn't static for some machine types.
query-current-machine is also kind of wrong because wakeup-suspend-support *is* static for most machine types.
The most appropriate solution depends a lot on how/when management software needs to query this. If they only need to query it at runtime for a running VM, there's no reason for us to go of our way and add complexity just to make it look like static data in query-machines. On the other hand, if they really need to query it before configuring/starting a VM, it won't be useful at all to make it available only at runtime. Daniel, when/how exactly software would need to query the new flag?
Worse, a machine type property that is static for all machine types now could conceivably become dynamic when we add a machine type configuration knob.
This isn't the first time a machine capability that seems static actually depends on other configuration arguments. We will probably need to address this eventually.
Would a way to tie the property to the configuration knob help? Something like wakeup-suspend-support taking values true (supported), false (not supported), and "acpi" (supported if machine type configuration knob "acpi" is switched on).
I would prefer a more generic mechanism. Maybe make 'query-machines' accept a 'machine-options' argument? -- Eduardo
Hi, On 05/23/2018 09:27 AM, Eduardo Habkost wrote:
On Wed, May 23, 2018 at 11:17:55AM +0200, Markus Armbruster wrote:
Eduardo Habkost <ehabkost@redhat.com> writes:
On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote: [...]
Since no objection was made back then, this logic was put into query-target starting in v2. Still, I don't have any favorites though: query-target looks ok, query-machine looks ok and a new API looks ok too. It's all about what makes (more) sense in the management level, I think. I understand the original objection from Eric: having to add a new command for every runtime flag we want to expose to the user looks wrong to me. Agreed.
However, extending query-machines and query-target looks wrong too, however. query-target looks wrong because this not a property of the target. query-machines is wrong because this is not a static property of the machine-type, but of the running machine instance. Of the two, query-machines looks less wrong.
Arguably, -no-acpi should not exist. It's an ad hoc flag that sneakily splits a few machine types into two variants, with and without ACPI. It's silently ignored for other machine types, even APCI-capable ones.
If the machine type variants with and without ACPI were separate types, wakeup-suspend-support would be a static property of the machine type.
However, "separate types" probably doesn't scale: I'm afraid we'd end up with an undesirable number of machine types. Avoiding that is exactly why we have machine types with configurable options. I suspect that's how ACPI should be configured (if at all).
So, should we make -no-acpi sugar for a machine type parameter? And then deprecate -no-acpi for good measure? I think we should.
Can we have a new query command that could be an obvious container for simple machine capabilities that are not static? A name like "query-machine" would be generic enough for that, I guess. Having command names differ only in a single letter is awkward, but let's focus on things other than naming now, and use query-current-machine like a working title.
query-machines is wrong because wakeup-suspend-support isn't static for some machine types.
query-current-machine is also kind of wrong because wakeup-suspend-support *is* static for most machine types.
The most appropriate solution depends a lot on how/when management software needs to query this.
If they only need to query it at runtime for a running VM, there's no reason for us to go of our way and add complexity just to make it look like static data in query-machines.
On the other hand, if they really need to query it before configuring/starting a VM, it won't be useful at all to make it available only at runtime.
Daniel, when/how exactly software would need to query the new flag?
The original idea of this series was to provide a way to inform management when not to execute a pm-suspend* command. This is a command from the guest agent, thus it's only available when the guest is already running. As far as I know there is no way to suspend the VM without using the guest agent. Thus, unless management wants to store this state to avoid querying it multiple times during the VM lifetime (I remember from Libvirt code that it stores some sort of capabilities of the domain in an internal state, although I can't recall if this info would be eligible for that), there is no need to query this until the VM is booted.
Worse, a machine type property that is static for all machine types now could conceivably become dynamic when we add a machine type configuration knob.
This isn't the first time a machine capability that seems static actually depends on other configuration arguments. We will probably need to address this eventually.
Would a way to tie the property to the configuration knob help? Something like wakeup-suspend-support taking values true (supported), false (not supported), and "acpi" (supported if machine type configuration knob "acpi" is switched on).
I would prefer a more generic mechanism. Maybe make 'query-machines' accept a 'machine-options' argument?
Eduardo Habkost <ehabkost@redhat.com> writes:
On Wed, May 23, 2018 at 11:17:55AM +0200, Markus Armbruster wrote:
Eduardo Habkost <ehabkost@redhat.com> writes:
On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote: [...]
Since no objection was made back then, this logic was put into query-target starting in v2. Still, I don't have any favorites though: query-target looks ok, query-machine looks ok and a new API looks ok too. It's all about what makes (more) sense in the management level, I think.
I understand the original objection from Eric: having to add a new command for every runtime flag we want to expose to the user looks wrong to me.
Agreed.
However, extending query-machines and query-target looks wrong too, however. query-target looks wrong because this not a property of the target. query-machines is wrong because this is not a static property of the machine-type, but of the running machine instance.
Of the two, query-machines looks less wrong.
Arguably, -no-acpi should not exist. It's an ad hoc flag that sneakily splits a few machine types into two variants, with and without ACPI. It's silently ignored for other machine types, even APCI-capable ones.
If the machine type variants with and without ACPI were separate types, wakeup-suspend-support would be a static property of the machine type.
However, "separate types" probably doesn't scale: I'm afraid we'd end up with an undesirable number of machine types. Avoiding that is exactly why we have machine types with configurable options. I suspect that's how ACPI should be configured (if at all).
So, should we make -no-acpi sugar for a machine type parameter? And then deprecate -no-acpi for good measure?
I think we should.
Would you like to take care of it?
Can we have a new query command that could be an obvious container for simple machine capabilities that are not static? A name like "query-machine" would be generic enough for that, I guess.
Having command names differ only in a single letter is awkward, but let's focus on things other than naming now, and use query-current-machine like a working title.
query-machines is wrong because wakeup-suspend-support isn't static for some machine types.
query-current-machine is also kind of wrong because wakeup-suspend-support *is* static for most machine types.
The most appropriate solution depends a lot on how/when management software needs to query this.
Right.
If they only need to query it at runtime for a running VM, there's no reason for us to go of our way and add complexity just to make it look like static data in query-machines.
On the other hand, if they really need to query it before configuring/starting a VM, it won't be useful at all to make it available only at runtime.
Daniel, when/how exactly software would need to query the new flag?
Worse, a machine type property that is static for all machine types now could conceivably become dynamic when we add a machine type configuration knob.
This isn't the first time a machine capability that seems static actually depends on other configuration arguments. We will probably need to address this eventually.
Then the best time to address it is now, provided we can :)
Would a way to tie the property to the configuration knob help? Something like wakeup-suspend-support taking values true (supported), false (not supported), and "acpi" (supported if machine type configuration knob "acpi" is switched on).
I would prefer a more generic mechanism. Maybe make 'query-machines' accept a 'machine-options' argument?
This can support arbitrary configuration dependencies, unlike my proposal. However, I fear combinatorial explosion would make querying anything but "default configuration" and "current configuration" impractical, and "default configuration" would be basically useless, as you can't predict how arguments will affect the value query-machines. Whether this is an issue depends on how management software wants to use query-machines. Whether the ability to support arbitrary configuration dependencies is a useful feature or an invitation to stupid stunts is another open question :) Here's a synthesis of the two proposals: have query-machines spell out which of its results are determinate, and which configuration bits need to be supplied to resolve the indeterminate ones. For machine type "pc-q35-*", wakeup-suspend-support would always yield true, but for "pc-i440fx-*" it would return true when passed an acpi: true argument, false when passed an acpi: false argument, and an encoding of "indeterminate, you need to pass an acpi argument to learn more" when passed no acpi argument. I'm not saying this synthesis makes sense, I'm just exploring the design space. We need input from libvirt guys.
On Wed, May 23, 2018 at 05:53:34PM +0200, Markus Armbruster wrote:
Eduardo Habkost <ehabkost@redhat.com> writes:
On Wed, May 23, 2018 at 11:17:55AM +0200, Markus Armbruster wrote:
Eduardo Habkost <ehabkost@redhat.com> writes:
On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote: [...]
Since no objection was made back then, this logic was put into query-target starting in v2. Still, I don't have any favorites though: query-target looks ok, query-machine looks ok and a new API looks ok too. It's all about what makes (more) sense in the management level, I think.
I understand the original objection from Eric: having to add a new command for every runtime flag we want to expose to the user looks wrong to me.
Agreed.
However, extending query-machines and query-target looks wrong too, however. query-target looks wrong because this not a property of the target. query-machines is wrong because this is not a static property of the machine-type, but of the running machine instance.
Of the two, query-machines looks less wrong.
Arguably, -no-acpi should not exist. It's an ad hoc flag that sneakily splits a few machine types into two variants, with and without ACPI. It's silently ignored for other machine types, even APCI-capable ones.
If the machine type variants with and without ACPI were separate types, wakeup-suspend-support would be a static property of the machine type.
However, "separate types" probably doesn't scale: I'm afraid we'd end up with an undesirable number of machine types. Avoiding that is exactly why we have machine types with configurable options. I suspect that's how ACPI should be configured (if at all).
So, should we make -no-acpi sugar for a machine type parameter? And then deprecate -no-acpi for good measure?
I think we should.
Would you like to take care of it?
Adding to my TODO-list, I hope I will be able to do it before the next release.
[...]
This isn't the first time a machine capability that seems static actually depends on other configuration arguments. We will probably need to address this eventually.
Then the best time to address it is now, provided we can :)
I'm not sure this is the best time. If libvirt only needs the runtime value and don't need any info at query-machines time, I think support for this on query-machines will be left unused and they will only use the query-current-machine value. Just giving libvirt the runtime data it wants (query-current-machine) seems way better than requiring libvirt to interpret a set of rules and independently calculate something QEMU already knows.
Would a way to tie the property to the configuration knob help? Something like wakeup-suspend-support taking values true (supported), false (not supported), and "acpi" (supported if machine type configuration knob "acpi" is switched on).
I would prefer a more generic mechanism. Maybe make 'query-machines' accept a 'machine-options' argument?
This can support arbitrary configuration dependencies, unlike my proposal. However, I fear combinatorial explosion would make querying anything but "default configuration" and "current configuration" impractical, and "default configuration" would be basically useless, as you can't predict how arguments will affect the value query-machines.
Whether this is an issue depends on how management software wants to use query-machines.
Whether the ability to support arbitrary configuration dependencies is a useful feature or an invitation to stupid stunts is another open question :)
Here's a synthesis of the two proposals: have query-machines spell out which of its results are determinate, and which configuration bits need to be supplied to resolve the indeterminate ones. For machine type "pc-q35-*", wakeup-suspend-support would always yield true, but for "pc-i440fx-*" it would return true when passed an acpi: true argument, false when passed an acpi: false argument, and an encoding of "indeterminate, you need to pass an acpi argument to learn more" when passed no acpi argument.
I like this proposal for other query-machines fields (like bus information), but I think doing this for wakeup-suspend-support is overkill, based on Daniel's description of its intended usage.
I'm not saying this synthesis makes sense, I'm just exploring the design space.
We need input from libvirt guys.
I have the impression we need real use cases so they can evaluate the proposal. wakeup-suspend-support doesn't seem like a use case that really needs support on query-machines (because we can simply provide the data at runtime). -- Eduardo
Eduardo Habkost <ehabkost@redhat.com> writes:
On Wed, May 23, 2018 at 05:53:34PM +0200, Markus Armbruster wrote:
Eduardo Habkost <ehabkost@redhat.com> writes:
On Wed, May 23, 2018 at 11:17:55AM +0200, Markus Armbruster wrote:
Eduardo Habkost <ehabkost@redhat.com> writes:
On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote: [...]
Since no objection was made back then, this logic was put into query-target starting in v2. Still, I don't have any favorites though: query-target looks ok, query-machine looks ok and a new API looks ok too. It's all about what makes (more) sense in the management level, I think.
I understand the original objection from Eric: having to add a new command for every runtime flag we want to expose to the user looks wrong to me.
Agreed.
However, extending query-machines and query-target looks wrong too, however. query-target looks wrong because this not a property of the target. query-machines is wrong because this is not a static property of the machine-type, but of the running machine instance.
Of the two, query-machines looks less wrong.
Arguably, -no-acpi should not exist. It's an ad hoc flag that sneakily splits a few machine types into two variants, with and without ACPI. It's silently ignored for other machine types, even APCI-capable ones.
If the machine type variants with and without ACPI were separate types, wakeup-suspend-support would be a static property of the machine type.
However, "separate types" probably doesn't scale: I'm afraid we'd end up with an undesirable number of machine types. Avoiding that is exactly why we have machine types with configurable options. I suspect that's how ACPI should be configured (if at all).
So, should we make -no-acpi sugar for a machine type parameter? And then deprecate -no-acpi for good measure?
I think we should.
Would you like to take care of it?
Adding to my TODO-list, I hope I will be able to do it before the next release.
Thanks!
Can we have a new query command that could be an obvious container for simple machine capabilities that are not static? A name like "query-machine" would be generic enough for that, I guess.
Having command names differ only in a single letter is awkward, but let's focus on things other than naming now, and use query-current-machine like a working title.
query-machines is wrong because wakeup-suspend-support isn't static for some machine types.
query-current-machine is also kind of wrong because wakeup-suspend-support *is* static for most machine types.
The most appropriate solution depends a lot on how/when management software needs to query this.
Right.
If they only need to query it at runtime for a running VM, there's no reason for us to go of our way and add complexity just to make it look like static data in query-machines.
On the other hand, if they really need to query it before configuring/starting a VM, it won't be useful at all to make it available only at runtime.
Daniel, when/how exactly software would need to query the new flag?
Worse, a machine type property that is static for all machine types now could conceivably become dynamic when we add a machine type configuration knob.
This isn't the first time a machine capability that seems static actually depends on other configuration arguments. We will probably need to address this eventually.
Then the best time to address it is now, provided we can :)
I'm not sure this is the best time. If libvirt only needs the runtime value and don't need any info at query-machines time, I think support for this on query-machines will be left unused and they will only use the query-current-machine value.
Just giving libvirt the runtime data it wants (query-current-machine) seems way better than requiring libvirt to interpret a set of rules and independently calculate something QEMU already knows.
I wouldn't mind adding a query-current-machine to report dynamic machine capabilities if that helps QMP clients. query-machines could continue to report static machine capabilities then. However, we do need a plan on how to distribute machine capabilities between query-machines and query-current-machine, in particular how to handle changing staticness. wakeup-suspend-support is static for most machine types, but dynamic for some. Where should it go? It needs to go into query-current-machine when its dynamic with the current machine. It may go there just to keep things regular even if its static with the current machine. Does it go into query-machines, too? If not, clients lose the ability to examine all machines efficiently. Even if this isn't an issue for wakeup-suspend-support: are we sure this can't be an issue for any future capabilities? If it goes into query-machines, what should its value be for the machine types where it's dynamic? Should it be absent, perhaps, letting clients know they have to consult query-current-machine to find the value? What if a capability previously thought static becomes dynamic? We can add it to query-current-machine just fine, but removing it from query-machines would be a compatibility break. Making it optional, too. Should all new members of MachineInfo be optional, just in case? These are design questions we need to ponder *now*. Picking a solution that satisfies current needs while ignoring future needs has bitten us in the posterior time and again. We're not going to successfully predict *all* future needs, but not even trying should be easy to beat. That's what I meant by "the best time to address it is now". [...]
On Fri, May 25, 2018 at 08:30:59AM +0200, Markus Armbruster wrote:
Eduardo Habkost <ehabkost@redhat.com> writes:
On Wed, May 23, 2018 at 05:53:34PM +0200, Markus Armbruster wrote: [...]
Worse, a machine type property that is static for all machine types now could conceivably become dynamic when we add a machine type configuration knob.
This isn't the first time a machine capability that seems static actually depends on other configuration arguments. We will probably need to address this eventually.
Then the best time to address it is now, provided we can :)
I'm not sure this is the best time. If libvirt only needs the runtime value and don't need any info at query-machines time, I think support for this on query-machines will be left unused and they will only use the query-current-machine value.
Just giving libvirt the runtime data it wants (query-current-machine) seems way better than requiring libvirt to interpret a set of rules and independently calculate something QEMU already knows.
I wouldn't mind adding a query-current-machine to report dynamic machine capabilities if that helps QMP clients. query-machines could continue to report static machine capabilities then.
However, we do need a plan on how to distribute machine capabilities between query-machines and query-current-machine, in particular how to handle changing staticness.
Handling dynamic data that becomes static is easy: we can keep it on both.
wakeup-suspend-support is static for most machine types, but dynamic for some. Where should it go?
I think it obviously should go on query-current-machine. Maybe it can be added to query-machines in the future if it's deemed useful.
It needs to go into query-current-machine when its dynamic with the current machine. It may go there just to keep things regular even if its static with the current machine.
Does it go into query-machines, too? If not, clients lose the ability to examine all machines efficiently. Even if this isn't an issue for wakeup-suspend-support: are we sure this can't be an issue for any future capabilities?
I don't think this will be an issue for wakup-suspend-support, but this is already an issue for existing capabilities. See below[1].
If it goes into query-machines, what should its value be for the machine types where it's dynamic? Should it be absent, perhaps, letting clients know they have to consult query-current-machine to find the value?
What if a capability previously thought static becomes dynamic? We can add it to query-current-machine just fine, but removing it from query-machines would be a compatibility break. Making it optional, too. Should all new members of MachineInfo be optional, just in case?
The above are questions we must ponder if we are considering extending query-machines. We have been avoiding them for a few years, by simply not extending query-machines very often and letting libvirt hardcode machine-type info. :(
These are design questions we need to ponder *now*. Picking a solution that satisfies current needs while ignoring future needs has bitten us in the posterior time and again. We're not going to successfully predict *all* future needs, but not even trying should be easy to beat.
That's what I meant by "the best time to address it is now".
I agree. I just think there are countless other use cases that require extending query-machines today. I'd prefer to use one of them as a starting point for this design exercise, instead of wakeup-suspend-support. [1] Doing a: $ git grep 'STR.*machine, "' on libvirt source is enough to find some code demonstrating where query-machines is already lacking today: src/libxl/libxl_capabilities.c:583: if (STREQ(machine, "xenpv")) src/libxl/libxl_capabilities.c:729: if (STREQ(domCaps->machine, "xenfv")) src/libxl/libxl_driver.c:6379: if (STRNEQ(machine, "xenpv") && STRNEQ(machine, "xenfv")) { src/qemu/qemu_capabilities.c:2435: STREQ(def->os.machine, "ppce500")) src/qemu/qemu_capabilities.c:2439: STREQ(def->os.machine, "prep")) src/qemu/qemu_capabilities.c:2443: STREQ(def->os.machine, "bamboo")) src/qemu/qemu_capabilities.c:2446: if (STREQ(def->os.machine, "mpc8544ds")) src/qemu/qemu_capabilities.c:5376: STREQ(def->os.machine, "isapc"); src/qemu/qemu_command.c:3829: if (STRPREFIX(def->os.machine, "s390-virtio") && src/qemu/qemu_domain.c:2798: if (STREQ(def->os.machine, "isapc")) { src/qemu/qemu_domain.c:5009: if (STREQ(def->os.machine, "versatilepb")) src/qemu/qemu_domain.c:8222: return (STRPREFIX(machine, "pc-q35") || src/qemu/qemu_domain.c:8223: STREQ(machine, "q35")); src/qemu/qemu_domain.c:8237: return (STREQ(machine, "pc") || src/qemu/qemu_domain.c:8238: STRPREFIX(machine, "pc-0.") || src/qemu/qemu_domain.c:8239: STRPREFIX(machine, "pc-1.") || src/qemu/qemu_domain.c:8240: STRPREFIX(machine, "pc-i440") || src/qemu/qemu_domain.c:8241: STRPREFIX(machine, "rhel")); src/qemu/qemu_domain.c:8285: const char *p = STRSKIP(machine, "pc-q35-"); src/qemu/qemu_domain.c:8310: return STRPREFIX(machine, "s390-ccw"); src/qemu/qemu_domain.c:8329: if (STRNEQ(machine, "virt") && src/qemu/qemu_domain.c:8330: !STRPREFIX(machine, "virt-")) src/qemu/qemu_domain.c:8351: if (STRNEQ(machine, "pseries") && src/qemu/qemu_domain.c:8352: !STRPREFIX(machine, "pseries-")) src/qemu/qemu_domain.c:8564: STREQ(machine, "malta") || src/qemu/qemu_domain.c:8565: STREQ(machine, "sun4u") || src/qemu/qemu_domain.c:8566: STREQ(machine, "g3beige"); src/qemu/qemu_domain_address.c:467: if (!(STRPREFIX(def->os.machine, "vexpress-") || src/qemu/qemu_domain_address.c:2145: if (STREQ(def->os.machine, "versatilepb")) src/util/virarch.c:170: } else if (STREQ(ut.machine, "amd64")) { -- Eduardo
Eduardo Habkost <ehabkost@redhat.com> writes:
On Fri, May 25, 2018 at 08:30:59AM +0200, Markus Armbruster wrote:
Eduardo Habkost <ehabkost@redhat.com> writes:
On Wed, May 23, 2018 at 05:53:34PM +0200, Markus Armbruster wrote: [...]
Worse, a machine type property that is static for all machine types now could conceivably become dynamic when we add a machine type configuration knob.
This isn't the first time a machine capability that seems static actually depends on other configuration arguments. We will probably need to address this eventually.
Then the best time to address it is now, provided we can :)
I'm not sure this is the best time. If libvirt only needs the runtime value and don't need any info at query-machines time, I think support for this on query-machines will be left unused and they will only use the query-current-machine value.
Just giving libvirt the runtime data it wants (query-current-machine) seems way better than requiring libvirt to interpret a set of rules and independently calculate something QEMU already knows.
I wouldn't mind adding a query-current-machine to report dynamic machine capabilities if that helps QMP clients. query-machines could continue to report static machine capabilities then.
However, we do need a plan on how to distribute machine capabilities between query-machines and query-current-machine, in particular how to handle changing staticness.
Handling dynamic data that becomes static is easy: we can keep it on both.
The duplication is less than elegant, but backward-compatibility generally is.
wakeup-suspend-support is static for most machine types, but dynamic for some. Where should it go?
I think it obviously should go on query-current-machine. Maybe it can be added to query-machines in the future if it's deemed useful.
It needs to go into query-current-machine when its dynamic with the current machine. It may go there just to keep things regular even if its static with the current machine.
Does it go into query-machines, too? If not, clients lose the ability to examine all machines efficiently. Even if this isn't an issue for wakeup-suspend-support: are we sure this can't be an issue for any future capabilities?
I don't think this will be an issue for wakup-suspend-support, but this is already an issue for existing capabilities. See below[1].
If it goes into query-machines, what should its value be for the machine types where it's dynamic? Should it be absent, perhaps, letting clients know they have to consult query-current-machine to find the value?
What if a capability previously thought static becomes dynamic? We can add it to query-current-machine just fine, but removing it from query-machines would be a compatibility break. Making it optional, too. Should all new members of MachineInfo be optional, just in case?
The above are questions we must ponder if we are considering extending query-machines. We have been avoiding them for a few years, by simply not extending query-machines very often and letting libvirt hardcode machine-type info. :(
These are design questions we need to ponder *now*. Picking a solution that satisfies current needs while ignoring future needs has bitten us in the posterior time and again. We're not going to successfully predict *all* future needs, but not even trying should be easy to beat.
That's what I meant by "the best time to address it is now".
I agree. I just think there are countless other use cases that require extending query-machines today. I'd prefer to use one of them as a starting point for this design exercise, instead of wakeup-suspend-support.
Analyzing all the use cases we know makes sense.
[1] Doing a: $ git grep 'STR.*machine, "' on libvirt source is enough to find some code demonstrating where query-machines is already lacking today:
src/libxl/libxl_capabilities.c:583: if (STREQ(machine, "xenpv")) src/libxl/libxl_capabilities.c:729: if (STREQ(domCaps->machine, "xenfv")) src/libxl/libxl_driver.c:6379: if (STRNEQ(machine, "xenpv") && STRNEQ(machine, "xenfv")) { src/qemu/qemu_capabilities.c:2435: STREQ(def->os.machine, "ppce500")) src/qemu/qemu_capabilities.c:2439: STREQ(def->os.machine, "prep")) src/qemu/qemu_capabilities.c:2443: STREQ(def->os.machine, "bamboo")) src/qemu/qemu_capabilities.c:2446: if (STREQ(def->os.machine, "mpc8544ds")) src/qemu/qemu_capabilities.c:5376: STREQ(def->os.machine, "isapc"); src/qemu/qemu_command.c:3829: if (STRPREFIX(def->os.machine, "s390-virtio") && src/qemu/qemu_domain.c:2798: if (STREQ(def->os.machine, "isapc")) { src/qemu/qemu_domain.c:5009: if (STREQ(def->os.machine, "versatilepb")) src/qemu/qemu_domain.c:8222: return (STRPREFIX(machine, "pc-q35") || src/qemu/qemu_domain.c:8223: STREQ(machine, "q35")); src/qemu/qemu_domain.c:8237: return (STREQ(machine, "pc") || src/qemu/qemu_domain.c:8238: STRPREFIX(machine, "pc-0.") || src/qemu/qemu_domain.c:8239: STRPREFIX(machine, "pc-1.") || src/qemu/qemu_domain.c:8240: STRPREFIX(machine, "pc-i440") || src/qemu/qemu_domain.c:8241: STRPREFIX(machine, "rhel")); src/qemu/qemu_domain.c:8285: const char *p = STRSKIP(machine, "pc-q35-"); src/qemu/qemu_domain.c:8310: return STRPREFIX(machine, "s390-ccw"); src/qemu/qemu_domain.c:8329: if (STRNEQ(machine, "virt") && src/qemu/qemu_domain.c:8330: !STRPREFIX(machine, "virt-")) src/qemu/qemu_domain.c:8351: if (STRNEQ(machine, "pseries") && src/qemu/qemu_domain.c:8352: !STRPREFIX(machine, "pseries-")) src/qemu/qemu_domain.c:8564: STREQ(machine, "malta") || src/qemu/qemu_domain.c:8565: STREQ(machine, "sun4u") || src/qemu/qemu_domain.c:8566: STREQ(machine, "g3beige"); src/qemu/qemu_domain_address.c:467: if (!(STRPREFIX(def->os.machine, "vexpress-") || src/qemu/qemu_domain_address.c:2145: if (STREQ(def->os.machine, "versatilepb")) src/util/virarch.c:170: } else if (STREQ(ut.machine, "amd64")) {
How can we get from this grep to a list of static or dynamic machine type capabilties?
On Mon, May 28, 2018 at 09:23:54AM +0200, Markus Armbruster wrote:
Eduardo Habkost <ehabkost@redhat.com> writes: [...]
[1] Doing a: $ git grep 'STR.*machine, "' on libvirt source is enough to find some code demonstrating where query-machines is already lacking today: [...] How can we get from this grep to a list of static or dynamic machine type capabilties?
Let's look at the code: $ git grep -W 'STR.*machine, "' src/libxl/libxl_capabilities.c=libxlMakeDomainOSCaps(const char *machine, src/libxl/libxl_capabilities.c- virDomainCapsOSPtr os, src/libxl/libxl_capabilities.c- virFirmwarePtr *firmwares, src/libxl/libxl_capabilities.c- size_t nfirmwares) src/libxl/libxl_capabilities.c-{ src/libxl/libxl_capabilities.c- virDomainCapsLoaderPtr capsLoader = &os->loader; src/libxl/libxl_capabilities.c- size_t i; src/libxl/libxl_capabilities.c- src/libxl/libxl_capabilities.c- os->supported = true; src/libxl/libxl_capabilities.c- src/libxl/libxl_capabilities.c: if (STREQ(machine, "xenpv")) src/libxl/libxl_capabilities.c- return 0; I don't understand why this one is here, but we can find out what we could add to query-machines to make this unnecessary. [...] -- src/libxl/libxl_capabilities.c=libxlMakeDomainCapabilities(virDomainCapsPtr domCaps, src/libxl/libxl_capabilities.c- virFirmwarePtr *firmwares, src/libxl/libxl_capabilities.c- size_t nfirmwares) src/libxl/libxl_capabilities.c-{ src/libxl/libxl_capabilities.c- virDomainCapsOSPtr os = &domCaps->os; src/libxl/libxl_capabilities.c- virDomainCapsDeviceDiskPtr disk = &domCaps->disk; src/libxl/libxl_capabilities.c- virDomainCapsDeviceGraphicsPtr graphics = &domCaps->graphics; src/libxl/libxl_capabilities.c- virDomainCapsDeviceVideoPtr video = &domCaps->video; src/libxl/libxl_capabilities.c- virDomainCapsDeviceHostdevPtr hostdev = &domCaps->hostdev; src/libxl/libxl_capabilities.c- src/libxl/libxl_capabilities.c: if (STREQ(domCaps->machine, "xenfv")) src/libxl/libxl_capabilities.c- domCaps->maxvcpus = HVM_MAX_VCPUS; src/libxl/libxl_capabilities.c- else src/libxl/libxl_capabilities.c- domCaps->maxvcpus = PV_MAX_VCPUS; Looks like libvirt isn't using MachineInfo::cpu-max. libvirt bug, or workaround for QEMU limitation? [...] -- src/libxl/libxl_driver.c=libxlConnectGetDomainCapabilities(virConnectPtr conn, src/libxl/libxl_driver.c- const char *emulatorbin, src/libxl/libxl_driver.c- const char *arch_str, src/libxl/libxl_driver.c- const char *machine, src/libxl/libxl_driver.c- const char *virttype_str, src/libxl/libxl_driver.c- unsigned int flags) src/libxl/libxl_driver.c-{ [...] src/libxl/libxl_driver.c- if (machine) { src/libxl/libxl_driver.c: if (STRNEQ(machine, "xenpv") && STRNEQ(machine, "xenfv")) { src/libxl/libxl_driver.c- virReportError(VIR_ERR_INVALID_ARG, "%s", src/libxl/libxl_driver.c- _("Xen only supports 'xenpv' and 'xenfv' machines")); Not sure if this should be encoded in QEMU. accel=xen works with other PC machines, doesn't it? [...] -- src/qemu/qemu_capabilities.c=bool virQEMUCapsHasPCIMultiBus(virQEMUCapsPtr qemuCaps, src/qemu/qemu_capabilities.c- const virDomainDef *def) src/qemu/qemu_capabilities.c-{ src/qemu/qemu_capabilities.c- /* x86_64 and i686 support PCI-multibus on all machine types src/qemu/qemu_capabilities.c- * since forever */ src/qemu/qemu_capabilities.c- if (ARCH_IS_X86(def->os.arch)) src/qemu/qemu_capabilities.c- return true; src/qemu/qemu_capabilities.c- src/qemu/qemu_capabilities.c- if (def->os.arch == VIR_ARCH_PPC || src/qemu/qemu_capabilities.c- ARCH_IS_PPC64(def->os.arch)) { src/qemu/qemu_capabilities.c- /* src/qemu/qemu_capabilities.c- * Usage of pci.0 naming: src/qemu/qemu_capabilities.c- * src/qemu/qemu_capabilities.c- * ref405ep: no pci src/qemu/qemu_capabilities.c- * taihu: no pci src/qemu/qemu_capabilities.c- * bamboo: 1.1.0 src/qemu/qemu_capabilities.c- * mac99: 2.0.0 src/qemu/qemu_capabilities.c- * g3beige: 2.0.0 src/qemu/qemu_capabilities.c- * prep: 1.4.0 src/qemu/qemu_capabilities.c- * pseries: 2.0.0 src/qemu/qemu_capabilities.c- * mpc8544ds: forever src/qemu/qemu_capabilities.c- * virtex-m507: no pci src/qemu/qemu_capabilities.c- * ppce500: 1.6.0 src/qemu/qemu_capabilities.c- */ src/qemu/qemu_capabilities.c- src/qemu/qemu_capabilities.c- /* We do not store the qemu version in domain status XML. src/qemu/qemu_capabilities.c- * Hope the user is using a QEMU new enough to use 'pci.0', src/qemu/qemu_capabilities.c- * otherwise the results of this function will be wrong src/qemu/qemu_capabilities.c- * for domains already running at the time of daemon src/qemu/qemu_capabilities.c- * restart */ src/qemu/qemu_capabilities.c- if (qemuCaps->version == 0) src/qemu/qemu_capabilities.c- return true; src/qemu/qemu_capabilities.c- src/qemu/qemu_capabilities.c- if (qemuCaps->version >= 2000000) src/qemu/qemu_capabilities.c- return true; src/qemu/qemu_capabilities.c- src/qemu/qemu_capabilities.c- if (qemuCaps->version >= 1006000 && src/qemu/qemu_capabilities.c: STREQ(def->os.machine, "ppce500")) src/qemu/qemu_capabilities.c- return true; src/qemu/qemu_capabilities.c- src/qemu/qemu_capabilities.c- if (qemuCaps->version >= 1004000 && src/qemu/qemu_capabilities.c: STREQ(def->os.machine, "prep")) src/qemu/qemu_capabilities.c- return true; src/qemu/qemu_capabilities.c- src/qemu/qemu_capabilities.c- if (qemuCaps->version >= 1001000 && src/qemu/qemu_capabilities.c: STREQ(def->os.machine, "bamboo")) src/qemu/qemu_capabilities.c- return true; src/qemu/qemu_capabilities.c- src/qemu/qemu_capabilities.c: if (STREQ(def->os.machine, "mpc8544ds")) src/qemu/qemu_capabilities.c- return true; This is due to the lack of bus information on query-machines. [...] src/qemu/qemu_capabilities.c- src/qemu/qemu_capabilities.c- return false; src/qemu/qemu_capabilities.c- } src/qemu/qemu_capabilities.c- src/qemu/qemu_capabilities.c- /* If 'virt' supports PCI, it supports multibus. src/qemu/qemu_capabilities.c- * No extra conditions here for simplicity. src/qemu/qemu_capabilities.c- */ src/qemu/qemu_capabilities.c- if (qemuDomainIsVirt(def)) src/qemu/qemu_capabilities.c- return true; src/qemu/qemu_capabilities.c- src/qemu/qemu_capabilities.c- return false; src/qemu/qemu_capabilities.c-} -- src/qemu/qemu_capabilities.c=virQEMUCapsSupportsVmport(virQEMUCapsPtr qemuCaps, src/qemu/qemu_capabilities.c- const virDomainDef *def) src/qemu/qemu_capabilities.c-{ src/qemu/qemu_capabilities.c- if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_VMPORT_OPT)) src/qemu/qemu_capabilities.c- return false; src/qemu/qemu_capabilities.c- src/qemu/qemu_capabilities.c- return qemuDomainIsI440FX(def) || src/qemu/qemu_capabilities.c- qemuDomainIsQ35(def) || src/qemu/qemu_capabilities.c: STREQ(def->os.machine, "isapc"); src/qemu/qemu_capabilities.c-} Lack of bus information on query-machines? -- src/qemu/qemu_command.c=qemuBuildMemballoonCommandLine(virCommandPtr cmd, src/qemu/qemu_command.c- const virDomainDef *def, src/qemu/qemu_command.c- virQEMUCapsPtr qemuCaps) src/qemu/qemu_command.c-{ src/qemu/qemu_command.c- virBuffer buf = VIR_BUFFER_INITIALIZER; src/qemu/qemu_command.c- src/qemu/qemu_command.c: if (STRPREFIX(def->os.machine, "s390-virtio") && src/qemu/qemu_command.c- virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390) && def->memballoon) src/qemu/qemu_command.c- def->memballoon->model = VIR_DOMAIN_MEMBALLOON_MODEL_NONE; Lack of bus information on query-machines? [...] -- src/qemu/qemu_domain.c=qemuDomainDefAddDefaultDevices(virDomainDefPtr def, src/qemu/qemu_domain.c- virQEMUCapsPtr qemuCaps) src/qemu/qemu_domain.c-{ [...] src/qemu/qemu_domain.c: if (STREQ(def->os.machine, "isapc")) { src/qemu/qemu_domain.c- addDefaultUSB = false; Lack of bus/defaults information on query-machines? This whole function is a collection of cases where bus/device/defaults information is missing on query-machines: src/qemu/qemu_domain.c- break; src/qemu/qemu_domain.c- } src/qemu/qemu_domain.c- if (qemuDomainIsQ35(def)) { src/qemu/qemu_domain.c- addPCIeRoot = true; src/qemu/qemu_domain.c- addImplicitSATA = true; [...] src/qemu/qemu_domain.c- if (qemuDomainIsI440FX(def)) src/qemu/qemu_domain.c- addPCIRoot = true; [...] src/qemu/qemu_domain.c- break; src/qemu/qemu_domain.c- src/qemu/qemu_domain.c- case VIR_ARCH_ARMV7L: src/qemu/qemu_domain.c- case VIR_ARCH_AARCH64: src/qemu/qemu_domain.c- addDefaultUSB = false; src/qemu/qemu_domain.c- addDefaultMemballoon = false; src/qemu/qemu_domain.c- if (qemuDomainIsVirt(def)) src/qemu/qemu_domain.c- addPCIeRoot = virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_GPEX); src/qemu/qemu_domain.c- break; src/qemu/qemu_domain.c- src/qemu/qemu_domain.c- case VIR_ARCH_PPC64: src/qemu/qemu_domain.c- case VIR_ARCH_PPC64LE: src/qemu/qemu_domain.c- addPCIRoot = true; src/qemu/qemu_domain.c- addDefaultUSBKBD = true; src/qemu/qemu_domain.c- addDefaultUSBMouse = true; src/qemu/qemu_domain.c- /* For pSeries guests, the firmware provides the same src/qemu/qemu_domain.c- * functionality as the pvpanic device, so automatically src/qemu/qemu_domain.c- * add the definition if not already present */ src/qemu/qemu_domain.c- if (qemuDomainIsPSeries(def)) src/qemu/qemu_domain.c- addPanicDevice = true; src/qemu/qemu_domain.c- break; [...] -- src/qemu/qemu_domain.c=qemuDomainDefaultNetModel(const virDomainDef *def, src/qemu/qemu_domain.c- virQEMUCapsPtr qemuCaps) src/qemu/qemu_domain.c-{ src/qemu/qemu_domain.c- if (ARCH_IS_S390(def->os.arch)) src/qemu/qemu_domain.c- return "virtio"; src/qemu/qemu_domain.c- src/qemu/qemu_domain.c- if (def->os.arch == VIR_ARCH_ARMV7L || src/qemu/qemu_domain.c- def->os.arch == VIR_ARCH_AARCH64) { src/qemu/qemu_domain.c: if (STREQ(def->os.machine, "versatilepb")) src/qemu/qemu_domain.c- return "smc91c111"; src/qemu/qemu_domain.c- src/qemu/qemu_domain.c- if (qemuDomainIsVirt(def)) src/qemu/qemu_domain.c- return "virtio"; src/qemu/qemu_domain.c- src/qemu/qemu_domain.c- /* Incomplete. vexpress (and a few others) use this, but not all src/qemu/qemu_domain.c- * arm boards */ src/qemu/qemu_domain.c- return "lan9118"; Lack of bus/defaults information on query-machines? [...] -- src/qemu/qemu_domain.c=qemuDomainMachineIsQ35(const char *machine) src/qemu/qemu_domain.c-{ src/qemu/qemu_domain.c: return (STRPREFIX(machine, "pc-q35") || src/qemu/qemu_domain.c: STREQ(machine, "q35")); src/qemu/qemu_domain.c-} Need to grep for callers of this function. -- src/qemu/qemu_domain.c=qemuDomainMachineIsI440FX(const char *machine) src/qemu/qemu_domain.c-{ src/qemu/qemu_domain.c: return (STREQ(machine, "pc") || src/qemu/qemu_domain.c: STRPREFIX(machine, "pc-0.") || src/qemu/qemu_domain.c: STRPREFIX(machine, "pc-1.") || src/qemu/qemu_domain.c: STRPREFIX(machine, "pc-i440") || src/qemu/qemu_domain.c: STRPREFIX(machine, "rhel")); src/qemu/qemu_domain.c-} Need to grep for callers of this function. --- src/qemu/qemu_domain.c=qemuDomainMachineNeedsFDC(const char *machine) src/qemu/qemu_domain.c-{ src/qemu/qemu_domain.c: const char *p = STRSKIP(machine, "pc-q35-"); src/qemu/qemu_domain.c- src/qemu/qemu_domain.c- if (p) { src/qemu/qemu_domain.c- if (STRPREFIX(p, "1.") || src/qemu/qemu_domain.c- STRPREFIX(p, "2.0") || src/qemu/qemu_domain.c- STRPREFIX(p, "2.1") || src/qemu/qemu_domain.c- STRPREFIX(p, "2.2") || src/qemu/qemu_domain.c- STRPREFIX(p, "2.3")) src/qemu/qemu_domain.c- return false; src/qemu/qemu_domain.c- return true; src/qemu/qemu_domain.c- } src/qemu/qemu_domain.c- return false; src/qemu/qemu_domain.c-} Lack of bus/defaults information on query-machines. -- src/qemu/qemu_domain.c=qemuDomainMachineIsS390CCW(const char *machine) src/qemu/qemu_domain.c-{ src/qemu/qemu_domain.c: return STRPREFIX(machine, "s390-ccw"); src/qemu/qemu_domain.c-} Need to grep for callers of this function. -- src/qemu/qemu_domain.c=qemuDomainMachineIsVirt(const char *machine, src/qemu/qemu_domain.c- const virArch arch) src/qemu/qemu_domain.c-{ src/qemu/qemu_domain.c- if (arch != VIR_ARCH_ARMV7L && src/qemu/qemu_domain.c- arch != VIR_ARCH_AARCH64) src/qemu/qemu_domain.c- return false; src/qemu/qemu_domain.c- src/qemu/qemu_domain.c: if (STRNEQ(machine, "virt") && src/qemu/qemu_domain.c: !STRPREFIX(machine, "virt-")) src/qemu/qemu_domain.c- return false; src/qemu/qemu_domain.c- src/qemu/qemu_domain.c- return true; src/qemu/qemu_domain.c-} Need to grep for callers of this function. -- src/qemu/qemu_domain.c=qemuDomainMachineIsPSeries(const char *machine, src/qemu/qemu_domain.c- const virArch arch) src/qemu/qemu_domain.c-{ src/qemu/qemu_domain.c- if (!ARCH_IS_PPC64(arch)) src/qemu/qemu_domain.c- return false; src/qemu/qemu_domain.c- src/qemu/qemu_domain.c: if (STRNEQ(machine, "pseries") && src/qemu/qemu_domain.c: !STRPREFIX(machine, "pseries-")) src/qemu/qemu_domain.c- return false; src/qemu/qemu_domain.c- src/qemu/qemu_domain.c- return true; src/qemu/qemu_domain.c-} Need to grep for callers of this function. -- src/qemu/qemu_domain.c=qemuDomainMachineHasBuiltinIDE(const char *machine) src/qemu/qemu_domain.c-{ src/qemu/qemu_domain.c- return qemuDomainMachineIsI440FX(machine) || src/qemu/qemu_domain.c: STREQ(machine, "malta") || src/qemu/qemu_domain.c: STREQ(machine, "sun4u") || src/qemu/qemu_domain.c: STREQ(machine, "g3beige"); src/qemu/qemu_domain.c-} Lack of bus/defaults information on query-machines. [...] -- src/qemu/qemu_domain_address.c=qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def, src/qemu/qemu_domain_address.c- virQEMUCapsPtr qemuCaps) src/qemu/qemu_domain_address.c-{ src/qemu/qemu_domain_address.c- if (def->os.arch != VIR_ARCH_ARMV7L && src/qemu/qemu_domain_address.c- def->os.arch != VIR_ARCH_AARCH64) src/qemu/qemu_domain_address.c- return; src/qemu/qemu_domain_address.c- src/qemu/qemu_domain_address.c: if (!(STRPREFIX(def->os.machine, "vexpress-") || src/qemu/qemu_domain_address.c- qemuDomainIsVirt(def))) src/qemu/qemu_domain_address.c- return; Lack of bus/device/defaults information on query-machines? [...] -- src/qemu/qemu_domain_address.c=qemuDomainSupportsPCI(virDomainDefPtr def, src/qemu/qemu_domain_address.c- virQEMUCapsPtr qemuCaps) src/qemu/qemu_domain_address.c-{ src/qemu/qemu_domain_address.c- if ((def->os.arch != VIR_ARCH_ARMV7L) && (def->os.arch != VIR_ARCH_AARCH64)) src/qemu/qemu_domain_address.c- return true; src/qemu/qemu_domain_address.c- src/qemu/qemu_domain_address.c: if (STREQ(def->os.machine, "versatilepb")) src/qemu/qemu_domain_address.c- return true; src/qemu/qemu_domain_address.c- src/qemu/qemu_domain_address.c- if (qemuDomainIsVirt(def) && src/qemu/qemu_domain_address.c- virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_GPEX)) src/qemu/qemu_domain_address.c- return true; src/qemu/qemu_domain_address.c- Lack of bus information on query-machines. [...] -- Eduardo
Hi, Sorry for the delay. I'll summarize what I've understood from the discussion so far: - query-target is the wrong place for this flag. query-machines is (less) wrong because it is not a static property of the machine object - a new "query-current-machine" can be created to host these dynamic properties that belongs to the current instance of the VM - there are machines in which the suspend support may vary with a "-no-acpi" option that would disable both the suspend and wake-up support. In this case, I see no problem into counting this flag into the logic (assuming it is possible, of course) and setting it as "false" if there is -no-acpi present (or even making the API returning "yes", "no" or "acpi" like Markus suggested) somewhere. Based on the last email from Eduardo, apparently there is a handful of other machine properties that can be hosted in either this new query-current-machine API or query-machines. I believe that this is more of a long term goal, but this new query-current-machine API would be a good kick-off and we should go for it. Is this a fair understanding? Did I miss something? Thanks, Daniel On 05/29/2018 11:55 AM, Eduardo Habkost wrote:
On Mon, May 28, 2018 at 09:23:54AM +0200, Markus Armbruster wrote:
Eduardo Habkost <ehabkost@redhat.com> writes: [...]
[1] Doing a: $ git grep 'STR.*machine, "' on libvirt source is enough to find some code demonstrating where query-machines is already lacking today: [...] How can we get from this grep to a list of static or dynamic machine type capabilties? Let's look at the code:
$ git grep -W 'STR.*machine, "' src/libxl/libxl_capabilities.c=libxlMakeDomainOSCaps(const char *machine, src/libxl/libxl_capabilities.c- virDomainCapsOSPtr os, src/libxl/libxl_capabilities.c- virFirmwarePtr *firmwares, src/libxl/libxl_capabilities.c- size_t nfirmwares) src/libxl/libxl_capabilities.c-{ src/libxl/libxl_capabilities.c- virDomainCapsLoaderPtr capsLoader = &os->loader; src/libxl/libxl_capabilities.c- size_t i; src/libxl/libxl_capabilities.c- src/libxl/libxl_capabilities.c- os->supported = true; src/libxl/libxl_capabilities.c- src/libxl/libxl_capabilities.c: if (STREQ(machine, "xenpv")) src/libxl/libxl_capabilities.c- return 0;
I don't understand why this one is here, but we can find out what we could add to query-machines to make this unnecessary.
[...] -- src/libxl/libxl_capabilities.c=libxlMakeDomainCapabilities(virDomainCapsPtr domCaps, src/libxl/libxl_capabilities.c- virFirmwarePtr *firmwares, src/libxl/libxl_capabilities.c- size_t nfirmwares) src/libxl/libxl_capabilities.c-{ src/libxl/libxl_capabilities.c- virDomainCapsOSPtr os = &domCaps->os; src/libxl/libxl_capabilities.c- virDomainCapsDeviceDiskPtr disk = &domCaps->disk; src/libxl/libxl_capabilities.c- virDomainCapsDeviceGraphicsPtr graphics = &domCaps->graphics; src/libxl/libxl_capabilities.c- virDomainCapsDeviceVideoPtr video = &domCaps->video; src/libxl/libxl_capabilities.c- virDomainCapsDeviceHostdevPtr hostdev = &domCaps->hostdev; src/libxl/libxl_capabilities.c- src/libxl/libxl_capabilities.c: if (STREQ(domCaps->machine, "xenfv")) src/libxl/libxl_capabilities.c- domCaps->maxvcpus = HVM_MAX_VCPUS; src/libxl/libxl_capabilities.c- else src/libxl/libxl_capabilities.c- domCaps->maxvcpus = PV_MAX_VCPUS;
Looks like libvirt isn't using MachineInfo::cpu-max. libvirt bug, or workaround for QEMU limitation?
[...] -- src/libxl/libxl_driver.c=libxlConnectGetDomainCapabilities(virConnectPtr conn, src/libxl/libxl_driver.c- const char *emulatorbin, src/libxl/libxl_driver.c- const char *arch_str, src/libxl/libxl_driver.c- const char *machine, src/libxl/libxl_driver.c- const char *virttype_str, src/libxl/libxl_driver.c- unsigned int flags) src/libxl/libxl_driver.c-{ [...] src/libxl/libxl_driver.c- if (machine) { src/libxl/libxl_driver.c: if (STRNEQ(machine, "xenpv") && STRNEQ(machine, "xenfv")) { src/libxl/libxl_driver.c- virReportError(VIR_ERR_INVALID_ARG, "%s", src/libxl/libxl_driver.c- _("Xen only supports 'xenpv' and 'xenfv' machines"));
Not sure if this should be encoded in QEMU. accel=xen works with other PC machines, doesn't it?
[...] -- src/qemu/qemu_capabilities.c=bool virQEMUCapsHasPCIMultiBus(virQEMUCapsPtr qemuCaps, src/qemu/qemu_capabilities.c- const virDomainDef *def) src/qemu/qemu_capabilities.c-{ src/qemu/qemu_capabilities.c- /* x86_64 and i686 support PCI-multibus on all machine types src/qemu/qemu_capabilities.c- * since forever */ src/qemu/qemu_capabilities.c- if (ARCH_IS_X86(def->os.arch)) src/qemu/qemu_capabilities.c- return true; src/qemu/qemu_capabilities.c- src/qemu/qemu_capabilities.c- if (def->os.arch == VIR_ARCH_PPC || src/qemu/qemu_capabilities.c- ARCH_IS_PPC64(def->os.arch)) { src/qemu/qemu_capabilities.c- /* src/qemu/qemu_capabilities.c- * Usage of pci.0 naming: src/qemu/qemu_capabilities.c- * src/qemu/qemu_capabilities.c- * ref405ep: no pci src/qemu/qemu_capabilities.c- * taihu: no pci src/qemu/qemu_capabilities.c- * bamboo: 1.1.0 src/qemu/qemu_capabilities.c- * mac99: 2.0.0 src/qemu/qemu_capabilities.c- * g3beige: 2.0.0 src/qemu/qemu_capabilities.c- * prep: 1.4.0 src/qemu/qemu_capabilities.c- * pseries: 2.0.0 src/qemu/qemu_capabilities.c- * mpc8544ds: forever src/qemu/qemu_capabilities.c- * virtex-m507: no pci src/qemu/qemu_capabilities.c- * ppce500: 1.6.0 src/qemu/qemu_capabilities.c- */ src/qemu/qemu_capabilities.c- src/qemu/qemu_capabilities.c- /* We do not store the qemu version in domain status XML. src/qemu/qemu_capabilities.c- * Hope the user is using a QEMU new enough to use 'pci.0', src/qemu/qemu_capabilities.c- * otherwise the results of this function will be wrong src/qemu/qemu_capabilities.c- * for domains already running at the time of daemon src/qemu/qemu_capabilities.c- * restart */ src/qemu/qemu_capabilities.c- if (qemuCaps->version == 0) src/qemu/qemu_capabilities.c- return true; src/qemu/qemu_capabilities.c- src/qemu/qemu_capabilities.c- if (qemuCaps->version >= 2000000) src/qemu/qemu_capabilities.c- return true; src/qemu/qemu_capabilities.c- src/qemu/qemu_capabilities.c- if (qemuCaps->version >= 1006000 && src/qemu/qemu_capabilities.c: STREQ(def->os.machine, "ppce500")) src/qemu/qemu_capabilities.c- return true; src/qemu/qemu_capabilities.c- src/qemu/qemu_capabilities.c- if (qemuCaps->version >= 1004000 && src/qemu/qemu_capabilities.c: STREQ(def->os.machine, "prep")) src/qemu/qemu_capabilities.c- return true; src/qemu/qemu_capabilities.c- src/qemu/qemu_capabilities.c- if (qemuCaps->version >= 1001000 && src/qemu/qemu_capabilities.c: STREQ(def->os.machine, "bamboo")) src/qemu/qemu_capabilities.c- return true; src/qemu/qemu_capabilities.c- src/qemu/qemu_capabilities.c: if (STREQ(def->os.machine, "mpc8544ds")) src/qemu/qemu_capabilities.c- return true;
This is due to the lack of bus information on query-machines.
[...] src/qemu/qemu_capabilities.c- src/qemu/qemu_capabilities.c- return false; src/qemu/qemu_capabilities.c- } src/qemu/qemu_capabilities.c- src/qemu/qemu_capabilities.c- /* If 'virt' supports PCI, it supports multibus. src/qemu/qemu_capabilities.c- * No extra conditions here for simplicity. src/qemu/qemu_capabilities.c- */ src/qemu/qemu_capabilities.c- if (qemuDomainIsVirt(def)) src/qemu/qemu_capabilities.c- return true; src/qemu/qemu_capabilities.c- src/qemu/qemu_capabilities.c- return false; src/qemu/qemu_capabilities.c-} -- src/qemu/qemu_capabilities.c=virQEMUCapsSupportsVmport(virQEMUCapsPtr qemuCaps, src/qemu/qemu_capabilities.c- const virDomainDef *def) src/qemu/qemu_capabilities.c-{ src/qemu/qemu_capabilities.c- if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_VMPORT_OPT)) src/qemu/qemu_capabilities.c- return false; src/qemu/qemu_capabilities.c- src/qemu/qemu_capabilities.c- return qemuDomainIsI440FX(def) || src/qemu/qemu_capabilities.c- qemuDomainIsQ35(def) || src/qemu/qemu_capabilities.c: STREQ(def->os.machine, "isapc"); src/qemu/qemu_capabilities.c-}
Lack of bus information on query-machines?
-- src/qemu/qemu_command.c=qemuBuildMemballoonCommandLine(virCommandPtr cmd, src/qemu/qemu_command.c- const virDomainDef *def, src/qemu/qemu_command.c- virQEMUCapsPtr qemuCaps) src/qemu/qemu_command.c-{ src/qemu/qemu_command.c- virBuffer buf = VIR_BUFFER_INITIALIZER; src/qemu/qemu_command.c- src/qemu/qemu_command.c: if (STRPREFIX(def->os.machine, "s390-virtio") && src/qemu/qemu_command.c- virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390) && def->memballoon) src/qemu/qemu_command.c- def->memballoon->model = VIR_DOMAIN_MEMBALLOON_MODEL_NONE;
Lack of bus information on query-machines?
[...] -- src/qemu/qemu_domain.c=qemuDomainDefAddDefaultDevices(virDomainDefPtr def, src/qemu/qemu_domain.c- virQEMUCapsPtr qemuCaps) src/qemu/qemu_domain.c-{ [...] src/qemu/qemu_domain.c: if (STREQ(def->os.machine, "isapc")) { src/qemu/qemu_domain.c- addDefaultUSB = false;
Lack of bus/defaults information on query-machines?
This whole function is a collection of cases where bus/device/defaults information is missing on query-machines:
src/qemu/qemu_domain.c- break; src/qemu/qemu_domain.c- } src/qemu/qemu_domain.c- if (qemuDomainIsQ35(def)) { src/qemu/qemu_domain.c- addPCIeRoot = true; src/qemu/qemu_domain.c- addImplicitSATA = true; [...] src/qemu/qemu_domain.c- if (qemuDomainIsI440FX(def)) src/qemu/qemu_domain.c- addPCIRoot = true; [...] src/qemu/qemu_domain.c- break; src/qemu/qemu_domain.c- src/qemu/qemu_domain.c- case VIR_ARCH_ARMV7L: src/qemu/qemu_domain.c- case VIR_ARCH_AARCH64: src/qemu/qemu_domain.c- addDefaultUSB = false; src/qemu/qemu_domain.c- addDefaultMemballoon = false; src/qemu/qemu_domain.c- if (qemuDomainIsVirt(def)) src/qemu/qemu_domain.c- addPCIeRoot = virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_GPEX); src/qemu/qemu_domain.c- break; src/qemu/qemu_domain.c- src/qemu/qemu_domain.c- case VIR_ARCH_PPC64: src/qemu/qemu_domain.c- case VIR_ARCH_PPC64LE: src/qemu/qemu_domain.c- addPCIRoot = true; src/qemu/qemu_domain.c- addDefaultUSBKBD = true; src/qemu/qemu_domain.c- addDefaultUSBMouse = true; src/qemu/qemu_domain.c- /* For pSeries guests, the firmware provides the same src/qemu/qemu_domain.c- * functionality as the pvpanic device, so automatically src/qemu/qemu_domain.c- * add the definition if not already present */ src/qemu/qemu_domain.c- if (qemuDomainIsPSeries(def)) src/qemu/qemu_domain.c- addPanicDevice = true; src/qemu/qemu_domain.c- break; [...] -- src/qemu/qemu_domain.c=qemuDomainDefaultNetModel(const virDomainDef *def, src/qemu/qemu_domain.c- virQEMUCapsPtr qemuCaps) src/qemu/qemu_domain.c-{ src/qemu/qemu_domain.c- if (ARCH_IS_S390(def->os.arch)) src/qemu/qemu_domain.c- return "virtio"; src/qemu/qemu_domain.c- src/qemu/qemu_domain.c- if (def->os.arch == VIR_ARCH_ARMV7L || src/qemu/qemu_domain.c- def->os.arch == VIR_ARCH_AARCH64) { src/qemu/qemu_domain.c: if (STREQ(def->os.machine, "versatilepb")) src/qemu/qemu_domain.c- return "smc91c111"; src/qemu/qemu_domain.c- src/qemu/qemu_domain.c- if (qemuDomainIsVirt(def)) src/qemu/qemu_domain.c- return "virtio"; src/qemu/qemu_domain.c- src/qemu/qemu_domain.c- /* Incomplete. vexpress (and a few others) use this, but not all src/qemu/qemu_domain.c- * arm boards */ src/qemu/qemu_domain.c- return "lan9118";
Lack of bus/defaults information on query-machines?
[...] -- src/qemu/qemu_domain.c=qemuDomainMachineIsQ35(const char *machine) src/qemu/qemu_domain.c-{ src/qemu/qemu_domain.c: return (STRPREFIX(machine, "pc-q35") || src/qemu/qemu_domain.c: STREQ(machine, "q35")); src/qemu/qemu_domain.c-}
Need to grep for callers of this function.
-- src/qemu/qemu_domain.c=qemuDomainMachineIsI440FX(const char *machine) src/qemu/qemu_domain.c-{ src/qemu/qemu_domain.c: return (STREQ(machine, "pc") || src/qemu/qemu_domain.c: STRPREFIX(machine, "pc-0.") || src/qemu/qemu_domain.c: STRPREFIX(machine, "pc-1.") || src/qemu/qemu_domain.c: STRPREFIX(machine, "pc-i440") || src/qemu/qemu_domain.c: STRPREFIX(machine, "rhel")); src/qemu/qemu_domain.c-}
Need to grep for callers of this function.
--- src/qemu/qemu_domain.c=qemuDomainMachineNeedsFDC(const char *machine) src/qemu/qemu_domain.c-{ src/qemu/qemu_domain.c: const char *p = STRSKIP(machine, "pc-q35-"); src/qemu/qemu_domain.c- src/qemu/qemu_domain.c- if (p) { src/qemu/qemu_domain.c- if (STRPREFIX(p, "1.") || src/qemu/qemu_domain.c- STRPREFIX(p, "2.0") || src/qemu/qemu_domain.c- STRPREFIX(p, "2.1") || src/qemu/qemu_domain.c- STRPREFIX(p, "2.2") || src/qemu/qemu_domain.c- STRPREFIX(p, "2.3")) src/qemu/qemu_domain.c- return false; src/qemu/qemu_domain.c- return true; src/qemu/qemu_domain.c- } src/qemu/qemu_domain.c- return false; src/qemu/qemu_domain.c-}
Lack of bus/defaults information on query-machines.
-- src/qemu/qemu_domain.c=qemuDomainMachineIsS390CCW(const char *machine) src/qemu/qemu_domain.c-{ src/qemu/qemu_domain.c: return STRPREFIX(machine, "s390-ccw"); src/qemu/qemu_domain.c-}
Need to grep for callers of this function.
-- src/qemu/qemu_domain.c=qemuDomainMachineIsVirt(const char *machine, src/qemu/qemu_domain.c- const virArch arch) src/qemu/qemu_domain.c-{ src/qemu/qemu_domain.c- if (arch != VIR_ARCH_ARMV7L && src/qemu/qemu_domain.c- arch != VIR_ARCH_AARCH64) src/qemu/qemu_domain.c- return false; src/qemu/qemu_domain.c- src/qemu/qemu_domain.c: if (STRNEQ(machine, "virt") && src/qemu/qemu_domain.c: !STRPREFIX(machine, "virt-")) src/qemu/qemu_domain.c- return false; src/qemu/qemu_domain.c- src/qemu/qemu_domain.c- return true; src/qemu/qemu_domain.c-}
Need to grep for callers of this function.
-- src/qemu/qemu_domain.c=qemuDomainMachineIsPSeries(const char *machine, src/qemu/qemu_domain.c- const virArch arch) src/qemu/qemu_domain.c-{ src/qemu/qemu_domain.c- if (!ARCH_IS_PPC64(arch)) src/qemu/qemu_domain.c- return false; src/qemu/qemu_domain.c- src/qemu/qemu_domain.c: if (STRNEQ(machine, "pseries") && src/qemu/qemu_domain.c: !STRPREFIX(machine, "pseries-")) src/qemu/qemu_domain.c- return false; src/qemu/qemu_domain.c- src/qemu/qemu_domain.c- return true; src/qemu/qemu_domain.c-}
Need to grep for callers of this function.
-- src/qemu/qemu_domain.c=qemuDomainMachineHasBuiltinIDE(const char *machine) src/qemu/qemu_domain.c-{ src/qemu/qemu_domain.c- return qemuDomainMachineIsI440FX(machine) || src/qemu/qemu_domain.c: STREQ(machine, "malta") || src/qemu/qemu_domain.c: STREQ(machine, "sun4u") || src/qemu/qemu_domain.c: STREQ(machine, "g3beige"); src/qemu/qemu_domain.c-}
Lack of bus/defaults information on query-machines.
[...] -- src/qemu/qemu_domain_address.c=qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def, src/qemu/qemu_domain_address.c- virQEMUCapsPtr qemuCaps) src/qemu/qemu_domain_address.c-{ src/qemu/qemu_domain_address.c- if (def->os.arch != VIR_ARCH_ARMV7L && src/qemu/qemu_domain_address.c- def->os.arch != VIR_ARCH_AARCH64) src/qemu/qemu_domain_address.c- return; src/qemu/qemu_domain_address.c- src/qemu/qemu_domain_address.c: if (!(STRPREFIX(def->os.machine, "vexpress-") || src/qemu/qemu_domain_address.c- qemuDomainIsVirt(def))) src/qemu/qemu_domain_address.c- return;
Lack of bus/device/defaults information on query-machines?
[...] -- src/qemu/qemu_domain_address.c=qemuDomainSupportsPCI(virDomainDefPtr def, src/qemu/qemu_domain_address.c- virQEMUCapsPtr qemuCaps) src/qemu/qemu_domain_address.c-{ src/qemu/qemu_domain_address.c- if ((def->os.arch != VIR_ARCH_ARMV7L) && (def->os.arch != VIR_ARCH_AARCH64)) src/qemu/qemu_domain_address.c- return true; src/qemu/qemu_domain_address.c- src/qemu/qemu_domain_address.c: if (STREQ(def->os.machine, "versatilepb")) src/qemu/qemu_domain_address.c- return true; src/qemu/qemu_domain_address.c- src/qemu/qemu_domain_address.c- if (qemuDomainIsVirt(def) && src/qemu/qemu_domain_address.c- virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_GPEX)) src/qemu/qemu_domain_address.c- return true; src/qemu/qemu_domain_address.c-
Lack of bus information on query-machines.
[...]
Daniel Henrique Barboza <danielhb@linux.ibm.com> writes:
Hi,
Sorry for the delay. I'll summarize what I've understood from the discussion so far:
- query-target is the wrong place for this flag. query-machines is (less) wrong because it is not a static property of the machine object
- a new "query-current-machine" can be created to host these dynamic properties that belongs to the current instance of the VM
- there are machines in which the suspend support may vary with a "-no-acpi" option that would disable both the suspend and wake-up support. In this case, I see no problem into counting this flag into the logic (assuming it is possible, of course) and setting it as "false" if there is -no-acpi present (or even making the API returning "yes", "no" or "acpi" like Markus suggested) somewhere.
Based on the last email from Eduardo, apparently there is a handful of other machine properties that can be hosted in either this new query-current-machine API or query-machines. I believe that this is more of a long term goal, but this new query-current-machine API would be a good kick-off and we should go for it.
Is this a fair understanding? Did I miss something?
Sounds fair to me. Adding query-current-machine on the evidence of just one flag would be questionable, but Eduardo expects there to be more, so it's okay. Whether a property is static or dynamic can change over time, which makes the choice of query-machines vs. query-current-machine non-trivial. We better write down how we plan to handle mispredictions, i.e. what to do when a property we put into query-machines turns out to be dynamic, or a property we put into query-current-machine turns out to be static, or we'd like query-machines to cover a property that is static except for a few machines. Eduardo, anything to add?
participants (3)
-
Daniel Henrique Barboza -
Eduardo Habkost -
Markus Armbruster