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@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,
[...]