Re: [kvm-unit-tests PATCH v6 2/2] s390x: topology: Checking Configuration Topology Information

[Thomas Huth <thuth@redhat.com>]

On 02/02/2023 10.28, Pierre Morel wrote:
> STSI with function code 15 is used to store the CPU configuration
> topology.
> 
> We retrieve the maximum nested level with SCLP and use the
> topology tree provided by the drawers, books, sockets, cores
> arguments.
> 
> We check :
> - if the topology stored is coherent between the QEMU -smp
>    parameters and kernel parameters.
> - the number of CPUs
> - the maximum number of CPUs
> - the number of containers of each levels for every STSI(15.1.x)
>    instruction allowed by the machine.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
...
> +static inline int cpus_in_tle_mask(uint64_t val)
> +{
> +	int i, n;
> +
> +	for (i = 0, n = 0; i < 64; i++, val >>= 1)
> +		if (val & 0x01)
> +			n++;
> +	return n;

I'd suggest to use __builtin_popcountl here instead of looping.

> +}
> +
>   #endif  /* _S390X_STSI_H_ */
> diff --git a/s390x/topology.c b/s390x/topology.c
> index 20f7ba2..f21c653 100644
> --- a/s390x/topology.c
> +++ b/s390x/topology.c
> @@ -16,6 +16,18 @@
>   #include <smp.h>
>   #include <sclp.h>
>   #include <s390x/hardware.h>
> +#include <s390x/stsi.h>
> +
> +static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));

Isn't the SYSIB just one page only? Why reserve two pages here?

> +static int max_nested_lvl;
> +static int number_of_cpus;
> +static int max_cpus = 1;
> +
> +/* Topology level as defined by architecture */
> +static int arch_topo_lvl[CPU_TOPOLOGY_MAX_LEVEL];
> +/* Topology nested level as reported in STSI */
> +static int stsi_nested_lvl[CPU_TOPOLOGY_MAX_LEVEL];
>   
>   #define PTF_REQ_HORIZONTAL	0
>   #define PTF_REQ_VERTICAL	1
> @@ -122,11 +134,241 @@ end:
>   	report_prefix_pop();
>   }
>   
> +/*
> + * stsi_check_maxcpus
> + * @info: Pointer to the stsi information
> + *
> + * The product of the numbers of containers per level
> + * is the maximum number of CPU allowed by the machine.
> + */
> +static void stsi_check_maxcpus(struct sysinfo_15_1_x *info)
> +{
> +	int n, i;
> +
> +	report_prefix_push("maximum cpus");
> +
> +	for (i = 0, n = 1; i < CPU_TOPOLOGY_MAX_LEVEL; i++) {
> +		report_info("Mag%d: %d", CPU_TOPOLOGY_MAX_LEVEL - i, info->mag[i]);
> +		n *= info->mag[i] ? info->mag[i] : 1;

You could use the Elvis operator here instead.

> +	}
> +	report(n == max_cpus, "Maximum CPUs %d expected %d", n, max_cpus);
> +
> +	report_prefix_pop();
> +}
> +
> +/*
> + * stsi_check_tle_coherency
> + * @info: Pointer to the stsi information
> + * @sel2: Topology level to check.
> + *
> + * We verify that we get the expected number of Topology List Entry
> + * containers for a specific level.
> + */
> +static void stsi_check_tle_coherency(struct sysinfo_15_1_x *info, int sel2)
> +{
> +	struct topology_container *tc, *end;
> +	struct topology_core *cpus;
> +	int n = 0;
> +	int i;
> +
> +	report_prefix_push("TLE coherency");
> +
> +	tc = &info->tle[0].container;
> +	end = (struct topology_container *)((unsigned long)info + info->length);

s/unsigned long/uintptr_t/ please!


> +
> +	for (i = 0; i < CPU_TOPOLOGY_MAX_LEVEL; i++)
> +		stsi_nested_lvl[i] = 0;

memset(stsi_nested_lvl, 0, sizeof(stsi_nested_lvl)) ?

> +	while (tc < end) {
> +		if (tc->nl > 5) {

Use ">= CPU_TOPOLOGY_MAX_LEVEL" instead of "> 5" ?

> +			report_abort("Unexpected TL Entry: tle->nl: %d", tc->nl);
> +			return;
> +		}
> +		if (tc->nl == 0) {
> +			cpus = (struct topology_core *)tc;
> +			n += cpus_in_tle_mask(cpus->mask);
> +			report_info("cpu type %02x  d: %d pp: %d", cpus->type, cpus->d, cpus->pp);
> +			report_info("origin : %04x mask %016lx", cpus->origin, cpus->mask);
> +		}
> +
> +		stsi_nested_lvl[tc->nl]++;
> +		report_info("level %d: lvl: %d id: %d cnt: %d",
> +			    tc->nl, tc->nl, tc->id, stsi_nested_lvl[tc->nl]);
> +
> +		/* trick: CPU TLEs are twice the size of containers TLE */
> +		if (tc->nl == 0)
> +			tc++;

IMHO it might be cleaner to have a "uint8_t *" or "void *" to the current 
position in the sysinfo block, and do the pointer arithmetic on that pointer 
instead... well, it's likely just a matter of taste.

> +		tc++;
> +	}
> +	report(n == number_of_cpus, "Number of CPUs  : %d expect %d", n, number_of_cpus);
> +	/*
> +	 * For KVM we accept
> +	 * - only 1 type of CPU
> +	 * - only horizontal topology
> +	 * - only dedicated CPUs
> +	 * This leads to expect the number of entries of level 0 CPU
> +	 * Topology Level Entry (TLE) to be:
> +	 * 1 + (number_of_cpus - 1)  / arch_topo_lvl[0]
> +	 *
> +	 * For z/VM or LPAR this number can only be greater if different
> +	 * polarity, CPU types because there may be a nested level 0 CPU TLE
> +	 * for each of the CPU/polarity/sharing types in a level 1 container TLE.
> +	 */
> +	n =  (number_of_cpus - 1)  / arch_topo_lvl[0];
> +	report(stsi_nested_lvl[0] >=  n + 1,
> +	       "CPU Type TLE    : %d expect %d", stsi_nested_lvl[0], n + 1);
> +
> +	/* For each level found in STSI */
> +	for (i = 1; i < CPU_TOPOLOGY_MAX_LEVEL; i++) {
> +		/*
> +		 * For non QEMU/KVM hypervisor the concatenation of the levels
> +		 * above level 1 are architecture dependent.
> +		 * Skip these checks.
> +		 */
> +		if (!host_is_kvm() && sel2 != 2)
> +			continue;
> +
> +		/* For QEMU/KVM we expect a simple calculation */
> +		if (sel2 > i) {
> +			report(stsi_nested_lvl[i] ==  n + 1,
> +			       "Container TLE  %d: %d expect %d", i, stsi_nested_lvl[i], n + 1);
> +			n /= arch_topo_lvl[i];
> +		}
> +	}
> +
> +	report_prefix_pop();
> +}
> +
> +/*
> + * check_sysinfo_15_1_x
> + * @info: pointer to the STSI info structure
> + * @sel2: the selector giving the topology level to check
> + *
> + * Check if the validity of the STSI instruction and then
> + * calls specific checks on the information buffer.
> + */
> +static void check_sysinfo_15_1_x(struct sysinfo_15_1_x *info, int sel2)
> +{
> +	int ret;
> +
> +	report_prefix_pushf("mnested %d 15_1_%d", max_nested_lvl, sel2);
> +
> +	ret = stsi(pagebuf, 15, 1, sel2);
> +	if (max_nested_lvl >= sel2) {
> +		report(!ret, "Valid stsi instruction");
> +	} else {
> +		report(ret, "Invalid stsi instruction");
> +		goto end;
> +	}
> +
> +	stsi_check_maxcpus(info);
> +	stsi_check_tle_coherency(info, sel2);

You could also move the two stsi_check_* calls into the first part of the 
if-statement, then you could get rid of the goto in the second part.

> +end:
> +	report_prefix_pop();
> +}
> +
> +static int sclp_get_mnest(void)
> +{
> +	ReadInfo *sccb = (void *)_sccb;
> +
> +	sclp_mark_busy();
> +	memset(_sccb, 0, PAGE_SIZE);
> +	sccb->h.length = PAGE_SIZE;
> +
> +	sclp_service_call(SCLP_CMDW_READ_SCP_INFO, sccb);
> +	assert(sccb->h.response_code == SCLP_RC_NORMAL_READ_COMPLETION);
> +
> +	return sccb->stsi_parm;
> +}
> +
> +/*
> + * test_stsi
> + *
> + * Retrieves the maximum nested topology level supported by the architecture
> + * and the number of CPUs.
> + * Calls the checking for the STSI instruction in sel2 reverse level order
> + * from 6 (CPU_TOPOLOGY_MAX_LEVEL) to 2 to have the most interesting level,
> + * the one triggering a topology-change-report-pending condition, level 2,
> + * at the end of the report.
> + *
> + */
> +static void test_stsi(void)
> +{
> +	int sel2;
> +
> +	max_nested_lvl = sclp_get_mnest();
> +	report_info("SCLP maximum nested level : %d", max_nested_lvl);
> +
> +	number_of_cpus = sclp_get_cpu_num();
> +	report_info("SCLP number of CPU: %d", number_of_cpus);
> +
> +	/* STSI selector 2 can takes values between 2 and 6 */
> +	for (sel2 = 6; sel2 >= 2; sel2--)
> +		check_sysinfo_15_1_x((struct sysinfo_15_1_x *)pagebuf, sel2);
> +}
> +
> +/*
> + * parse_topology_args
> + * @argc: number of arguments
> + * @argv: argument array
> + *
> + * This function initialize the architecture topology levels
> + * which should be the same as the one provided by the hypervisor.
> + *
> + * We use the current names found in IBM/Z literature, Linux and QEMU:
> + * cores, sockets/packages, books, drawers and nodes to facilitate the
> + * human machine interface but store the result in a machine abstract
> + * array of architecture topology levels.
> + * Note that when QEMU uses socket as a name for the topology level 1
> + * Linux uses package or physical_package.
> + */
> +static void parse_topology_args(int argc, char **argv)
> +{
> +	int i;
> +
> +	report_info("%d arguments", argc);
> +	for (i = 1; i < argc; i++) {
> +		if (!strcmp("-cores", argv[i])) {
> +			i++;
> +			if (i >= argc)
> +				report_abort("-cores needs a parameter");
> +			arch_topo_lvl[0] = atol(argv[i]);
> +			report_info("cores: %d", arch_topo_lvl[0]);
> +		} else if (!strcmp("-sockets", argv[i])) {
> +			i++;
> +			if (i >= argc)
> +				report_abort("-sockets needs a parameter");
> +			arch_topo_lvl[1] = atol(argv[i]);
> +			report_info("sockets: %d", arch_topo_lvl[1]);
> +		} else if (!strcmp("-books", argv[i])) {
> +			i++;
> +			if (i >= argc)
> +				report_abort("-books needs a parameter");
> +			arch_topo_lvl[2] = atol(argv[i]);
> +			report_info("books: %d", arch_topo_lvl[2]);
> +		} else if (!strcmp("-drawers", argv[i])) {
> +			i++;
> +			if (i >= argc)
> +				report_abort("-drawers needs a parameter");
> +			arch_topo_lvl[3] = atol(argv[i]);
> +			report_info("drawers: %d", arch_topo_lvl[3]);
> +		}

Maybe abort on unkown parameters, to avoid that typos go unnoticed?

> +	}
> +
> +	for (i = 0; i < CPU_TOPOLOGY_MAX_LEVEL; i++) {
> +		if (!arch_topo_lvl[i])
> +			arch_topo_lvl[i] = 1;
> +		max_cpus *= arch_topo_lvl[i];
> +	}
> +}

  Thomas