On 07/11/2016 09:46 AM, Cedric Bosdonnat wrote:
> On Fri, 2016-07-08 at 17:16 +0100, Joao Martins wrote:
>> Parse libxl_hwcaps accounting for versions for Xen 4.4.
>> libxl_hwcaps is a set of cpuid leaves output that is described in [0] or
>> [1] in Xen 4.7. This is a collection of CPUID leaves that are we version
>> in libvirt whenever leaves are reordered or added. Thus we keep the
>> common ones in one struct and others for each version. Since
>> libxl_hwcaps doesn't appear to have a stable format across all supported
>> versions thus we need to keep track of changes as a compromise whenever
>> these extracting host featuresets are exported by libxl. Also we don't
>> fail in initializing the driver in case parsing of hwcaps failed.
>>
>> xen/include/asm-x86/cpufeature.h
>> include/public/arch-x86/cpufeatureset.h
>>
>> Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
>> ---
>> Probably to make it easier to extend hwcaps handling in the future may
>> be it's better to introduce an enum for the version and restructure this
>> in another way?
>> ---
>> src/libxl/libxl_capabilities.c | 106 +++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 103 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c
>> index f71fd3b..c0857e5 100644
>> --- a/src/libxl/libxl_capabilities.c
>> +++ b/src/libxl/libxl_capabilities.c
>> @@ -36,6 +36,8 @@
>> #include "domain_capabilities.h"
>> #include "vircommand.h"
>> #include "libxl_capabilities.h"
>> +#include "cpu/cpu_x86.h"
>> +#include "cpu/cpu_x86_data.h"
>>
>>
>> #define VIR_FROM_THIS VIR_FROM_LIBXL
>> @@ -43,7 +45,8 @@
>> VIR_LOG_INIT("libxl.libxl_capabilities");
>>
>> /* see xen-unstable.hg/xen/include/asm-x86/cpufeature.h */
>> -#define LIBXL_X86_FEATURE_PAE_MASK 0x40
>> +#define LIBXL_X86_FEATURE_PAE_MASK (1 << 6)
>> +#define LIBXL_X86_FEATURE_LM_MASK (1 << 29)
>
> +1 for the notation change: makes it much easier to associate to the
> original bits definitions.
Will add a comment in the commit message regarding this change.
>
>>
>> struct guest_arch {
>> @@ -57,17 +60,94 @@ struct guest_arch {
>>
>> #define XEN_CAP_REGEX "(xen|hvm)-[[:digit:]]+\\.[[:digit:]]+-(aarch64|armv7l|x86_32|x86_64|ia64|powerpc64)(p|be)?"
>>
>> +static int
>> +libxlCapsAddCPUID(virCPUx86Data *data, virCPUx86CPUID *cpuid, ssize_t ncaps)
>> +{
>> + size_t i;
>> +
>> + for (i = 0; i < ncaps; i++) {
>> + virCPUx86CPUID *c = &cpuid[i];
>> +
>> + if (virCPUx86DataAddCPUID(data, c) < 0) {
>> + VIR_DEBUG("Failed to add CPUID(%x,%x)", c->eax_in, c->ecx_in);
>> + return -1;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * The words represented in physinfo.hw_cap are host CPUID (sub) leafs.
>> + * Position of these hasn't changed much up until Xen 4.7 with a rework
>> + * on how CPUID is handled internally. As a side-effect it got normalized
>> + * and also added more feature words. Although cannot be relied upon as
>> + * stable interface, and hence we version changes in position of the features
>> + * across all supported versions of the libxl driver until libxl exposes a
>> + * stable representation of these capabilities. Fortunately not a lot of
>> + * variation happened so it's still possible to keep track of these leafs
>> + * to describe host CPU in libvirt capabilities. v0 is for Xen 4.4 up to 4.6,
>> + * while v1 is meant for Xen 4.7, as depicted in the table below:
>> + *
>> + * | v0 (Xen 4.4 - 4.6) | v1 (Xen >= 4.7) |
>> + * ---------------------------------------------
>> + * word 0 | CPUID.00000001.EDX | CPUID.00000001.EDX |
>> + * word 1 | CPUID.80000001.EDX | CPUID.00000001.ECX |
>> + * word 2 | CPUID.80860001 | CPUID.80000001.EDX |
>> + * word 3 | - Linux - | CPUID.80000001.ECX |
>> + * word 4 | CPUID.00000001.ECX | CPUID.0000000D:1.EAX |
>> + * word 5 | CPUID.C0000001 | CPUID.00000007:0.EBX |
>> + * word 6 | CPUID.80000001.ECX | CPUID.00000007:0.ECX |
>> + * word 7 | CPUID.00000007.EBX | CPUID.80000007.EDX |
>> + * word 8 | - Non existent - | CPUID.80000008.EBX |
>> + *
>> + */
>> +static virCPUDataPtr
>> +libxlCapsNodeData(virCPUDefPtr cpu, libxl_hwcap hwcap, unsigned version)
>> +{
>> + ssize_t ncaps;
>> + virCPUx86Data data = VIR_CPU_X86_DATA_INIT;
>> + virCPUx86CPUID cpuid[] = {
>> + { .eax_in = 0x00000001, .edx = hwcap[0] },
>> + { .eax_in = 0x00000001, .ecx = (version > 0 ? hwcap[1] : hwcap[4]) },
>
> Shouldn't constants be used rather than magic numbers for the versions?
> How about something like LIBXL_HWCAP_V0?
Yeap - I agree.
>
> Other than that, the patch looks good to me.
Cool - Thanks for looking at it!
Joao
>
> --
> Cedric
>
>> + { .eax_in = 0x80000001, .edx = (version > 0 ? hwcap[2] : hwcap[1]) },
>> + { .eax_in = 0x80000001, .ecx = (version > 0 ? hwcap[3] : hwcap[6]) },
>> + { .eax_in = 0x00000007, .ebx = (version > 0 ? hwcap[5] : hwcap[7]) }
>> + };
>> + virCPUx86CPUID cpuid_ver1[] = {
>> + { .eax_in = 0x0000000D, .ecx_in = 1U, .eax = hwcap[4] },
>> + { .eax_in = 0x00000007, .ecx_in = 0U, .ecx = hwcap[6] },
>> + { .eax_in = 0x80000007, .ecx_in = 0U, .edx = hwcap[7] },
>> + };
>> +
>> + ncaps = ARRAY_CARDINALITY(cpuid);
>> + if (libxlCapsAddCPUID(&data, cpuid, ncaps) < 0)
>> + return NULL;
>> +
>> + ncaps = ARRAY_CARDINALITY(cpuid_ver1);
>> + if (version > 0 && libxlCapsAddCPUID(&data, cpuid_ver1, ncaps) < 0)
>> + return NULL;
>> +
>> + return virCPUx86MakeData(cpu->arch, &data);
>> +}
>> +
>> /* hw_caps is an array of 32-bit words whose meaning is listed in
>> * xen-unstable.hg/xen/include/asm-x86/cpufeature.h. Each feature
>> * is defined in the form X*32+Y, corresponding to the Y'th bit in
>> * the X'th 32-bit word of hw_cap.
>> */
>> static int
>> -libxlCapsInitCPU(virCapsPtr caps, libxl_physinfo *phy_info)
>> +libxlCapsInitCPU(virCapsPtr caps, libxl_physinfo *phy_info, unsigned version)
>> {
>> virCPUDefPtr cpu = NULL;
>> + virCPUDataPtr data = NULL;
>> int ret = -1;
>> int host_pae;
>> + int host_lm;
>> +
>> + /* parse hw_cap only for x86 */
>> + if (!phy_info->hw_cap[0])
>> + return 0;
>>
>> if (VIR_ALLOC(cpu) < 0)
>> goto error;
>> @@ -77,15 +157,26 @@ libxlCapsInitCPU(virCapsPtr caps, libxl_physinfo *phy_info)
>> virCapabilitiesAddHostFeature(caps, "pae") < 0)
>> goto error;
>>
>> + host_lm = phy_info->hw_cap[version > 0 ? 2 : 1] & LIBXL_X86_FEATURE_LM_MASK;
>> + if (host_lm)
>> + cpu->arch = VIR_ARCH_X86_64;
>> + else
>> + cpu->arch = VIR_ARCH_I686;
>> +
>> cpu->type = VIR_CPU_TYPE_HOST;
>> cpu->cores = phy_info->cores_per_socket;
>> cpu->threads = phy_info->threads_per_core;
>> cpu->sockets = phy_info->nr_cpus / (cpu->cores * cpu->threads);
>> caps->host.cpu = cpu;
>>
>> + if (!(data = libxlCapsNodeData(cpu, phy_info->hw_cap, version))
>> + || cpuDecode(cpu, data, NULL, 0, NULL) < 0)
>> + goto cleanup;
>> +
>> ret = 0;
>>
>> cleanup:
>> + cpuDataFree(data);
>>
>> return ret;
>>
>> @@ -97,7 +188,9 @@ libxlCapsInitCPU(virCapsPtr caps, libxl_physinfo *phy_info)
>> static int
>> libxlCapsInitHost(libxl_ctx *ctx, virCapsPtr caps)
>> {
>> + const libxl_version_info *ver_info;
>> libxl_physinfo phy_info;
>> + unsigned version;
>>
>> if (libxl_get_physinfo(ctx, &phy_info) != 0) {
>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> @@ -105,8 +198,15 @@ libxlCapsInitHost(libxl_ctx *ctx, virCapsPtr caps)
>> return -1;
>> }
>>
>> - if (libxlCapsInitCPU(caps, &phy_info) < 0)
>> + if ((ver_info = libxl_get_version_info(ctx)) == NULL) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Failed to get version info from libxenlight"));
>> return -1;
>> + }
>> +
>> + version = (ver_info->xen_version_minor >= 7);
>> + if (libxlCapsInitCPU(caps, &phy_info, version) < 0)
>> + VIR_WARN("Failed to initialize host cpu data");
>>
>> if (virCapabilitiesSetNetPrefix(caps, LIBXL_GENERATED_PREFIX_XEN) < 0)
>> return -1;
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list