Re: [libvirt] [PATCH v3 0/6] BaselineHypervisorCPU using QEMU QMP exchanges
Hmm, I guess I could have checked the cover letter before looking at the
individual patches. That would safe me some time and thinking :-)
On Thu, Sep 27, 2018 at 16:26:39 -0500, Chris Venteicher wrote:
Some architectures (S390) depend on QEMU to compute baseline CPU
model and
expand a models feature set.
Interacting with QEMU requires starting the QEMU process and completing one or
more query-cpu-model-baseline QMP exchanges with QEMU in addition to a
query-cpu-model-expansion QMP exchange to expand all features in the model.
See "s390x CPU models: exposing features" patch set on Qemu-devel for
discussion
of QEMU aspects.
This is part of resolution of: https://bugzilla.redhat.com/show_bug.cgi?id=1511999
Version 3 attempts to address all issues from V1 and V2 including making the
QEMU process activation for QMP Queries generic (not specific to capabilities)
and moving that code from qemu_capabilities to qemu_process.
So far so good.
I attempted to make the new qemu_process functions consistent with
the functions
but mostly ended up reusing the guts of the previous functions from qemu_capabilities.
I guess you wanted to say "... consistent with the existing functions in
qemu_process.c" or something similar, right? It's fine to just move and
generalize the functions from qemu_capabilities (and perhaps make the
original functions just wrappers around the new ones if needed).
Of interest may be the choice to reuse a domain structure to hold the
Qmp Query
process state and connection information.
This sounds like a bad idea to me.
Also, pay attention to the use of a large random number to uniquely
identify decoupled processes in terms of sockets and file system
footprint. If you believe it's worth the effort I think there are
some pre-existing library functions to establish files with unique
identifiers in a thread safe way.
We already have src/util/virrandom.c for random numbers, but it doesn't
really matter anyway since we don't need or either want to use them
here. Just use mkdtemp() and store the socket, pid, whatever you need in
there.
The last patch may also be interesting and is based on past
discussion of QEMU cpu
expansion only returning migratable features except for one x86 case where
non-migratable features are explicitly requested. The patch records that features
are migratable based on QEMU only returning migratable features. The main result
is the CPU xml for S390 CPU's contains the same migratability info the X86 currently
does. The testcases were updated to reflect the change. Is this ok?
Yeah, looks OK, although I didn't look too closely at it.
Unlike the previous versions every patch should compile independently
if applied
in sequence.
Good, each series have to make sure the code can be compiled after every
single patch in the series (so that we can easily use git bisect). But
please, don't achieve the goal by squashing patches until the result can
be compiled.
Jirka