[libvirt] [PATCH 35/41] cpu: Rework cpuUpdate
John Ferlan
jferlan at redhat.com
Tue Aug 30 21:53:56 UTC 2016
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 at 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,
[...]
More information about the libvir-list
mailing list