From b5aa3a33bc770714f8a68954c05ea362fcfd4d47 Mon Sep 17 00:00:00 2001

Message-Id: <b5aa3a33bc770714f8a68954c05ea362fcfd4d47@dist-git>

From: Daniel Henrique Barboza <danielhb413@gmail.com>

Date: Mon, 5 Oct 2020 10:25:30 -0400

Subject: [PATCH] virhostcpu.c: fix 'die_id' parsing for Power hosts

Commit 7b79ee2f78 makes assumptions about die_id parsing in

the sysfs that aren't true for Power hosts. In both Power8

and Power9, running 5.6 and 4.18 kernel respectively,

'die_id' is set to -1:

$ cat /sys/devices/system/cpu/cpu0/topology/die_id

-1

This breaks virHostCPUGetDie() parsing because it is trying to

retrieve an unsigned integer, causing problems during VM start:

virFileReadValueUint:4128 : internal error: Invalid unsigned integer

value '-1' in file '/sys/devices/system/cpu/cpu0/topology/die_id'

This isn't necessarily a PowerPC only behavior. Linux kernel commit

0e344d8c70 added in the former Documentation/cputopology.txt, now

Documentation/admin-guide/cputopology.rst, that:

  To be consistent on all architectures, include/linux/topology.h

  provides default definitions for any of the above macros that are

  not defined by include/asm-XXX/topology.h:

  1) topology_physical_package_id: -1

  2) topology_die_id: -1

  (...)

This means that it might be expected that an architecture that

does not implement the die_id element will mark it as -1 in

sysfs.

It is not required to change die_id implementation from uInt to

Int because of that. Instead, let's change the parsing of the

die_id in virHostCPUGetDie() to read an integer value and, in

case it's -1, default it to zero like in case of file not found.

This is enough to solve the issue Power hosts are experiencing.

Fixes: 7b79ee2f78bbf2af76df2f6466919e19ae05aeeb

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

(cherry picked from commit 0137bf0dab2738d5443e2f407239856e2aa25bb3)

https://bugzilla.redhat.com/show_bug.cgi?id=1876742

Signed-off-by: Daniel Henrique Barboza <dbarboza@redhat.com>

Message-Id: <20201005142530.3961036-1-dbarboza@redhat.com>

Reviewed-by: Jiri Denemark <jdenemar@redhat.com>

---

 src/util/virhostcpu.c | 21 ++++++++++++++-------

 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c

index 09c959cd25..218272d7ec 100644

--- a/src/util/virhostcpu.c

+++ b/src/util/virhostcpu.c

@@ -221,16 +221,23 @@ virHostCPUGetSocket(unsigned int cpu, unsigned int *socket)

 int

 virHostCPUGetDie(unsigned int cpu, unsigned int *die)

 {

-    int ret = virFileReadValueUint(die,

-                                   "%s/cpu/cpu%u/topology/die_id",

-                                   SYSFS_SYSTEM_PATH, cpu);

+    int die_id;

+    int ret = virFileReadValueInt(&die_id,

+                                  "%s/cpu/cpu%u/topology/die_id",

+                                  SYSFS_SYSTEM_PATH, cpu);

-    /* If the file is not there, it's 0 */

-    if (ret == -2)

-        *die = 0;

-    else if (ret < 0)

+    if (ret == -1)

         return -1;

+    /* If the file is not there, it's 0.

+     * Another alternative is die_id set to -1, meaning that

+     * the arch does not have die_id support. Set @die to

+     * 0 in this case too. */

+    if (ret == -2 || die_id < 0)

+        *die = 0;

+    else

+        *die = die_id;

+

     return 0;

 }

-- 

2.28.0