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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]