[libvirt PATCH 0/6] libxl: remove enum libxlHwcapVersion
While looking at users of virCPU.*Free, I found some code that is supposed to deal with Xen < 4.7. The minimum version is 4.9 since the following commit: commit 68940b3fb3c43b8aa03cb6fd2f1d00b1737c9b2c Author: Jim Fehlig <jfehlig@suse.com> CommitDate: 2021-06-17 10:11:56 -0600 Xen: Bump minimum supported Xen version to 4.9 Remove the code dealing with the old version and use g_auto moer. Ján Tomko (6): libxl: capabilities: assume Xen version >= 4.7 libxl: remove enum libxlHwcapVersion libxl: capsInitCPU: fail if we can't initialize host features libxl: refactor libxlCapsInitCPU libxl: use g_auto in libxlCapsNodeData libxl: use g_auto in libxlDomainGetEmulatorType src/libxl/libxl_capabilities.c | 119 ++++++++++----------------------- 1 file changed, 37 insertions(+), 82 deletions(-) -- 2.31.1
Remove the code handling old Xen's hwcap words, as well as the comment describing it. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libxl/libxl_capabilities.c | 54 +++++++++++++--------------------- 1 file changed, 20 insertions(+), 34 deletions(-) diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c index b4bd1d7e62..1953d7a87a 100644 --- a/src/libxl/libxl_capabilities.c +++ b/src/libxl/libxl_capabilities.c @@ -90,41 +90,33 @@ libxlCapsAddCPUID(virCPUData *data, virCPUx86CPUID *cpuid, ssize_t ncaps) * 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 trivial to keep track of these leafs - * to describe host CPU in libvirt capabilities. v0 stands for Xen 4.4 - * up to 4.6, while v1 is meant for Xen 4.7, as depicted in the table below: + * to describe host CPU in libvirt capabilities. * - * | 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 | + * | Xen >= 4.7 | + * ------------------------ + * word 0 | CPUID.00000001.EDX | + * word 1 | CPUID.00000001.ECX | + * word 2 | CPUID.80000001.EDX | + * word 3 | CPUID.80000001.ECX | + * word 4 | CPUID.0000000D:1.EAX | + * word 5 | CPUID.00000007:0.EBX | + * word 6 | CPUID.00000007:0.ECX | + * word 7 | CPUID.80000007.EDX | + * word 8 | CPUID.80000008.EBX | * */ static virCPUData * libxlCapsNodeData(virCPUDef *cpu, libxl_hwcap hwcap, - enum libxlHwcapVersion version) + enum libxlHwcapVersion version G_GNUC_UNUSED) { ssize_t ncaps; virCPUData *cpudata = NULL; virCPUx86CPUID cpuid[] = { - { .eax_in = 0x00000001, - .edx = hwcap[0] }, - { .eax_in = 0x00000001, - .ecx = (version > LIBXL_HWCAP_V0 ? hwcap[1] : hwcap[4]) }, - { .eax_in = 0x80000001, - .edx = (version > LIBXL_HWCAP_V0 ? hwcap[2] : hwcap[1]) }, - { .eax_in = 0x80000001, - .ecx = (version > LIBXL_HWCAP_V0 ? hwcap[3] : hwcap[6]) }, - { .eax_in = 0x00000007, - .ebx = (version > LIBXL_HWCAP_V0 ? hwcap[5] : hwcap[7]) }, - }; - virCPUx86CPUID cpuid_ver1[] = { + { .eax_in = 0x00000001, .edx = hwcap[0] }, + { .eax_in = 0x00000001, .ecx = hwcap[1] }, + { .eax_in = 0x80000001, .edx = hwcap[2] }, + { .eax_in = 0x80000001, .ecx = hwcap[3] }, + { .eax_in = 0x00000007, .ebx = hwcap[5] }, { .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] }, @@ -137,11 +129,6 @@ libxlCapsNodeData(virCPUDef *cpu, libxl_hwcap hwcap, if (libxlCapsAddCPUID(cpudata, cpuid, ncaps) < 0) goto error; - ncaps = G_N_ELEMENTS(cpuid_ver1); - if (version > LIBXL_HWCAP_V0 && - libxlCapsAddCPUID(cpudata, cpuid_ver1, ncaps) < 0) - goto error; - return cpudata; error: @@ -156,7 +143,7 @@ libxlCapsNodeData(virCPUDef *cpu, libxl_hwcap hwcap, */ static int libxlCapsInitCPU(virCaps *caps, libxl_physinfo *phy_info, - enum libxlHwcapVersion version) + enum libxlHwcapVersion version G_GNUC_UNUSED) { virCPUData *data = NULL; virCPUDef *cpu = NULL; @@ -175,8 +162,7 @@ libxlCapsInitCPU(virCaps *caps, libxl_physinfo *phy_info, virCapabilitiesAddHostFeature(caps, "pae") < 0) goto error; - host_lm = (phy_info->hw_cap[version > LIBXL_HWCAP_V0 ? 2 : 1] - & LIBXL_X86_FEATURE_LM_MASK); + host_lm = (phy_info->hw_cap[2] & LIBXL_X86_FEATURE_LM_MASK); if (host_lm) cpu->arch = VIR_ARCH_X86_64; else -- 2.31.1
As well as the code probing for the version in libxlCapsInitHost. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libxl/libxl_capabilities.c | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c index 1953d7a87a..d98109ea86 100644 --- a/src/libxl/libxl_capabilities.c +++ b/src/libxl/libxl_capabilities.c @@ -45,11 +45,6 @@ VIR_LOG_INIT("libxl.libxl_capabilities"); #define LIBXL_X86_FEATURE_PAE_MASK (1 << 6) #define LIBXL_X86_FEATURE_LM_MASK (1 << 29) -enum libxlHwcapVersion { - LIBXL_HWCAP_V0 = 0, /* for Xen 4.4 .. 4.6 */ - LIBXL_HWCAP_V1, /* for Xen 4.7 and up */ -}; - struct guest_arch { virArch arch; int hvm; @@ -106,8 +101,7 @@ libxlCapsAddCPUID(virCPUData *data, virCPUx86CPUID *cpuid, ssize_t ncaps) * */ static virCPUData * -libxlCapsNodeData(virCPUDef *cpu, libxl_hwcap hwcap, - enum libxlHwcapVersion version G_GNUC_UNUSED) +libxlCapsNodeData(virCPUDef *cpu, libxl_hwcap hwcap) { ssize_t ncaps; virCPUData *cpudata = NULL; @@ -142,8 +136,7 @@ libxlCapsNodeData(virCPUDef *cpu, libxl_hwcap hwcap, * the X'th 32-bit word of hw_cap. */ static int -libxlCapsInitCPU(virCaps *caps, libxl_physinfo *phy_info, - enum libxlHwcapVersion version G_GNUC_UNUSED) +libxlCapsInitCPU(virCaps *caps, libxl_physinfo *phy_info) { virCPUData *data = NULL; virCPUDef *cpu = NULL; @@ -177,7 +170,7 @@ libxlCapsInitCPU(virCaps *caps, libxl_physinfo *phy_info, ret = 0; - if (!(data = libxlCapsNodeData(cpu, phy_info->hw_cap, version)) || + if (!(data = libxlCapsNodeData(cpu, phy_info->hw_cap)) || cpuDecode(cpu, data, NULL) < 0) { VIR_WARN("Failed to initialize host cpu features"); goto error; @@ -196,8 +189,6 @@ libxlCapsInitCPU(virCaps *caps, libxl_physinfo *phy_info, static int libxlCapsInitHost(libxl_ctx *ctx, virCaps *caps) { - const libxl_version_info *ver_info; - enum libxlHwcapVersion version; libxl_physinfo phy_info; int ret = -1; @@ -208,14 +199,7 @@ libxlCapsInitHost(libxl_ctx *ctx, virCaps *caps) goto cleanup; } - if ((ver_info = libxl_get_version_info(ctx)) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Failed to get version info from libxenlight")); - goto cleanup; - } - - version = (ver_info->xen_version_minor >= 7); - if (libxlCapsInitCPU(caps, &phy_info, version) < 0) + if (libxlCapsInitCPU(caps, &phy_info) < 0) goto cleanup; if (virCapabilitiesSetNetPrefix(caps, LIBXL_GENERATED_PREFIX_XEN) < 0) -- 2.31.1
Introduced by: commit 17322e551861a5bc4dd464a7c5399204dc8c7caa libxl: describe host cpu features based on hwcaps with the justification that libxl_hwcaps does not have a stable format across all version. Even though the code would return '0' in the case of such failure, it frees the 'cpu' pointer, while keeping it in caps->host. Based on that, assume it does not happen in current usage. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libxl/libxl_capabilities.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c index d98109ea86..9d0ed921bd 100644 --- a/src/libxl/libxl_capabilities.c +++ b/src/libxl/libxl_capabilities.c @@ -166,16 +166,17 @@ libxlCapsInitCPU(virCaps *caps, libxl_physinfo *phy_info) cpu->threads = phy_info->threads_per_core; cpu->dies = 1; cpu->sockets = phy_info->nr_cpus / (cpu->cores * cpu->threads); - caps->host.cpu = cpu; - - ret = 0; if (!(data = libxlCapsNodeData(cpu, phy_info->hw_cap)) || cpuDecode(cpu, data, NULL) < 0) { - VIR_WARN("Failed to initialize host cpu features"); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to initialize host cpu features")); goto error; } + caps->host.cpu = cpu; + ret = 0; + cleanup: virCPUDataFree(data); -- 2.31.1
Use g_auto, g_steal_pointer and remove unnecessary labels. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libxl/libxl_capabilities.c | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c index 9d0ed921bd..38a9b36d8b 100644 --- a/src/libxl/libxl_capabilities.c +++ b/src/libxl/libxl_capabilities.c @@ -138,9 +138,8 @@ libxlCapsNodeData(virCPUDef *cpu, libxl_hwcap hwcap) static int libxlCapsInitCPU(virCaps *caps, libxl_physinfo *phy_info) { - virCPUData *data = NULL; - virCPUDef *cpu = NULL; - int ret = -1; + g_autoptr(virCPUData) data = NULL; + g_autoptr(virCPUDef) cpu = NULL; int host_pae; int host_lm; @@ -153,7 +152,7 @@ libxlCapsInitCPU(virCaps *caps, libxl_physinfo *phy_info) host_pae = phy_info->hw_cap[0] & LIBXL_X86_FEATURE_PAE_MASK; if (host_pae && virCapabilitiesAddHostFeature(caps, "pae") < 0) - goto error; + return -1; host_lm = (phy_info->hw_cap[2] & LIBXL_X86_FEATURE_LM_MASK); if (host_lm) @@ -171,20 +170,11 @@ libxlCapsInitCPU(virCaps *caps, libxl_physinfo *phy_info) cpuDecode(cpu, data, NULL) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to initialize host cpu features")); - goto error; + return -1; } - caps->host.cpu = cpu; - ret = 0; - - cleanup: - virCPUDataFree(data); - - return ret; - - error: - virCPUDefFree(cpu); - goto cleanup; + caps->host.cpu = g_steal_pointer(&cpu); + return 0; } static int -- 2.31.1
Also remove pointless labels. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libxl/libxl_capabilities.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c index 38a9b36d8b..ef77bec7a0 100644 --- a/src/libxl/libxl_capabilities.c +++ b/src/libxl/libxl_capabilities.c @@ -104,7 +104,7 @@ static virCPUData * libxlCapsNodeData(virCPUDef *cpu, libxl_hwcap hwcap) { ssize_t ncaps; - virCPUData *cpudata = NULL; + g_autoptr(virCPUData) cpudata = NULL; virCPUx86CPUID cpuid[] = { { .eax_in = 0x00000001, .edx = hwcap[0] }, { .eax_in = 0x00000001, .ecx = hwcap[1] }, @@ -117,17 +117,13 @@ libxlCapsNodeData(virCPUDef *cpu, libxl_hwcap hwcap) }; if (!(cpudata = virCPUDataNew(cpu->arch))) - goto error; + return NULL; ncaps = G_N_ELEMENTS(cpuid); if (libxlCapsAddCPUID(cpudata, cpuid, ncaps) < 0) - goto error; + return NULL; - return cpudata; - - error: - virCPUDataFree(cpudata); - return NULL; + return g_steal_pointer(&cpudata); } /* hw_caps is an array of 32-bit words whose meaning is listed in -- 2.31.1
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libxl/libxl_capabilities.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c index ef77bec7a0..a516910215 100644 --- a/src/libxl/libxl_capabilities.c +++ b/src/libxl/libxl_capabilities.c @@ -715,13 +715,13 @@ int libxlDomainGetEmulatorType(const virDomainDef *def) { int ret = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN; - virCommand *cmd = NULL; + g_autoptr(virCommand) cmd = NULL; g_autofree char *output = NULL; if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { if (def->emulator) { if (!virFileExists(def->emulator)) - goto cleanup; + return ret; cmd = virCommandNew(def->emulator); @@ -729,14 +729,12 @@ libxlDomainGetEmulatorType(const virDomainDef *def) virCommandSetOutputBuffer(cmd, &output); if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + return ret; if (strstr(output, LIBXL_QEMU_DM_STR)) ret = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL; } } - cleanup: - virCommandFree(cmd); return ret; } -- 2.31.1
On 9/6/21 08:58, Ján Tomko wrote:
While looking at users of virCPU.*Free, I found some code that is supposed to deal with Xen < 4.7. The minimum version is 4.9 since the following commit:
commit 68940b3fb3c43b8aa03cb6fd2f1d00b1737c9b2c Author: Jim Fehlig <jfehlig@suse.com> CommitDate: 2021-06-17 10:11:56 -0600
Xen: Bump minimum supported Xen version to 4.9
Remove the code dealing with the old version and use g_auto moer.
Ján Tomko (6): libxl: capabilities: assume Xen version >= 4.7 libxl: remove enum libxlHwcapVersion libxl: capsInitCPU: fail if we can't initialize host features libxl: refactor libxlCapsInitCPU libxl: use g_auto in libxlCapsNodeData libxl: use g_auto in libxlDomainGetEmulatorType
src/libxl/libxl_capabilities.c | 119 ++++++++++----------------------- 1 file changed, 37 insertions(+), 82 deletions(-)
Reviewed-by: Jim Fehlig <jfehlig@suse.com> Unrelated to anything in this series, but while testing the patches I noticed odd values for the 'die_id' attribute in 'virsh capabilities' <cpus num='24'> <cpu id='0' socket_id='0' die_id='0' core_id='0' siblings='0-1'/> <cpu id='1' socket_id='0' die_id='-1073069552' core_id='0' siblings='0-1'/> <cpu id='2' socket_id='0' die_id='-1073741696' core_id='1' siblings='2-3'/> <cpu id='3' socket_id='0' die_id='-1073317232' core_id='1' siblings='2-3'/> <cpu id='4' socket_id='0' die_id='0' core_id='2' siblings='4-5'/> ... </cpus> I see commit 7b79ee2f78b uses /sys/devices/system/cpu/cpu$num/topology/die_id to determine the value. On my test machine, all /sys/devices/system/cpu/cpu$num/topology/die_id files contain 0. I'll need to dig a little deeper to determine why this doesn't work on xen. Regards, Jim
participants (2)
-
Jim Fehlig -
Ján Tomko