On 03/05/2018 06:44 AM, Viktor Mihajlovski wrote:

Detect whether QEMU supports the QMP query-cpus-fast API and set QEMU_CAPS_QUERY_CPUS_FAST in this case.

Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Acked-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 4 +++- src/qemu/qemu_capabilities.h | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)

Now that the 2.12 caps update has been pushed... It's "open season" on the qemu_capabilities.{c,h}. Since this series has been on list the longest, I'll start here... This particular patch will need an update to add the new flag to the various caps_2.12*.xml files (VIR_TEST_REGENERATE_OUTPUT=1 tests/qemucapabilitiestest does the trick).

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index b5eb8cf..6635f5e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -459,6 +459,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "pl011", "machine.pseries.max-cpu-compat", "dump-completed", + "query-cpus-fast", );

@@ -1579,7 +1580,8 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = { { "query-qmp-schema", QEMU_CAPS_QUERY_QMP_SCHEMA }, { "query-cpu-model-expansion", QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION}, { "query-cpu-definitions", QEMU_CAPS_QUERY_CPU_DEFINITIONS}, - { "query-named-block-nodes", QEMU_CAPS_QUERY_NAMED_BLOCK_NODES} + { "query-named-block-nodes", QEMU_CAPS_QUERY_NAMED_BLOCK_NODES}, + { "query-cpus-fast", QEMU_CAPS_QUERY_CPUS_FAST}

I know this has been ACK'd; however, I recently pushed a small fixup in this area that you'll have a merge with, see commit id '1706bef6' (I saw that while working on something else, but noted this patch is affected). Still for this new entry, it'd be better to use the format : { "query-cpus-fast", QEMU_CAPS_QUERY_CPUS_FAST }, John

};

struct virQEMUCapsStringFlags virQEMUCapsMigration[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index c2ec2be..e3c31ab 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -444,6 +444,7 @@ typedef enum { QEMU_CAPS_DEVICE_PL011, /* -device pl011 (not user-instantiable) */ QEMU_CAPS_MACHINE_PSERIES_MAX_CPU_COMPAT, /* -machine pseries,max-cpu-compat= */ QEMU_CAPS_DUMP_COMPLETED, /* DUMP_COMPLETED event */ + QEMU_CAPS_QUERY_CPUS_FAST, /* query-cpus-fast command */

QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags;

...

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8b4efc8..4079fb3 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8683,7 +8683,12 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver, if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) return -1;

- rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), &info, maxvcpus, hotplug); + rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), + &info, + maxvcpus, + hotplug, + virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(vm)->qemuCaps, + QEMU_CAPS_QUERY_CPUS_FAST));

Perhaps we should create a qemuMonitorGetCPUInfoFast API since we should also be able to return the @props {core-id, thread-id, socket-id} and report them. I'm not steadfast on this suggestion - maybe Peter has a stronger opinion one way or the other. Still it allows future adjustments and additions to -fast to be more easily "handled".

It would seem those values could be useful although I do see there could be some "impact" with how hotplug works and the default setting to -1 of the values by qemuMonitorCPUInfoClear. Actually, query-cpus[-fast] is not reporting the topology (socket, core,

On 23.03.2018 17:05, John Ferlan wrote: [...] thread). The reported thread id is referring to the QEMU CPU thread.

FWIW: Below this point as the returned @info structure is parsed, there's a comment about query-cpus that would then of course not necessarily be true or at the very least "adjusted properly" for this new -fast model.

...

if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; @@ -8803,7 +8808,10 @@ qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver, if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) return -1;

- haltedmap = qemuMonitorGetCpuHalted(qemuDomainGetMonitor(vm), maxvcpus); + haltedmap = qemuMonitorGetCpuHalted(qemuDomainGetMonitor(vm), + maxvcpus, + virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(vm)->qemuCaps, + QEMU_CAPS_QUERY_CPUS_FAST));

Maybe this one should pass 'false' for now until patch 4 comes along to add the code that fetches the cpu-state and alters halted for the -fast model. Depending on how one feels about larger patches vs having a 2 patch gap to not have the support for -fast... I dunno, I could see reason to merge patch 4 here too in order to be in a patch feature for feature compatible.> FWIW, I intended to make the patches deprecation-safe, i.e. prevent calling query-cpus on a QEMU not supporting it anymore. I see your point

Agreed, this one escaped me. Will change. though and would have no principal objections to merging patches 2 and 4 (plus the testcase patches 3 and 5). Would be great to hear more opinions...

...

if (qemuDomainObjExitMonitor(driver, vm) < 0 || !haltedmap) goto cleanup;

[...]

...

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index a09e93e..6a5fb12 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1466,7 +1466,8 @@ int qemuMonitorJSONSystemReset(qemuMonitorPtr mon) static int qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data, struct qemuMonitorQueryCpusEntry **entries, - size_t *nentries) + size_t *nentries, + bool fast) { struct qemuMonitorQueryCpusEntry *cpus = NULL; int ret = -1; @@ -1491,11 +1492,19 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data, }

/* Some older qemu versions don't report the thread_id so treat this as - * non-fatal, simply returning no data */ - ignore_value(virJSONValueObjectGetNumberInt(entry, "CPU", &cpuid)); - ignore_value(virJSONValueObjectGetNumberInt(entry, "thread_id", &thread)); - ignore_value(virJSONValueObjectGetBoolean(entry, "halted", &halted)); - qom_path = virJSONValueObjectGetString(entry, "qom_path"); + * non-fatal, simply returning no data. + * The return data of query-cpus-fast has different field names + */ + if (fast) { + ignore_value(virJSONValueObjectGetNumberInt(entry, "cpu-index", &cpuid)); + ignore_value(virJSONValueObjectGetNumberInt(entry, "thread-id", &thread)); + qom_path = virJSONValueObjectGetString(entry, "qom-path"); + } else { + ignore_value(virJSONValueObjectGetNumberInt(entry, "CPU", &cpuid)); + ignore_value(virJSONValueObjectGetNumberInt(entry, "thread_id", &thread)); + ignore_value(virJSONValueObjectGetBoolean(entry, "halted", &halted)); + qom_path = virJSONValueObjectGetString(entry, "qom_path"); + }

So query-fast doesn't report "halted" which means we default to false for every arch other than s390 which gets fixed in 2 patches. IIRC other arches weren't ever reporting halted = true for the not fast and only our test environment had the halted set. Although I could be wrong - Peter was much more involved in that code, so I assume he'd have a more definitive answer. Halted was reported at least for x86, and a value of true was rather common ony multi-core systems. However, defaulting to false doesn't hurt (beyond the temporary impact on s390), because the user visible halted value is a tristate for a while now.

Anyway, from the 0/6 cover, I see:

"query-cpus-fast doesn't return the halted state for a virtual CPU, meaning that the vcpu.<n>.halted value won't be reported with 'virsh domstats' anymore. This is OK, as stats values are not guaranteed to be reported under all circumstances and on all architectures."

That's not really the case here since halted defaults to false, but figured I'd ask just to be sure. If not reporting it was the desire, then "bool halted" would need to to turn into a virTristateBool in qemuMonitorQueryCpusEntry and _qemuMonitorCPUInfo. That way it would be either properly reported as halted or not and if not provided, it would not be printed. But that may cause issues for other code that uses halted... Of course I'm not sure

...

cpus[i].qemu_id = cpuid; cpus[i].tid = thread;

John

-- Regards, Viktor Mihajlovski

On 03/26/2018 07:28 AM, Viktor Mihajlovski wrote:

...

...

...

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8b4efc8..4079fb3 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8683,7 +8683,12 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver, if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) return -1;

- rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), &info, maxvcpus, hotplug); + rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), + &info, + maxvcpus, + hotplug, + virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(vm)->qemuCaps, + QEMU_CAPS_QUERY_CPUS_FAST));

Perhaps we should create a qemuMonitorGetCPUInfoFast API since we should also be able to return the @props {core-id, thread-id, socket-id} and report them. I'm not steadfast on this suggestion - maybe Peter has a stronger opinion one way or the other. Still it allows future adjustments and additions to -fast to be more easily "handled".

It would seem those values could be useful although I do see there could be some "impact" with how hotplug works and the default setting to -1 of the values by qemuMonitorCPUInfoClear. Actually, query-cpus[-fast] is not reporting the topology (socket, core,

On 23.03.2018 17:05, John Ferlan wrote: [...] thread). The reported thread id is referring to the QEMU CPU thread. Well, I have to take that back as I ignored that QEMU is providing the

On 26.03.2018 10:12, Viktor Mihajlovski wrote: props since 2.10 with query-cpus (and now with query-cpus-fast). If we decide to parse the props, we could do it with the pre-existing qemuMonitorGetCPUInfo function, but I would prefer this to be done in separate patch. [...]

I didn't even look at the query-cpus and @props - I was just considering the -fast code. And yes, looking at @props for query-cpus would have to be a separate patch with it's own capability - if of course it was even deemed worthwhile to do... John

On 03/05/2018 06:44 AM, Viktor Mihajlovski wrote:

Extended the json monitor test program with support for query-cpus-fast and added a sample file set for x86 data obtained using the it.

Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- ...qemumonitorjson-cpuinfo-x86-full-fast-cpus.json | 71 +++++++++++++ ...umonitorjson-cpuinfo-x86-full-fast-hotplug.json | 115 ++++++++++++++++++++ .../qemumonitorjson-cpuinfo-x86-full-fast.data | 109 +++++++++++++++++++ tests/qemumonitorjsontest.c | 118 ++++++++++++++++----- tests/qemumonitortestutils.c | 6 ++ tests/qemumonitortestutils.h | 1 + 6 files changed, 394 insertions(+), 26 deletions(-) create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-cpus.json create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-hotplug.json create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast.data

diff --git a/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-cpus.json b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-cpus.json new file mode 100644 index 0000000..88fd2b8 --- /dev/null +++ b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-cpus.json @@ -0,0 +1,71 @@ +{ + "return": [ + { + "arch": "x86", + "cpu-index": 0, + "qom-path": "/machine/unattached/device[0]", + "thread-id": 895040 + },

Each of these seems to be missing the "props" structure even though the hotplug does have it. As noted in my 2/6 review - that could be useful data to parse (and return and save for printing in the stats output). In fact I recall those values being the most painful when dealing with this the last time I was reviewing in here...

+ { + "arch": "x86", + "cpu-index": 1, + "qom-path": "/machine/peripheral/vcpu1", + "thread-id": 895056 + },

[...]

diff --git a/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-hotplug.json b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-hotplug.json new file mode 100644 index 0000000..aff5aa3 --- /dev/null +++ b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-hotplug.json @@ -0,0 +1,115 @@ +{ + "return": [ + { + "props": { + "core-id": 0, + "thread-id": 0, + "socket-id": 10 + }, + "vcpus-count": 1, + "qom-path": "/machine/peripheral/vcpu10", + "type": "Broadwell-x86_64-cpu" + },

See you do show the props here...

+ { + "props": { + "core-id": 0, + "thread-id": 0, + "socket-id": 9 + }, + "vcpus-count": 1, + "qom-path": "/machine/peripheral/vcpu9", + "type": "Broadwell-x86_64-cpu" + },

[...]

diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 5e30fb0..b20d59a 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -1439,3 +1439,9 @@ qemuMonitorTestGetAgent(qemuMonitorTestPtr test) { return test->agent; }

Format here is 2 blank lines between methods John

+ +virDomainObjPtr +qemuMonitorTestGetDomainObj(qemuMonitorTestPtr test) +{ + return test->vm; +} diff --git a/tests/qemumonitortestutils.h b/tests/qemumonitortestutils.h index 8b19b37..3604a1b 100644 --- a/tests/qemumonitortestutils.h +++ b/tests/qemumonitortestutils.h @@ -96,5 +96,6 @@ void qemuMonitorTestFree(qemuMonitorTestPtr test);

qemuMonitorPtr qemuMonitorTestGetMonitor(qemuMonitorTestPtr test); qemuAgentPtr qemuMonitorTestGetAgent(qemuMonitorTestPtr test); +virDomainObjPtr qemuMonitorTestGetDomainObj(qemuMonitorTestPtr test);

#endif /* __VIR_QEMU_MONITOR_TEST_UTILS_H__ */

On 23.03.2018 17:08, John Ferlan wrote: [...]

...

+static void +qemuMonitorJSONExtractCPUS390Info(virJSONValuePtr jsoncpu, + struct qemuMonitorQueryCpusEntry *cpu) +{ + const char *cpu_state = virJSONValueObjectGetString(jsoncpu, "cpu-state"); + + if (STREQ_NULLABLE(cpu_state, "operating") || + STREQ_NULLABLE(cpu_state, "load")) + cpu->halted = false; + else if (STREQ_NULLABLE(cpu_state, "stopped") || + STREQ_NULLABLE(cpu_state, "check-stop")) + cpu->halted = true;

Realistically, since false is the default, then only "stopped" and "check-stop" need to be checked... Even 'uninitialized' would show up as false since it's not checked.

As you say, it's not necessary to handle the false case explicitly. Still, I would like to be explicit here.

Perhaps you should create and carry up the calling stack a copy of the cpu-state that way eventually it could be printed in a 'stats' output as well...

I suppose another concern is that some future halted state is created and this code does account for that leading to incorrect reporting and well some other issues since @halted is used for various decisions.

At this point in time I'm mainly concerned about providing the same client behaviour with both query-cpus and query-cpus-fast. In the long run it might make sense to provide architecture specific CPU information, but this will require more thought and discussion.

...

+} + +/** + * qemuMonitorJSONExtractCPUArchInfo: + * @arch: virtual CPU's architecture + * @jsoncpu: pointer to a single JSON cpu entry + * @cpu: pointer to a single cpu entry * + * Extracts architecure specific virtual CPU data for a single + * CPU from the QAPI response using an architecture specific + * function. + * + */ +static void +qemuMonitorJSONExtractCPUArchInfo(const char *arch, + virJSONValuePtr jsoncpu, + struct qemuMonitorQueryCpusEntry *cpu) +{ + if (STREQ_NULLABLE(arch, "s390")) + qemuMonitorJSONExtractCPUS390Info(jsoncpu, cpu); +} + +/** + * qemuMonitorJSONExtractCPUArchInfo:

^^ This is still qemuMonitorJSONExtractCPUInfo

oops

...

+ * @data: JSON response data + * @entries: filled with detected cpu entries on success + * @nentries: number of entries returned + * @fast: true if this is a response from query-cpus-fast + * + * The JSON response @data will have the following format + * in case @fast == false * [{ "arch": "x86", * "current": true, * "CPU": 0, * "qom_path": "/machine/unattached/device[0]", * "pc": -2130415978, * "halted": true, - * "thread_id": 2631237}, + * "thread_id": 2631237, + * ...}, + * {...} + * ] + * and for @fast == true + * [{ "arch": "x86", + * "cpu-index": 0, + * "qom-path": "/machine/unattached/device[0]", + * "thread_id": 2631237, + * ...},

May as well show the whole example and even provide a s390 example so that it's more obvious... can do

...

* {...} * ] */ @@ -1486,6 +1556,7 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data, int thread = 0; bool halted = false; const char *qom_path; + const char *arch; if (!entry) { ret = -2; goto cleanup; @@ -1511,6 +1582,10 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data, cpus[i].halted = halted; if (VIR_STRDUP(cpus[i].qom_path, qom_path) < 0) goto cleanup; + + /* process optional architecture-specific data */ + arch = virJSONValueObjectGetString(entry, "arch"); + qemuMonitorJSONExtractCPUArchInfo(arch, entry, cpus + i);

Since you're passing @entry anyway, you could fetch @arch in qemuMonitorJSONExtractCPUArchInfo and since it's only valid for "fast == true", calling should be gated on that; otherwise, this would be called for both fast options. I can push the architecture extraction down the stack, but I wouldn't make the call optional. While not used, there's still architecture-specific information returned in query-cpus.

BTW: Rather than "cpus[i]" and "cpus + i", could we just create a local/stack "cpu" and use it?

I'll have a look.

John

...

}

VIR_STEAL_PTR(*entries, cpus);

-- Regards, Viktor Mihajlovski

On 03/05/2018 06:44 AM, Viktor Mihajlovski wrote:

The s390 testcase verifies that the s390-specific cpu-state field is correctly mapped to the halted property.

Since a few of the x86 and ppc testcases return "halted": "true" it was necessary to update the respective .data files.

Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- .../qemumonitorjson-cpuinfo-ppc64-hotplug-4.data | 8 ++++++++ .../qemumonitorjson-cpuinfo-s390-fast-cpus.json | 21 +++++++++++++++++++++ .../qemumonitorjson-cpuinfo-s390-fast-hotplug.json | 21 +++++++++++++++++++++ .../qemumonitorjson-cpuinfo-s390-fast.data | 19 +++++++++++++++++++ ...qemumonitorjson-cpuinfo-x86-basic-pluggable.data | 5 +++++ .../qemumonitorjson-cpuinfo-x86-node-full.data | 2 ++ tests/qemumonitorjsontest.c | 5 +++++ 7 files changed, 81 insertions(+) create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast-cpus.json create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast-hotplug.json create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast.data

I think you could "split" this out print the halted earlier (perhaps before any of these patches) just to prove/disprove what (if anything) changes when the -fast command is added. Then the s390 specific change gets included with patch 4 (or merged with patch 2 depending on how things go). John

diff --git a/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-ppc64-hotplug-4.data b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-ppc64-hotplug-4.data index 7c90889..5f6b865 100644 --- a/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-ppc64-hotplug-4.data +++ b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-ppc64-hotplug-4.data @@ -52,41 +52,49 @@ alias='vcpu0' qom_path='/machine/peripheral/vcpu0' topology: core='8' vcpus='8' + halted [vcpu libvirt-id='9'] online=yes hotpluggable=yes thread-id='23171' query-cpus-id='17' + halted [vcpu libvirt-id='10'] online=yes hotpluggable=yes thread-id='23172' query-cpus-id='18' + halted [vcpu libvirt-id='11'] online=yes hotpluggable=yes thread-id='23173' query-cpus-id='19' + halted [vcpu libvirt-id='12'] online=yes hotpluggable=yes thread-id='23174' query-cpus-id='20' + halted [vcpu libvirt-id='13'] online=yes hotpluggable=yes thread-id='23175' query-cpus-id='21' + halted [vcpu libvirt-id='14'] online=yes hotpluggable=yes thread-id='23176' query-cpus-id='22' + halted [vcpu libvirt-id='15'] online=yes hotpluggable=yes thread-id='23177' query-cpus-id='23' + halted [vcpu libvirt-id='16'] online=yes hotpluggable=yes diff --git a/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast-cpus.json b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast-cpus.json new file mode 100644 index 0000000..4082e0e --- /dev/null +++ b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast-cpus.json @@ -0,0 +1,21 @@ +{ + "return": [ + { + "arch": "s390", + "cpu-index": 0, + "props": {"core-id": 0}, + "qom-path": "/machine/unattached/device[0]", + "thread-id": 89504, + "cpu-state": "operating" + }, + { + "arch": "s390", + "cpu-index": 1, + "props": {"core-id": 1}, + "qom-path": "/machine/unattached/device[1]", + "thread-id": 89505, + "cpu-state": "stopped" + } + ], + "id": "libvirt-42" +} diff --git a/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast-hotplug.json b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast-hotplug.json new file mode 100644 index 0000000..8016b5b --- /dev/null +++ b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast-hotplug.json @@ -0,0 +1,21 @@ +{ + "return": [ + { + "props": { + "core-id": 1 + }, + "vcpus-count": 1, + "qom-path": "/machine/unattached/device[1]", + "type": "host-s390x-cpu" + }, + { + "props": { + "core-id": 0 + }, + "vcpus-count": 1, + "qom-path": "/machine/unattached/device[0]", + "type": "host-s390x-cpu" + } + ], + "id": "libvirt-41" +} diff --git a/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast.data b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast.data new file mode 100644 index 0000000..9fc7041 --- /dev/null +++ b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast.data @@ -0,0 +1,19 @@ +[vcpu libvirt-id='0'] + online=yes + hotpluggable=no + thread-id='89504' + enable-id='1' + query-cpus-id='0' + type='host-s390x-cpu' + qom_path='/machine/unattached/device[0]' + topology: core='0' vcpus='1' +[vcpu libvirt-id='1'] + online=yes + hotpluggable=no + thread-id='89505' + enable-id='2' + query-cpus-id='1' + type='host-s390x-cpu' + qom_path='/machine/unattached/device[1]' + topology: core='1' vcpus='1' + halted diff --git a/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-basic-pluggable.data b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-basic-pluggable.data index 93cefb9..9a1788d 100644 --- a/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-basic-pluggable.data +++ b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-basic-pluggable.data @@ -7,6 +7,7 @@ type='qemu64-x86_64-cpu' qom_path='/machine/unattached/device[0]' topology: socket='0' core='0' thread='0' vcpus='1' + halted [vcpu libvirt-id='1'] online=yes hotpluggable=no @@ -16,6 +17,7 @@ type='qemu64-x86_64-cpu' qom_path='/machine/unattached/device[2]' topology: socket='0' core='0' thread='1' vcpus='1' + halted [vcpu libvirt-id='2'] online=yes hotpluggable=no @@ -25,6 +27,7 @@ type='qemu64-x86_64-cpu' qom_path='/machine/unattached/device[3]' topology: socket='0' core='1' thread='0' vcpus='1' + halted [vcpu libvirt-id='3'] online=yes hotpluggable=no @@ -34,6 +37,7 @@ type='qemu64-x86_64-cpu' qom_path='/machine/unattached/device[4]' topology: socket='0' core='1' thread='1' vcpus='1' + halted [vcpu libvirt-id='4'] online=yes hotpluggable=no @@ -43,6 +47,7 @@ type='qemu64-x86_64-cpu' qom_path='/machine/unattached/device[5]' topology: socket='1' core='0' thread='0' vcpus='1' + halted [vcpu libvirt-id='5'] online=no hotpluggable=yes diff --git a/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-node-full.data b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-node-full.data index 070ea08..0f7dbf1 100644 --- a/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-node-full.data +++ b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-node-full.data @@ -7,6 +7,7 @@ type='Broadwell-x86_64-cpu' qom_path='/machine/unattached/device[0]' topology: socket='0' core='0' thread='0' node='0' vcpus='1' + halted [vcpu libvirt-id='1'] online=yes hotpluggable=no @@ -16,6 +17,7 @@ type='Broadwell-x86_64-cpu' qom_path='/machine/unattached/device[2]' topology: socket='0' core='0' thread='1' node='1' vcpus='1' + halted [vcpu libvirt-id='2'] online=no hotpluggable=yes diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index a932e39..c642166 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -2712,6 +2712,9 @@ testQemuMonitorCPUInfoFormat(qemuMonitorCPUInfoPtr vcpus, virBufferAddLit(&buf, "\n"); }

+ if (vcpu->halted) + virBufferAddLit(&buf, "halted\n"); + virBufferAdjustIndent(&buf, -4); }

@@ -3029,6 +3032,8 @@ mymain(void) DO_TEST_CPU_INFO("ppc64-hotplug-4", 24); DO_TEST_CPU_INFO("ppc64-no-threads", 16);

+ DO_TEST_CPU_INFO_FAST("s390-fast", 2); + #define DO_TEST_BLOCK_NODE_DETECT(testname) \ do { \ if (virTestRun("node-name-detect(" testname ")", \

On 03/05/2018 06:44 AM, Viktor Mihajlovski wrote:

In order to not affect running VMs, refreshing the halted state is only performed if QEMU supports the query-cpus-fast QAPI.

Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> --- src/qemu/qemu_domain.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)

BTW: You'll need a patch to docs/news.xml as this is something newsworthy I would think. Although calling it "-fast" there may make people wonder about the "slowness" aspect of the current algorithm.

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 4079fb3..a5fcf22 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8801,8 +8801,13 @@ qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver, return 0;

/* The halted state is interresting only on s390(x). On other platforms

"interesting"

- * the data would be stale at the time when it would be used. */ - if (!ARCH_IS_S390(vm->def->os.arch)) + * the data would be stale at the time when it would be used. + * Calling qemuMonitorGetCpuHalted() can adversely affect the running + * VM's performance unless QEMU supports query-cpus-fast. + */ + if (!ARCH_IS_S390(vm->def->os.arch) || + !virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(vm)->qemuCaps, + QEMU_CAPS_QUERY_CPUS_FAST))

So IOW systems that reported this previously that don't get the updated QEMU bits (e.g. > 2.12) would now stop reporting halted only because of the adverse performance impact. But that is a data change and "policy" we'd be inflicting upon them which maybe they don't care about. Perhaps in that case we should have a "flag" that handles whether the caller cares or not. I understand the motivation and reasoning; however, based on how such things have been historically acked or nacked, I'm leaning towards saying this patch shouldn't go in. John

return 0;

if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)