diff mbox series

[kvm-unit-tests,v6,1/2] s390x: topology: Check the Perform Topology Function

Message ID 20230202092814.151081-2-pmorel@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series S390x: CPU Topology Information | expand

Commit Message

Pierre Morel Feb. 2, 2023, 9:28 a.m. UTC
We check that the PTF instruction is working correctly when
the cpu topology facility is available.

For KVM only, we test changing of the polarity between horizontal
and vertical and that a reset set the horizontal polarity.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 s390x/Makefile      |   1 +
 s390x/topology.c    | 155 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   3 +
 3 files changed, 159 insertions(+)
 create mode 100644 s390x/topology.c

Comments

Thomas Huth Feb. 8, 2023, 11:06 a.m. UTC | #1
On 02/02/2023 10.28, Pierre Morel wrote:
> We check that the PTF instruction is working correctly when
> the cpu topology facility is available.
> 
> For KVM only, we test changing of the polarity between horizontal
> and vertical and that a reset set the horizontal polarity.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   s390x/Makefile      |   1 +
>   s390x/topology.c    | 155 ++++++++++++++++++++++++++++++++++++++++++++
>   s390x/unittests.cfg |   3 +
>   3 files changed, 159 insertions(+)
>   create mode 100644 s390x/topology.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 52a9d82..b5fe8a3 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -39,6 +39,7 @@ tests += $(TEST_DIR)/panic-loop-extint.elf
>   tests += $(TEST_DIR)/panic-loop-pgm.elf
>   tests += $(TEST_DIR)/migration-sck.elf
>   tests += $(TEST_DIR)/exittime.elf
> +tests += $(TEST_DIR)/topology.elf
>   
>   pv-tests += $(TEST_DIR)/pv-diags.elf
>   
> diff --git a/s390x/topology.c b/s390x/topology.c
> new file mode 100644
> index 0000000..20f7ba2
> --- /dev/null
> +++ b/s390x/topology.c
> @@ -0,0 +1,155 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * CPU Topology
> + *
> + * Copyright IBM Corp. 2022
> + *
> + * Authors:
> + *  Pierre Morel <pmorel@linux.ibm.com>
> + */
> +
> +#include <libcflat.h>
> +#include <asm/page.h>
> +#include <asm/asm-offsets.h>
> +#include <asm/interrupt.h>
> +#include <asm/facility.h>
> +#include <smp.h>
> +#include <sclp.h>
> +#include <s390x/hardware.h>
> +
> +#define PTF_REQ_HORIZONTAL	0
> +#define PTF_REQ_VERTICAL	1
> +#define PTF_REQ_CHECK		2
> +
> +#define PTF_ERR_NO_REASON	0
> +#define PTF_ERR_ALRDY_POLARIZED	1
> +#define PTF_ERR_IN_PROGRESS	2
> +
> +extern int diag308_load_reset(u64);
> +
> +static int ptf(unsigned long fc, unsigned long *rc)
> +{
> +	int cc;
> +
> +	asm volatile(
> +		"       .insn   rre,0xb9a20000,%1,0\n"

Why are you specifying the instruction manually? I think both, GCC and Clang 
should know the "ptf" mnemonic, shouldn't they?

> +		"       ipm     %0\n"
> +		"       srl     %0,28\n"
> +		: "=d" (cc), "+d" (fc)
> +		:
> +		: "cc");
> +
> +	*rc = fc >> 8;
> +	return cc;
> +}
> +
> +static void test_ptf(void)
> +{
> +	unsigned long rc;
> +	int cc;
> +
> +	/* PTF is a privilege instruction */

s/privilege/privileged/ ?

> +	report_prefix_push("Privilege");
> +	enter_pstate();
> +	expect_pgm_int();
> +	ptf(PTF_REQ_CHECK, &rc);
> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("Wrong fc");
> +	expect_pgm_int();
> +	ptf(0xff, &rc);
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("Reserved bits");
> +	expect_pgm_int();
> +	ptf(0xffffffffffffff00UL, &rc);
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();

This function is quite big ... I'd maybe group the above checks for error 
conditions into a separate function instead.

> +	report_prefix_push("Topology Report pending");
> +	/*
> +	 * At this moment the topology may already have changed
> +	 * since the VM has been started.
> +	 * However, we can test if a second PTF instruction
> +	 * reports that the topology did not change since the
> +	 * preceding PFT instruction.
> +	 */
> +	ptf(PTF_REQ_CHECK, &rc);
> +	cc = ptf(PTF_REQ_CHECK, &rc);
> +	report(cc == 0, "PTF check should clear topology report");
> +	report_prefix_pop();
> +
> +	report_prefix_push("Topology polarisation check");
> +	/*
> +	 * We can not assume the state of the polarization for

s/can not/cannot/ ?

Also, you sometimes write polarization with "z" and sometimes with "s". I'd 
suggest to standardize on "z" (as in "IBM Z" ;-))

> +	 * any Virtual Machine but KVM.
> +	 * Let's skip the polarisation tests for other VMs.
> +	 */
> +	if (!host_is_kvm()) {
> +		report_skip("Topology polarisation check is done for KVM only");
> +		goto end;
> +	}
> +
> +	/*
> +	 * Set vertical polarization to verify that RESET sets
> +	 * horizontal polarization back.
> +	 */
> +	cc = ptf(PTF_REQ_VERTICAL, &rc);
> +	report(cc == 0, "Set vertical polarization.");
> +
> +	report(diag308_load_reset(1), "load normal reset done");
> +
> +	cc = ptf(PTF_REQ_CHECK, &rc);
> +	report(cc == 0, "Reset should clear topology report");
> +
> +	cc = ptf(PTF_REQ_HORIZONTAL, &rc);
> +	report(cc == 2 && rc == PTF_ERR_ALRDY_POLARIZED,
> +	       "After RESET polarization is horizontal");
> +
> +	/* Flip between vertical and horizontal polarization */
> +	cc = ptf(PTF_REQ_VERTICAL, &rc);
> +	report(cc == 0, "Change to vertical polarization.");
> +
> +	cc = ptf(PTF_REQ_CHECK, &rc);
> +	report(cc == 1, "Polarization change should set topology report");
> +
> +	cc = ptf(PTF_REQ_HORIZONTAL, &rc);
> +	report(cc == 0, "Change to horizontal polarization.");
> +
> +end:
> +	report_prefix_pop();
> +}

Apart from the nits, the patch looks fine to me.

  Thomas
Pierre Morel Feb. 10, 2023, 2:38 p.m. UTC | #2
On 2/8/23 12:06, Thomas Huth wrote:
> On 02/02/2023 10.28, Pierre Morel wrote:
>> We check that the PTF instruction is working correctly when
>> the cpu topology facility is available.
>>
>> For KVM only, we test changing of the polarity between horizontal
>> and vertical and that a reset set the horizontal polarity.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   s390x/Makefile      |   1 +
>>   s390x/topology.c    | 155 ++++++++++++++++++++++++++++++++++++++++++++
>>   s390x/unittests.cfg |   3 +
>>   3 files changed, 159 insertions(+)
>>   create mode 100644 s390x/topology.c
>>
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index 52a9d82..b5fe8a3 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -39,6 +39,7 @@ tests += $(TEST_DIR)/panic-loop-extint.elf
>>   tests += $(TEST_DIR)/panic-loop-pgm.elf
>>   tests += $(TEST_DIR)/migration-sck.elf
>>   tests += $(TEST_DIR)/exittime.elf
>> +tests += $(TEST_DIR)/topology.elf
>>   pv-tests += $(TEST_DIR)/pv-diags.elf
>> diff --git a/s390x/topology.c b/s390x/topology.c
>> new file mode 100644
>> index 0000000..20f7ba2
>> --- /dev/null
>> +++ b/s390x/topology.c
>> @@ -0,0 +1,155 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * CPU Topology
>> + *
>> + * Copyright IBM Corp. 2022
>> + *
>> + * Authors:
>> + *  Pierre Morel <pmorel@linux.ibm.com>
>> + */
>> +
>> +#include <libcflat.h>
>> +#include <asm/page.h>
>> +#include <asm/asm-offsets.h>
>> +#include <asm/interrupt.h>
>> +#include <asm/facility.h>
>> +#include <smp.h>
>> +#include <sclp.h>
>> +#include <s390x/hardware.h>
>> +
>> +#define PTF_REQ_HORIZONTAL    0
>> +#define PTF_REQ_VERTICAL    1
>> +#define PTF_REQ_CHECK        2
>> +
>> +#define PTF_ERR_NO_REASON    0
>> +#define PTF_ERR_ALRDY_POLARIZED    1
>> +#define PTF_ERR_IN_PROGRESS    2
>> +
>> +extern int diag308_load_reset(u64);
>> +
>> +static int ptf(unsigned long fc, unsigned long *rc)
>> +{
>> +    int cc;
>> +
>> +    asm volatile(
>> +        "       .insn   rre,0xb9a20000,%1,0\n"
> 
> Why are you specifying the instruction manually? I think both, GCC and 
> Clang should know the "ptf" mnemonic, shouldn't they?

:D right !

> 
>> +        "       ipm     %0\n"
>> +        "       srl     %0,28\n"
>> +        : "=d" (cc), "+d" (fc)
>> +        :
>> +        : "cc");
>> +
>> +    *rc = fc >> 8;
>> +    return cc;
>> +}
>> +
>> +static void test_ptf(void)
>> +{
>> +    unsigned long rc;
>> +    int cc;
>> +
>> +    /* PTF is a privilege instruction */
> 
> s/privilege/privileged/ ?

Yes, thanks

> 
>> +    report_prefix_push("Privilege");
>> +    enter_pstate();
>> +    expect_pgm_int();
>> +    ptf(PTF_REQ_CHECK, &rc);
>> +    check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
>> +    report_prefix_pop();
>> +
>> +    report_prefix_push("Wrong fc");
>> +    expect_pgm_int();
>> +    ptf(0xff, &rc);
>> +    check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> +    report_prefix_pop();
>> +
>> +    report_prefix_push("Reserved bits");
>> +    expect_pgm_int();
>> +    ptf(0xffffffffffffff00UL, &rc);
>> +    check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> +    report_prefix_pop();
> 
> This function is quite big ... I'd maybe group the above checks for 
> error conditions into a separate function instead.

OK

> 
>> +    report_prefix_push("Topology Report pending");
>> +    /*
>> +     * At this moment the topology may already have changed
>> +     * since the VM has been started.
>> +     * However, we can test if a second PTF instruction
>> +     * reports that the topology did not change since the
>> +     * preceding PFT instruction.
>> +     */
>> +    ptf(PTF_REQ_CHECK, &rc);
>> +    cc = ptf(PTF_REQ_CHECK, &rc);
>> +    report(cc == 0, "PTF check should clear topology report");
>> +    report_prefix_pop();
>> +
>> +    report_prefix_push("Topology polarisation check");
>> +    /*
>> +     * We can not assume the state of the polarization for
> 
> s/can not/cannot/ ?

OK

> 
> Also, you sometimes write polarization with "z" and sometimes with "s". 
> I'd suggest to standardize on "z" (as in "IBM Z" ;-))

OK

> 
>> +     * any Virtual Machine but KVM.
>> +     * Let's skip the polarisation tests for other VMs.
>> +     */
>> +    if (!host_is_kvm()) {
>> +        report_skip("Topology polarisation check is done for KVM only");
>> +        goto end;
>> +    }
>> +
>> +    /*
>> +     * Set vertical polarization to verify that RESET sets
>> +     * horizontal polarization back.
>> +     */
>> +    cc = ptf(PTF_REQ_VERTICAL, &rc);
>> +    report(cc == 0, "Set vertical polarization.");
>> +
>> +    report(diag308_load_reset(1), "load normal reset done");
>> +
>> +    cc = ptf(PTF_REQ_CHECK, &rc);
>> +    report(cc == 0, "Reset should clear topology report");
>> +
>> +    cc = ptf(PTF_REQ_HORIZONTAL, &rc);
>> +    report(cc == 2 && rc == PTF_ERR_ALRDY_POLARIZED,
>> +           "After RESET polarization is horizontal");
>> +
>> +    /* Flip between vertical and horizontal polarization */
>> +    cc = ptf(PTF_REQ_VERTICAL, &rc);
>> +    report(cc == 0, "Change to vertical polarization.");
>> +
>> +    cc = ptf(PTF_REQ_CHECK, &rc);
>> +    report(cc == 1, "Polarization change should set topology report");
>> +
>> +    cc = ptf(PTF_REQ_HORIZONTAL, &rc);
>> +    report(cc == 0, "Change to horizontal polarization.");
>> +
>> +end:
>> +    report_prefix_pop();
>> +}
> 
> Apart from the nits, the patch looks fine to me.
> 
>   Thomas
> 

Thanks,

Regards.
Pierre
Nina Schoetterl-Glausch Feb. 10, 2023, 2:51 p.m. UTC | #3
On Thu, 2023-02-02 at 10:28 +0100, Pierre Morel wrote:
> We check that the PTF instruction is working correctly when
> the cpu topology facility is available.
> 
> For KVM only, we test changing of the polarity between horizontal
> and vertical and that a reset set the horizontal polarity.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  s390x/Makefile      |   1 +
>  s390x/topology.c    | 155 ++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |   3 +
>  3 files changed, 159 insertions(+)
>  create mode 100644 s390x/topology.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 52a9d82..b5fe8a3 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -39,6 +39,7 @@ tests += $(TEST_DIR)/panic-loop-extint.elf
>  tests += $(TEST_DIR)/panic-loop-pgm.elf
>  tests += $(TEST_DIR)/migration-sck.elf
>  tests += $(TEST_DIR)/exittime.elf
> +tests += $(TEST_DIR)/topology.elf
>  
>  pv-tests += $(TEST_DIR)/pv-diags.elf
>  
> diff --git a/s390x/topology.c b/s390x/topology.c
> new file mode 100644
> index 0000000..20f7ba2
> --- /dev/null
> +++ b/s390x/topology.c
> @@ -0,0 +1,155 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * CPU Topology
> + *
> + * Copyright IBM Corp. 2022
> + *
> + * Authors:
> + *  Pierre Morel <pmorel@linux.ibm.com>
> + */
> +
> +#include <libcflat.h>
> +#include <asm/page.h>
> +#include <asm/asm-offsets.h>
> +#include <asm/interrupt.h>
> +#include <asm/facility.h>
> +#include <smp.h>
> +#include <sclp.h>
> +#include <s390x/hardware.h>
> +
> +#define PTF_REQ_HORIZONTAL	0
> +#define PTF_REQ_VERTICAL	1
> +#define PTF_REQ_CHECK		2
> +
> +#define PTF_ERR_NO_REASON	0
> +#define PTF_ERR_ALRDY_POLARIZED	1
> +#define PTF_ERR_IN_PROGRESS	2

Maybe also give the CC codes names for improved readability.

> +
> +extern int diag308_load_reset(u64);
> +
> +static int ptf(unsigned long fc, unsigned long *rc)
> +{
> +	int cc;
> +
> +	asm volatile(
> +		"       .insn   rre,0xb9a20000,%1,0\n"
> +		"       ipm     %0\n"
> +		"       srl     %0,28\n"
> +		: "=d" (cc), "+d" (fc)
> +		:
> +		: "cc");

Personally I always name asm arguments, but it is a very short snippet,
so still very readable. Could also pull the shift into C code,
but again, small difference.

> +
> +	*rc = fc >> 8;
> +	return cc;
> +}
> +
> +static void test_ptf(void)
> +{
> +	unsigned long rc;
> +	int cc;
> +
> +	/* PTF is a privilege instruction */
> +	report_prefix_push("Privilege");
> +	enter_pstate();
> +	expect_pgm_int();
> +	ptf(PTF_REQ_CHECK, &rc);
> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +	report_prefix_pop();

IMO, you should repeat this test for all FCs, since some are emulated,
others interpreted by SIE.

> +
> +	report_prefix_push("Wrong fc");

"Undefined fc" is more informative IMO.

> +	expect_pgm_int();
> +	ptf(0xff, &rc);
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("Reserved bits");
> +	expect_pgm_int();
> +	ptf(0xffffffffffffff00UL, &rc);
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +
> +	report_prefix_push("Topology Report pending");
> +	/*
> +	 * At this moment the topology may already have changed
> +	 * since the VM has been started.
> +	 * However, we can test if a second PTF instruction
> +	 * reports that the topology did not change since the
> +	 * preceding PFT instruction.
> +	 */
> +	ptf(PTF_REQ_CHECK, &rc);
> +	cc = ptf(PTF_REQ_CHECK, &rc);
> +	report(cc == 0, "PTF check should clear topology report");
> +	report_prefix_pop();
> +
> +	report_prefix_push("Topology polarisation check");
> +	/*
> +	 * We can not assume the state of the polarization for
> +	 * any Virtual Machine but KVM.

Random Capitalization :)
Why can you not test the same thing for other hypervisors/LPAR?

> +	 * Let's skip the polarisation tests for other VMs.
> +	 */
> +	if (!host_is_kvm()) {
> +		report_skip("Topology polarisation check is done for KVM only");
> +		goto end;
> +	}
> +
> +	/*
> +	 * Set vertical polarization to verify that RESET sets
> +	 * horizontal polarization back.
> +	 */

You might want to do a reset here also, since there could be some other
test case that could have run before and modified the polarization.
There isn't right now of course, but doing a reset improves separation of tests.

> +	cc = ptf(PTF_REQ_VERTICAL, &rc);
> +	report(cc == 0, "Set vertical polarization.");
> +
> +	report(diag308_load_reset(1), "load normal reset done");
> +
> +	cc = ptf(PTF_REQ_CHECK, &rc);
> +	report(cc == 0, "Reset should clear topology report");
> +
> +	cc = ptf(PTF_REQ_HORIZONTAL, &rc);
> +	report(cc == 2 && rc == PTF_ERR_ALRDY_POLARIZED,
> +	       "After RESET polarization is horizontal");
> +
> +	/* Flip between vertical and horizontal polarization */
> +	cc = ptf(PTF_REQ_VERTICAL, &rc);
> +	report(cc == 0, "Change to vertical polarization.");
> +
> +	cc = ptf(PTF_REQ_CHECK, &rc);
> +	report(cc == 1, "Polarization change should set topology report");
> +
> +	cc = ptf(PTF_REQ_HORIZONTAL, &rc);
> +	report(cc == 0, "Change to horizontal polarization.");
> +
> +end:
> +	report_prefix_pop();
> +}
> +
> +static struct {
> +	const char *name;
> +	void (*func)(void);
> +} tests[] = {
> +	{ "PTF", test_ptf},
> +	{ NULL, NULL }
> +};
> +
> +int main(int argc, char *argv[])
> +{
> +	int i;
> +
> +	report_prefix_push("CPU Topology");
> +
> +	if (!test_facility(11)) {
> +		report_skip("Topology facility not present");
> +		goto end;
> +	}
> +
> +	report_info("Virtual machine level %ld", stsi_get_fc());
> +
> +	for (i = 0; tests[i].name; i++) {
> +		report_prefix_push(tests[i].name);
> +		tests[i].func();
> +		report_prefix_pop();
> +	}
> +
> +end:
> +	report_prefix_pop();
> +	return report_summary();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index 3caf81e..3530cc4 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -208,3 +208,6 @@ groups = migration
>  [exittime]
>  file = exittime.elf
>  smp = 2
> +
> +[topology]
> +file = topology.elf
Pierre Morel Feb. 15, 2023, 8:20 a.m. UTC | #4
On 2/10/23 15:51, Nina Schoetterl-Glausch wrote:
> On Thu, 2023-02-02 at 10:28 +0100, Pierre Morel wrote:
>> We check that the PTF instruction is working correctly when
>> the cpu topology facility is available.
>>
>> For KVM only, we test changing of the polarity between horizontal
>> and vertical and that a reset set the horizontal polarity.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   s390x/Makefile      |   1 +
>>   s390x/topology.c    | 155 ++++++++++++++++++++++++++++++++++++++++++++
>>   s390x/unittests.cfg |   3 +
>>   3 files changed, 159 insertions(+)
>>   create mode 100644 s390x/topology.c
>>
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index 52a9d82..b5fe8a3 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -39,6 +39,7 @@ tests += $(TEST_DIR)/panic-loop-extint.elf
>>   tests += $(TEST_DIR)/panic-loop-pgm.elf
>>   tests += $(TEST_DIR)/migration-sck.elf
>>   tests += $(TEST_DIR)/exittime.elf
>> +tests += $(TEST_DIR)/topology.elf
>>   
>>   pv-tests += $(TEST_DIR)/pv-diags.elf
>>   
>> diff --git a/s390x/topology.c b/s390x/topology.c
>> new file mode 100644
>> index 0000000..20f7ba2
>> --- /dev/null
>> +++ b/s390x/topology.c
>> @@ -0,0 +1,155 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * CPU Topology
>> + *
>> + * Copyright IBM Corp. 2022
>> + *
>> + * Authors:
>> + *  Pierre Morel <pmorel@linux.ibm.com>
>> + */
>> +
>> +#include <libcflat.h>
>> +#include <asm/page.h>
>> +#include <asm/asm-offsets.h>
>> +#include <asm/interrupt.h>
>> +#include <asm/facility.h>
>> +#include <smp.h>
>> +#include <sclp.h>
>> +#include <s390x/hardware.h>
>> +
>> +#define PTF_REQ_HORIZONTAL	0
>> +#define PTF_REQ_VERTICAL	1
>> +#define PTF_REQ_CHECK		2
>> +
>> +#define PTF_ERR_NO_REASON	0
>> +#define PTF_ERR_ALRDY_POLARIZED	1
>> +#define PTF_ERR_IN_PROGRESS	2
> 
> Maybe also give the CC codes names for improved readability.

OK

> 
>> +
>> +extern int diag308_load_reset(u64);
>> +
>> +static int ptf(unsigned long fc, unsigned long *rc)
>> +{
>> +	int cc;
>> +
>> +	asm volatile(
>> +		"       .insn   rre,0xb9a20000,%1,0\n"
>> +		"       ipm     %0\n"
>> +		"       srl     %0,28\n"
>> +		: "=d" (cc), "+d" (fc)
>> +		:
>> +		: "cc");
> 
> Personally I always name asm arguments, but it is a very short snippet,
> so still very readable. Could also pull the shift into C code,
> but again, small difference.
> 
>> +
>> +	*rc = fc >> 8;
>> +	return cc;
>> +}
>> +
>> +static void test_ptf(void)
>> +{
>> +	unsigned long rc;
>> +	int cc;
>> +
>> +	/* PTF is a privilege instruction */
>> +	report_prefix_push("Privilege");
>> +	enter_pstate();
>> +	expect_pgm_int();
>> +	ptf(PTF_REQ_CHECK, &rc);
>> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
>> +	report_prefix_pop();
> 
> IMO, you should repeat this test for all FCs, since some are emulated,
> others interpreted by SIE.

right

> 
>> +
>> +	report_prefix_push("Wrong fc");
> 
> "Undefined fc" is more informative IMO.

OK

> 
>> +	expect_pgm_int();
>> +	ptf(0xff, &rc);
>> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> +	report_prefix_pop();
>> +
>> +	report_prefix_push("Reserved bits");
>> +	expect_pgm_int();
>> +	ptf(0xffffffffffffff00UL, &rc);
>> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> +	report_prefix_pop();
>> +
>> +	report_prefix_push("Topology Report pending");
>> +	/*
>> +	 * At this moment the topology may already have changed
>> +	 * since the VM has been started.
>> +	 * However, we can test if a second PTF instruction
>> +	 * reports that the topology did not change since the
>> +	 * preceding PFT instruction.
>> +	 */
>> +	ptf(PTF_REQ_CHECK, &rc);
>> +	cc = ptf(PTF_REQ_CHECK, &rc);
>> +	report(cc == 0, "PTF check should clear topology report");
>> +	report_prefix_pop();
>> +
>> +	report_prefix_push("Topology polarisation check");
>> +	/*
>> +	 * We can not assume the state of the polarization for
>> +	 * any Virtual Machine but KVM.
> 
> Random Capitalization :)

OK

> Why can you not test the same thing for other hypervisors/LPAR?

At first QEMU did not support vertical polarization so my tests would 
have get a false negative on LPAR.
I could have done different tests but did not.

I think that now it is alright to do the checks on LPAR too.


> 
>> +	 * Let's skip the polarisation tests for other VMs.
>> +	 */
>> +	if (!host_is_kvm()) {
>> +		report_skip("Topology polarisation check is done for KVM only");
>> +		goto end;
>> +	}
>> +
>> +	/*
>> +	 * Set vertical polarization to verify that RESET sets
>> +	 * horizontal polarization back.
>> +	 */
> 
> You might want to do a reset here also, since there could be some other
> test case that could have run before and modified the polarization.
> There isn't right now of course, but doing a reset improves separation of tests.

Not sure about this but it does not arm so why not.

Thanks.

regards,
Pierre
diff mbox series

Patch

diff --git a/s390x/Makefile b/s390x/Makefile
index 52a9d82..b5fe8a3 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -39,6 +39,7 @@  tests += $(TEST_DIR)/panic-loop-extint.elf
 tests += $(TEST_DIR)/panic-loop-pgm.elf
 tests += $(TEST_DIR)/migration-sck.elf
 tests += $(TEST_DIR)/exittime.elf
+tests += $(TEST_DIR)/topology.elf
 
 pv-tests += $(TEST_DIR)/pv-diags.elf
 
diff --git a/s390x/topology.c b/s390x/topology.c
new file mode 100644
index 0000000..20f7ba2
--- /dev/null
+++ b/s390x/topology.c
@@ -0,0 +1,155 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * CPU Topology
+ *
+ * Copyright IBM Corp. 2022
+ *
+ * Authors:
+ *  Pierre Morel <pmorel@linux.ibm.com>
+ */
+
+#include <libcflat.h>
+#include <asm/page.h>
+#include <asm/asm-offsets.h>
+#include <asm/interrupt.h>
+#include <asm/facility.h>
+#include <smp.h>
+#include <sclp.h>
+#include <s390x/hardware.h>
+
+#define PTF_REQ_HORIZONTAL	0
+#define PTF_REQ_VERTICAL	1
+#define PTF_REQ_CHECK		2
+
+#define PTF_ERR_NO_REASON	0
+#define PTF_ERR_ALRDY_POLARIZED	1
+#define PTF_ERR_IN_PROGRESS	2
+
+extern int diag308_load_reset(u64);
+
+static int ptf(unsigned long fc, unsigned long *rc)
+{
+	int cc;
+
+	asm volatile(
+		"       .insn   rre,0xb9a20000,%1,0\n"
+		"       ipm     %0\n"
+		"       srl     %0,28\n"
+		: "=d" (cc), "+d" (fc)
+		:
+		: "cc");
+
+	*rc = fc >> 8;
+	return cc;
+}
+
+static void test_ptf(void)
+{
+	unsigned long rc;
+	int cc;
+
+	/* PTF is a privilege instruction */
+	report_prefix_push("Privilege");
+	enter_pstate();
+	expect_pgm_int();
+	ptf(PTF_REQ_CHECK, &rc);
+	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
+	report_prefix_pop();
+
+	report_prefix_push("Wrong fc");
+	expect_pgm_int();
+	ptf(0xff, &rc);
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("Reserved bits");
+	expect_pgm_int();
+	ptf(0xffffffffffffff00UL, &rc);
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+
+	report_prefix_push("Topology Report pending");
+	/*
+	 * At this moment the topology may already have changed
+	 * since the VM has been started.
+	 * However, we can test if a second PTF instruction
+	 * reports that the topology did not change since the
+	 * preceding PFT instruction.
+	 */
+	ptf(PTF_REQ_CHECK, &rc);
+	cc = ptf(PTF_REQ_CHECK, &rc);
+	report(cc == 0, "PTF check should clear topology report");
+	report_prefix_pop();
+
+	report_prefix_push("Topology polarisation check");
+	/*
+	 * We can not assume the state of the polarization for
+	 * any Virtual Machine but KVM.
+	 * Let's skip the polarisation tests for other VMs.
+	 */
+	if (!host_is_kvm()) {
+		report_skip("Topology polarisation check is done for KVM only");
+		goto end;
+	}
+
+	/*
+	 * Set vertical polarization to verify that RESET sets
+	 * horizontal polarization back.
+	 */
+	cc = ptf(PTF_REQ_VERTICAL, &rc);
+	report(cc == 0, "Set vertical polarization.");
+
+	report(diag308_load_reset(1), "load normal reset done");
+
+	cc = ptf(PTF_REQ_CHECK, &rc);
+	report(cc == 0, "Reset should clear topology report");
+
+	cc = ptf(PTF_REQ_HORIZONTAL, &rc);
+	report(cc == 2 && rc == PTF_ERR_ALRDY_POLARIZED,
+	       "After RESET polarization is horizontal");
+
+	/* Flip between vertical and horizontal polarization */
+	cc = ptf(PTF_REQ_VERTICAL, &rc);
+	report(cc == 0, "Change to vertical polarization.");
+
+	cc = ptf(PTF_REQ_CHECK, &rc);
+	report(cc == 1, "Polarization change should set topology report");
+
+	cc = ptf(PTF_REQ_HORIZONTAL, &rc);
+	report(cc == 0, "Change to horizontal polarization.");
+
+end:
+	report_prefix_pop();
+}
+
+static struct {
+	const char *name;
+	void (*func)(void);
+} tests[] = {
+	{ "PTF", test_ptf},
+	{ NULL, NULL }
+};
+
+int main(int argc, char *argv[])
+{
+	int i;
+
+	report_prefix_push("CPU Topology");
+
+	if (!test_facility(11)) {
+		report_skip("Topology facility not present");
+		goto end;
+	}
+
+	report_info("Virtual machine level %ld", stsi_get_fc());
+
+	for (i = 0; tests[i].name; i++) {
+		report_prefix_push(tests[i].name);
+		tests[i].func();
+		report_prefix_pop();
+	}
+
+end:
+	report_prefix_pop();
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 3caf81e..3530cc4 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -208,3 +208,6 @@  groups = migration
 [exittime]
 file = exittime.elf
 smp = 2
+
+[topology]
+file = topology.elf