On 13.03.2018 17:07, Thomas Huth wrote:
> 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?
Done
>
>> + * 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?
Copy-paste...
>
>> +static char null_buf[32] = {};
>> +
[...]
>> +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.
I did this series to improve my inline assembly knowledge, so I'm happy
about suggestions and you already did find the assembly problems in the
other patches.
Done
>
>> + 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...
Sounds good, will add
>
>> + 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.
Sure
>
>> +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?
This test was originally a test I used to check my sthyi implementation,
so it was pretty tailored to the KVM code. So, I should make it >= to
keep it more general.
>
>> + 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);
>> +}
>> +
[...]
>> +
>> +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.
Sure
>
>> + /* 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.
Ok, will change for all patches in the series, how about:
test_report:
>
>> + report_prefix_pop();
>> + return report_summary();
>> +}
>
> Apart from the nits, this looks pretty good to me already!>
> Thanks,
> Thomas
>
Attachment:
signature.asc
Description: OpenPGP digital signature
