4:53 p.m.
On 08/12/2016 09:33 AM, Jiri Denemark wrote:
The reworked API is now called virCPUUpdate and it should change the
provided CPU definition into a one which can be consumed by QEMU command
s/by/by the/
line builder:
- host-passthrough remains unchanged
- host-model is turned into custom CPU with a model and features
copied from host
- custom CPU with minimum match is converted similarly to host-model
- optional features are updated according to host's CPU
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
po/POTFILES.in | 1 +
src/cpu/cpu.c | 59 ++++--
src/cpu/cpu.h | 11 +-
src/cpu/cpu_arm.c | 36 +++-
src/cpu/cpu_ppc64.c | 32 ++--
src/cpu/cpu_x86.c | 212 ++++++++-------------
src/libvirt_private.syms | 2 +-
src/qemu/qemu_command.c | 2 +-
src/qemu/qemu_domain.c | 2 +-
src/qemu/qemu_process.c | 2 +-
tests/cputest.c | 14 +-
.../cputestdata/x86-host+host-model-nofallback.xml | 2 +-
tests/cputestdata/x86-host+host-model.xml | 2 +-
.../x86-host+host-passthrough-features.xml | 4 +
tests/cputestdata/x86-host+host-passthrough.xml | 19 +-
tests/cputestdata/x86-host+min.xml | 27 +--
tests/cputestdata/x86-host+pentium3.xml | 39 ++--
tests/cputestdata/x86-host-invtsc+host-model.xml | 2 +-
.../cputestdata/x86-host-passthrough-features.xml | 4 +
19 files changed, 227 insertions(+), 245 deletions(-)
create mode 100644 tests/cputestdata/x86-host+host-passthrough-features.xml
create mode 100644 tests/cputestdata/x86-host-passthrough-features.xml
diff --git a/po/POTFILES.in b/po/POTFILES.in
index 25dbc84..1469240 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -43,6 +43,7 @@ src/conf/virchrdev.c
src/conf/virdomainobjlist.c
src/conf/virsecretobj.c
src/cpu/cpu.c
+src/cpu/cpu_arm.c
src/cpu/cpu_map.c
src/cpu/cpu_ppc64.c
src/cpu/cpu_x86.c
diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c
index fae3885..f3eb211 100644
--- a/src/cpu/cpu.c
+++ b/src/cpu/cpu.c
@@ -579,38 +579,71 @@ cpuBaseline(virCPUDefPtr *cpus,
/**
- * cpuUpdate:
+ * virCPUUpdate:
In a way I'd expect a "virCPUUpdate" API to go in src/util/...
somewhere. Not a problem to keep it here, but I guess when I see vir
prefixed functions I think src/util/...
*
- * @guest: guest CPU definition
+ * @arch: CPU architecture
+ * @guest: guest CPU definition to be updated
* @host: host CPU definition
*
* Updates @guest CPU definition according to @host CPU. This is required to
- * support guest CPU definition which are relative to host CPU, such as CPUs
- * with VIR_CPU_MODE_CUSTOM and optional features or VIR_CPU_MATCH_MINIMUM, or
- * CPUs with non-custom mode (VIR_CPU_MODE_HOST_MODEL,
- * VIR_CPU_MODE_HOST_PASSTHROUGH).
+ * support guest CPU definitions specified relatively to host CPU, such as
+ * CPUs with VIR_CPU_MODE_CUSTOM and optional features or
+ * VIR_CPU_MATCH_MINIMUM, or CPUs with VIR_CPU_MODE_HOST_MODEL.
+ * When the guest CPU was not specified relatively, the function does nothing
+ * and returns success.
*
* Returns 0 on success, -1 on error.
*/
int
-cpuUpdate(virCPUDefPtr guest,
- const virCPUDef *host)
+virCPUUpdate(virArch arch,
+ virCPUDefPtr guest,
+ const virCPUDef *host)
Still not 100% clear whether if eventually (patch 40) it's possible to
enter here with 'host == NULL'... Keeping in mind from patch 26 that
virQEMUCapsInitHostCPUModel could have qemuCaps->cpuModel = NULL, thus
the virQEMUCapsGetHostModel could then return a NULL from
qemuProcessUpdateGuestCPU which calls this function. However, that
possibility is gated by whether virQEMUCapsIsCPUModeSupported succeeds
or not. Since you've added NULLSTR and host ? checks, I'm partially
assuming it's possible to get here with host == NULL. The intro comments
here don't help me determine that though.
I'm lost in the terminology between old/new that's described in patch
40. If your belief is things are going to be OK, then I'm fine with
that, but I figured I'd at least point out what I saw and what got
confusing eventually. Hopefully it's understandable.
{
struct cpuArchDriver *driver;
- VIR_DEBUG("guest=%p, host=%p", guest, host);
+ VIR_DEBUG("arch=%s, guest=%p mode=%s model=%s, host=%p model=%s",
+ virArchToString(arch), guest, virCPUModeTypeToString(guest->mode),
+ NULLSTR(guest->model), host, NULLSTR(host ? host->model : NULL));
The Coverity build throws up here... According to cpu.h, arg 3 is
NONNULL; however, as I pointed out above that could change.
So at the very least cpu.h needs to change to remove the NONNULL(3).
- if ((driver = cpuGetSubDriver(host->arch)) == NULL)
+ if (!(driver = cpuGetSubDriver(arch)))
return -1;
- if (driver->update == NULL) {
+ if (guest->mode == VIR_CPU_MODE_HOST_PASSTHROUGH)
+ return 0;
+
+ if (guest->mode == VIR_CPU_MODE_CUSTOM &&
+ guest->match != VIR_CPU_MATCH_MINIMUM) {
+ size_t i;
+ bool optional = false;
+
+ for (i = 0; i < guest->nfeatures; i++) {
+ if (guest->features[i].policy == VIR_CPU_FEATURE_OPTIONAL) {
+ optional = true;
+ break;
+ }
+ }
+
+ if (!optional)
+ return 0;
+ }
+
+ /* We get here if guest CPU is either
+ * - host-model
+ * - custom with minimum match
+ * - custom with optional features
+ */
+ if (!driver->update) {
virReportError(VIR_ERR_NO_SUPPORT,
- _("cannot update guest CPU data for %s architecture"),
+ _("cannot update guest CPU for %s architecture"),
virArchToString(host->arch));'
Should this just be 'arch' ? if not, if host == NULL, then this will core.
return -1;
}
- return driver->update(guest, host);
+ if (driver->update(guest, host) < 0)
Seems as though all implemented ->update functions will return an error
if 'host' is NULL, so that's good I guess. Whether it's expected is
something you can answer!
+ return -1;
+
+ VIR_DEBUG("model=%s", NULLSTR(guest->model));
+ return 0;
}
diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
index 422818e..f4bb51d 100644
--- a/src/cpu/cpu.h
+++ b/src/cpu/cpu.h
@@ -87,8 +87,8 @@ typedef virCPUDefPtr
unsigned int flags);
typedef int
-(*cpuArchUpdate) (virCPUDefPtr guest,
- const virCPUDef *host);
+(*virCPUArchUpdate)(virCPUDefPtr guest,
+ const virCPUDef *host);
Why the name change? I see upcoming patches also use the new
nomenclature, although I'll assume not all driver functions will change.
Is that the goal for the future?
typedef int
(*cpuArchHasFeature) (const virCPUData *data,
@@ -114,7 +114,7 @@ struct cpuArchDriver {
cpuArchNodeData nodeData;
cpuArchGuestData guestData;
cpuArchBaseline baseline;
- cpuArchUpdate update;
+ virCPUArchUpdate update;
cpuArchHasFeature hasFeature;
cpuArchDataFormat dataFormat;
cpuArchDataParse dataParse;
@@ -182,9 +182,10 @@ cpuBaseline (virCPUDefPtr *cpus,
ATTRIBUTE_NONNULL(1);
int
-cpuUpdate (virCPUDefPtr guest,
+virCPUUpdate(virArch arch,
+ virCPUDefPtr guest,
const virCPUDef *host)
- ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+ ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
Remove the ATTRIBUTE_NONNULL(3) as noted above or even go back to the
earlier void function and allow it to error...
int
cpuHasFeature(const virCPUData *data,
[...]