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

[Nico Boehr <nrb@linux.ibm.com>]

Quoting Pierre Morel (2023-03-20 09:56:42)
> 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.
[...]
> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
> index 390fde7..9679332 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -238,3 +238,8 @@ uint64_t get_max_ram_size(void)
>  {
>         return max_ram_size;
>  }
> +
> +uint64_t sclp_get_stsi_mnest(void)
> +{

maybe add:
assert(read_info);

[...]
> diff --git a/s390x/topology.c b/s390x/topology.c
> index ce248f1..11ce931 100644
> --- a/s390x/topology.c
> +++ b/s390x/topology.c
[...]
> +/*
> + * Topology level as defined by architecture, all levels exists with
> + * a single container unless overwritten by the QEMU -smp parameter.
> + */
> +static int arch_topo_lvl[CPU_TOPOLOGY_MAX_LEVEL]; // = {1, 1, 1, 1, 1, 1};

So that's what is being provided to the test on the command line, right?

How about renaming this to expected_topo_lvl?

What do you mean by 'defined by architecture'?

[...]
> +/*
> + * stsi_check_mag
> + * @info: Pointer to the stsi information
> + *
> + * MAG field should match the architecture defined containers
> + * when MNEST as returned by SCLP matches MNEST of the SYSIB.
> + */
> +static void stsi_check_mag(struct sysinfo_15_1_x *info)
> +{
> +       int i;
> +
> +       report_prefix_push("MAG");
> +
> +       stsi_check_maxcpus(info);
> +
> +       /* Explicitly skip the test if both mnest do not match */
> +       if (max_nested_lvl != info->mnest)
> +               goto done;

What does it mean if the two don't match, i.e. is this an error? Or a skip? Or is it just expected?

[...]
> +/**
> + * check_tle:
> + * @tc: pointer to first TLE
> + *
> + * Recursively check the containers TLEs until we
> + * find a CPU TLE.
> + */
> +static uint8_t *check_tle(void *tc)
> +{
[...]
> +       report(!cpus->d || (cpus->pp == 3 || cpus->pp == 0),
> +              "Dedication versus entitlement");

Maybe skip here if the CPU is not dedicated? With shared CPUs we really can't check much here.

[...]
> +/*
> + * The Maximum Nested level is given by SCLP READ_SCP_INFO if the MNEST facility
> + * is available.
> + * If the MNEST facility is not available, sclp_get_stsi_mnest  returns 0 and the
> + * Maximum Nested level is 2
> + */
> +#define S390_DEFAULT_MNEST     2
> +static int sclp_get_mnest(void)
> +{
> +       return sclp_get_stsi_mnest() ?: S390_DEFAULT_MNEST;
> +}
> +
> +static int arch_max_cpus(void)

If arch_topo_lvl is renamed, also rename this function accordingly.

>  static struct {
>         const char *name;
>         void (*func)(void);
>  } tests[] = {
>         { "PTF", test_ptf},
> +       { "STSI", test_stsi},

missing space                ^