On 13.03.2018 13:01, Janosch Frank wrote:
> The Store Hypervisor Information (STHYI) instruction provides system
> information like available CPU resources on each system level. The
> instruction is fully emulated by the hypervisor.
>
> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxxxxxxx>
> ---
> s390x/Makefile | 1 +
> s390x/sthyi.c | 210 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> s390x/sthyi.h | 131 ++++++++++++++++++++++++++++++++
> s390x/unittests.cfg | 4 +
> 4 files changed, 346 insertions(+)
> create mode 100644 s390x/sthyi.c
> create mode 100644 s390x/sthyi.h
>
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 2d3336c..e062727 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -2,6 +2,7 @@ tests = $(TEST_DIR)/selftest.elf
> tests += $(TEST_DIR)/intercept.elf
> tests += $(TEST_DIR)/emulator.elf
> tests += $(TEST_DIR)/sieve.elf
> +tests += $(TEST_DIR)/sthyi.elf
>
> all: directories test_cases
>
> diff --git a/s390x/sthyi.c b/s390x/sthyi.c
> new file mode 100644
> index 0000000..e53cdb2
> --- /dev/null
> +++ b/s390x/sthyi.c
> @@ -0,0 +1,210 @@
> +/*
> + * Tests exceptions and data validity for the emulated sthyi
> + * instruction.
> + *
> + * Copyright 2017 IBM Corp.
Maybe update that to 2018?
> + * Authors:
> + * Janosch Frank <frankja@xxxxxxxxxxxxxxxxxx>
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Library General Public License version 2.
> + */
> +#include <libcflat.h>
> +#include <asm/asm-offsets.h>
> +#include <asm/interrupt.h>
> +#include <asm/page.h>
> +#include <asm/facility.h>
> +
> +#include "sthyi.h"
> +
> +static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
According to my documentation of sthyi, one 4k page should be enough?
Why two pages?
> +static char null_buf[32] = {};
> +
> +static int sthyi(uint64_t vaddr, uint64_t fcode)
> +{
> + register uint64_t code asm("0") = fcode;
> + register uint64_t dummy1 asm("1") = 0;
> + register uint64_t addr asm("2") = vaddr;
> + register uint64_t rc asm("3") = 0;
> + int cc = 0;
> +
> + asm volatile(
> + ".insn rre,0xB2560000,%[code],%[addr]\n"
> + "ipm %[cc]\n"
> + "srl %[cc],28\n"
> + : "=d" (rc), [cc] "=d" (cc) /* outputs */
> + : [code] "d" (code), "d" (dummy1), [addr] "a" (addr) /*inputs*/
> + : "memory", "cc" /* clobbers */);
> + return cc;
> +}
> +
> +static int sthyi_ill_reg_odd(void)
> +{
> + register uint64_t code asm("1") = 0;
> + register uint64_t dummy1 asm("2") = 0;
> + register uint64_t addr asm("3") = 0x424242424242000;
> + register uint64_t rc asm("4") = 0;
> + int cc = 0;
> +
> + asm volatile(
> + ".insn rre,0xB2560000,%[code],%[addr]\n"
> + "ipm %[cc]\n"
> + "srl %[cc],28\n"
> + : "=d" (rc), [cc] "=d" (cc) /* outputs */
> + : [code] "d" (code), "d" (dummy1), [addr] "a" (addr) /*inputs*/
> + : "memory", "cc" /* clobbers */);
> + return cc;
> +}
> +
> +static int sthyi_ill_reg_eq(void)
> +{
> + register uint64_t code asm("0") = 0;
> + register uint64_t dummy1 asm("0") = 0;
> + register uint64_t addr asm("0") = 0x424242424242000;
> + register uint64_t rc asm("0") = 0;
I wonder why the compiler does not complain here ... defining multiple
variables which all point to the same register ...
Since you don't really use the variables in the inline assembly below at
all, I think it would be better to simple do this without the register
variables here.
> + int cc = 0;
> +
> + asm volatile(
> + ".insn rre,0xB2560000,0,0\n"
> + "ipm %[cc]\n"
> + "srl %[cc],28\n"
> + : "=d" (rc), [cc] "=d" (cc) /* outputs */
> + : [code] "d" (code), "d" (dummy1), [addr] "a" (addr) /*inputs*/
> + : "memory", "cc" /* clobbers */);
> + return cc;
> +}
> +
> +static int sthyi_ill_fcode(uint64_t *urc)
> +{
> + register uint64_t code asm("0") = 0x0000000000004242;
> + register uint64_t dummy1 asm("1") = 0;
> + register uint64_t addr asm("2") = 0x424242424242000;
> + register uint64_t rc asm("3") = 0;
> + uint64_t cc = 0;
> +
> + asm volatile(
> + ".insn rre,0xB2560000,%[code],%[addr]\n"
> + "ipm %[cc]\n"
> + "srl %[cc],28\n"
> + : "=d" (rc), [cc] "=d" (cc) /* outputs */
> + : [code] "d" (code), "d" (dummy1), [addr] "a" (addr) /*inputs*/
> + : "memory", "cc" /* clobbers */);
> + *urc = rc;
> + return cc;
> +}
> +
> +static void test_exception_addr(void)
> +{
> + expect_pgm_int();
> + sthyi(42042, 0);
> + check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +}
> +
> +static void test_exception_reg_odd(void)
> +{
> + expect_pgm_int();
> + sthyi_ill_reg_odd();
Maybe this should check both registers separately, and not at the same
time, to make sure that the hypervisor checks both registers properly?
Otherwise one of the checks might cover the missing check for the other
register...
> + check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +}
> +
> +static void test_exception_reg_equal(void)
> +{
> + expect_pgm_int();
> + sthyi_ill_reg_eq();
> + check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +}
For the above three tests that only check with check_pgm_int_code(),
could you please add a report_prefix_push(...) at the beginning of the
function and a report_prefix_pop() at the end? I think the output will
look better with that.
> +static void test_function_code(uint64_t addr)
> +{
> + uint64_t urc = 0;
> + int cc = sthyi_ill_fcode(&urc);
> +
> + report("Ill. fcode", cc == 3 && urc == CODE_UNSUPP);
> +}
> +
> +static void test_fcode0_hdr(struct sthyi_hdr_sctn *hdr)
> +{
> + report("HDR length", (hdr->INFHDLN == sizeof(*hdr)
> + && !(hdr->INFHDLN % 8)));
It might be better to check for hdr->INFHDLN >= sizeof(*hdr) in case the
hypervisor supports a longer header in the future? Or can we be sure
that the header will never be extended again?
> + report("MACH sctn length", (hdr->INFMLEN >= sizeof(struct sthyi_mach_sctn)
> + && !(hdr->INFMLEN % 8)));
> + report("PAR sctn length", (hdr->INFPLEN >= sizeof(struct sthyi_par_sctn)
> + && !(hdr->INFPLEN % 8)));
> +
> + report("MACH offset", hdr->INFMOFF >= hdr->INFHDLN);
> + report("PAR offset", hdr->INFPOFF >= hdr->INFHDLN);
> +}
> +
> +static void test_fcode0_mach(struct sthyi_mach_sctn *mach)
> +{
> + int sum = mach->INFMSCPS + mach->INFMDCPS + mach->INFMSIFL + mach->INFMDIFL;
> +
> + if (mach->INFMVAL1 & MACH_ID_VLD) {
> + report("MACH type", memcmp(mach->INFMTYPE, null_buf, sizeof(mach->INFMTYPE)));
> + report("MACH manu", memcmp(mach->INFMMANU, null_buf, sizeof(mach->INFMMANU)));
> + report("MACH seq", memcmp(mach->INFMSEQ, null_buf, sizeof(mach->INFMSEQ)));
> + report("MACH plant", memcmp(mach->INFMPMAN, null_buf, sizeof(mach->INFMPMAN)));
> + }
> +
> + if (mach->INFMVAL1 & MACH_NAME_VLD)
> + report("MACH name", memcmp(mach->INFMNAME, null_buf,
> + sizeof(mach->INFMNAME)));
> +
> + if (mach->INFMVAL1 & MACH_CNT_VLD)
> + report("MACH core counts", sum);
> +}
> +
> +static void test_fcode0_par(struct sthyi_par_sctn *par)
> +{
> + int sum = par->INFPSCPS + par->INFPDCPS + par->INFPSIFL + par->INFPDIFL;
> +
> + if (par->INFPVAL1 & PART_CNT_VLD)
> + report("PAR core counts", sum);
> +
> + if (par->INFPVAL1 & PART_STSI_SUC) {
> + report("PAR number", par->INFPPNUM);
> + report("PAR name", memcmp(par->INFPPNAM, null_buf, sizeof(par->INFPPNAM)));
> + }
> +}
> +
> +static void test_fcode0(void)
> +{
> + struct sthyi_hdr_sctn *hdr;
> + struct sthyi_mach_sctn *mach;
> + struct sthyi_par_sctn *par;
> +
> + /* Zero destination memory. */
> + memset(pagebuf, 0, PAGE_SIZE * 2);
> +
> + sthyi((uint64_t)pagebuf, 0);
> + hdr = (void *)pagebuf;
> + mach = (void *)pagebuf + hdr->INFMOFF;
> + par = (void *)pagebuf + hdr->INFPOFF;
> +
> + test_fcode0_hdr(hdr);
> + test_fcode0_mach(mach);
> + test_fcode0_par(par);
> +}
> +
> +int main(void)
> +{
> + report_prefix_push("sthyi");
> +
> + /* Test for availability */
> + if (!test_facility(74))
> + goto exit;
Could you maybe change this to something like:
has_sthyi = test_facility(74);
report_xfail("STHYI available", !has_sthyi, has_sthyi);
if (!has_sthyi)
goto exit;
... so that the user sees at least something if the facility is not
available.
> + /* Test register/argument checking. */
> + test_exception_addr();
> + test_exception_reg_odd();
> + test_exception_reg_equal();
> + test_function_code((uint64_t) pagebuf);
> +
> + /* Test function code 0 - CP and IFL Capacity Information */
> + test_fcode0();
> +
> +exit:
Maybe use a different label name? ... I'm normally trying to avoid to
use names that are part of the standard libc, e.g. the exit() function
in this case.
> + report_prefix_pop();
> + return report_summary();
> +}
Apart from the nits, this looks pretty good to me already!
Thanks,
Thomas
