Re: [libvirt] [PATCH RFC 2/5] libxl: describe host cpu features based on hwcaps

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(a)oracle.com&gt; > --- > 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.

> > 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?

Other than that, the patch looks good to me.

-- 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;