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: 7b79ee2f78
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This commit is contained in:
Daniel Henrique Barboza 2020-03-16 21:01:34 -03:00 committed by Michal Privoznik
parent 008abeb03c
commit 0137bf0dab

View File

@ -220,16 +220,23 @@ virHostCPUGetSocket(unsigned int cpu, unsigned int *socket)
int
virHostCPUGetDie(unsigned int cpu, unsigned int *die)
{
int ret = virFileReadValueUint(die,
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;
}