[libvirt] [PATCH 0/3] Couple of build fixes after latest cpu changes
I've successfully tested these on mingw, arm and x86_64. Almost trivial, but not pushed yet. I'll wait for review. Michal Prívozník (3): virhostcpu.c: Fix misalignment in virHostCPUGetMSRFromKVM comment virhostcpu: Make virHostCPUGetMSR() work only on x86 cpu_x86: Fix placement of *CheckFeature functions src/cpu/cpu_x86.c | 68 ++++++++++++++++++++++--------------------- src/util/virhostcpu.c | 34 ++++++++++++---------- 2 files changed, 54 insertions(+), 48 deletions(-) -- 2.21.0
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virhostcpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index 10bf3a93d5..dabe3eed82 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -1301,7 +1301,7 @@ virHostCPUGetMSRFromKVM(unsigned long index ATTRIBUTE_UNUSED, /* * Returns 0 on success, * 1 when the MSR is not supported by the host CPU, -* -1 on error. + * -1 on error. */ int virHostCPUGetMSR(unsigned long index, -- 2.21.0
Model specific registers are a thing only on x86. Also, the /dev/cpu/0/msr path exists only on Linux and the fallback mechanism (asking KVM) exists on Linux and FreeBSD only. Therefore, move the function within #ifdef that checks all aforementioned constraints and provide a dummy stub for all other cases. This fixes the build on my arm box, mingw-* builds, etc. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virhostcpu.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index dabe3eed82..f4a62c74fa 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -1257,7 +1257,9 @@ virHostCPUGetMicrocodeVersion(void) #endif /* __linux__ */ -#if HAVE_LINUX_KVM_H && defined(KVM_GET_MSRS) +#if HAVE_LINUX_KVM_H && defined(KVM_GET_MSRS) && \ + (defined(__i386__) || defined(__x86_64__)) && \ + (defined(__linux__) || defined(__FreeBSD__)) static int virHostCPUGetMSRFromKVM(unsigned long index, uint64_t *result) @@ -1285,19 +1287,6 @@ virHostCPUGetMSRFromKVM(unsigned long index, return 0; } -#else - -static int -virHostCPUGetMSRFromKVM(unsigned long index ATTRIBUTE_UNUSED, - uint64_t *result ATTRIBUTE_UNUSED) -{ - virReportSystemError(ENOSYS, "%s", - _("Reading MSRs via KVM is not supported on this platform")); - return -1; -} -#endif /* HAVE_LINUX_KVM_H && defined(KVM_GET_MSRS) */ - - /* * Returns 0 on success, * 1 when the MSR is not supported by the host CPU, @@ -1334,3 +1323,18 @@ virHostCPUGetMSR(unsigned long index, return virHostCPUGetMSRFromKVM(index, msr); } + +#else + +int +virHostCPUGetMSR(unsigned long index ATTRIBUTE_UNUSED, + uint64_t *msr ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Reading MSRs is not supported on this platform")); + return -1; +} + +#endif /* HAVE_LINUX_KVM_H && defined(KVM_GET_MSRS) && \ + (defined(__i386__) || defined(__x86_64__)) && \ + (defined(__linux__) || defined(__FreeBSD__)) */ -- 2.21.0
In e17d10386 these functions were mistakenly moved into an #ifdef block, but remained used outside of it leaving the build broken for platforms where #ifdef evaluated to false. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/cpu/cpu_x86.c | 68 ++++++++++++++++++++++++----------------------- 1 file changed, 35 insertions(+), 33 deletions(-) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 1fe6770d57..f05bfa24e0 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -2707,39 +2707,6 @@ cpuidSet(uint32_t base, virCPUDataPtr data) } -static int -virCPUx86CheckFeature(const virCPUDef *cpu, - const char *name) -{ - int ret = -1; - virCPUx86MapPtr map; - virCPUx86ModelPtr model = NULL; - - if (!(map = virCPUx86GetMap())) - return -1; - - if (!(model = x86ModelFromCPU(cpu, map, -1))) - goto cleanup; - - ret = x86FeatureInData(name, &model->data, map); - - cleanup: - x86ModelFree(model); - return ret; -} - - -static int -virCPUx86DataCheckFeature(const virCPUData *data, - const char *name) -{ - virCPUx86MapPtr map; - - if (!(map = virCPUx86GetMap())) - return -1; - - return x86FeatureInData(name, &data->data.x86, map); -} static int @@ -2769,6 +2736,41 @@ virCPUx86GetHost(virCPUDefPtr cpu, #endif +static int +virCPUx86CheckFeature(const virCPUDef *cpu, + const char *name) +{ + int ret = -1; + virCPUx86MapPtr map; + virCPUx86ModelPtr model = NULL; + + if (!(map = virCPUx86GetMap())) + return -1; + + if (!(model = x86ModelFromCPU(cpu, map, -1))) + goto cleanup; + + ret = x86FeatureInData(name, &model->data, map); + + cleanup: + x86ModelFree(model); + return ret; +} + + +static int +virCPUx86DataCheckFeature(const virCPUData *data, + const char *name) +{ + virCPUx86MapPtr map; + + if (!(map = virCPUx86GetMap())) + return -1; + + return x86FeatureInData(name, &data->data.x86, map); +} + + static virCPUDefPtr virCPUx86Baseline(virCPUDefPtr *cpus, unsigned int ncpus, -- 2.21.0
Michal Privoznik <mprivozn@redhat.com> [2019-04-15, 06:50AM +0200]:
I've successfully tested these on mingw, arm and x86_64.
Fixes S390 as well, thanks.
Almost trivial, but not pushed yet. I'll wait for review.
I got another incidential build error that doesn't seem related though: ../../examples/domtop/domtop.c: In function 'print_cpu_usage': ../../examples/domtop/domtop.c:242:28: error: 'pos' may be used uninitialized in this function [-Werror=maybe-uninitialized] usage = (now_params[pos].value.ul - then_params[pos].value.ul) / ^ cc1: all warnings being treated as errors GCC is 8.2.1. Here's a patch: diff --git a/examples/domtop/domtop.c b/examples/domtop/domtop.c index c9b51aed..e463e287 100644 --- a/examples/domtop/domtop.c +++ b/examples/domtop/domtop.c @@ -206,7 +206,7 @@ print_cpu_usage(size_t cpu, } for (i = 0; i < ncpus; i++) { - size_t pos; + size_t pos = 0; double usage; /* check if the vCPU is in the maps */
On 4/15/19 8:12 AM, Bjoern Walk wrote:
Michal Privoznik <mprivozn@redhat.com> [2019-04-15, 06:50AM +0200]:
I've successfully tested these on mingw, arm and x86_64.
Fixes S390 as well, thanks.
Almost trivial, but not pushed yet. I'll wait for review.
I got another incidential build error that doesn't seem related though:
Yep, it's unrelated.
../../examples/domtop/domtop.c: In function 'print_cpu_usage': ../../examples/domtop/domtop.c:242:28: error: 'pos' may be used uninitialized in this function [-Werror=maybe-uninitialized] usage = (now_params[pos].value.ul - then_params[pos].value.ul) / ^ cc1: all warnings being treated as errors
GCC is 8.2.1. Here's a patch:
diff --git a/examples/domtop/domtop.c b/examples/domtop/domtop.c index c9b51aed..e463e287 100644 --- a/examples/domtop/domtop.c +++ b/examples/domtop/domtop.c @@ -206,7 +206,7 @@ print_cpu_usage(size_t cpu, }
for (i = 0; i < ncpus; i++) { - size_t pos; + size_t pos = 0; double usage;
/* check if the vCPU is in the maps */
This is a false positive. Either the control cannot get to line 242 or if it will then @pos was set. But if this patch helps you to compile cleanly then I'm up for taking it. Mind sending it as a proper patch? Thanks, Michal
On Mon, Apr 15, 2019 at 06:50:16 +0200, Michal Privoznik wrote:
I've successfully tested these on mingw, arm and x86_64.
Almost trivial, but not pushed yet. I'll wait for review.
Michal Prívozník (3): virhostcpu.c: Fix misalignment in virHostCPUGetMSRFromKVM comment virhostcpu: Make virHostCPUGetMSR() work only on x86 cpu_x86: Fix placement of *CheckFeature functions
src/cpu/cpu_x86.c | 68 ++++++++++++++++++++++--------------------- src/util/virhostcpu.c | 34 ++++++++++++---------- 2 files changed, 54 insertions(+), 48 deletions(-)
Oops, thanks for fixing the issues. Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
participants (3)
-
Bjoern Walk -
Jiri Denemark -
Michal Privoznik