Message ID | 20240620141700.4124157-8-nsg@linux.ibm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | s390x: STFLE nested interpretation | expand |
On Thu, 20 Jun 2024 16:17:00 +0200 Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote: > The STFLE instruction indicates installed facilities. > SIE can interpretively execute STFLE. > Use a snippet guest executing STFLE to get the result of > interpretive execution and check the result. > > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> [...] > +struct guest_stfle_res { > + uint16_t len; > + uint64_t reg; you don't really use reg, do you? > + unsigned char *mem; > +}; > + > +static struct guest_stfle_res run_guest(void) > +{ > + struct guest_stfle_res res; > + uint64_t guest_stfle_addr; uint64_t tmp; > + > + sie(&vm); > + assert(snippet_is_force_exit_value(&vm)); > + guest_stfle_addr = snippet_get_force_exit_value(&vm); > + res.mem = &vm.guest_mem[guest_stfle_addr]; > + memcpy(&res.reg, res.mem, sizeof(res.reg)); memcpy(&tmp, res.mem, etc); > + res.len = (res.reg & 0xff) + 1; (tmp & 0xff) + 1 etc > + res.mem += sizeof(res.reg); (you could just do res.mem++ if you had declared it as uint64_t * instead of unsigned char *) > + return res; > +} > + [...] > +int main(int argc, char **argv) > +{ > + struct args args = parse_args(argc, argv); > + > + if (!sclp_facilities.has_sief2) { > + report_skip("SIEF2 facility unavailable"); > + goto out; > + } > + > + report_info("PRNG seed: 0x%lx", args.seed); > + prng_s = prng_init(args.seed); > + setup_guest(); > + if (test_facility(7)) > + test_stfle_format_0(); since STFLE is literally the feature you are testing, maybe you can just skip, like you did for SIEF2? > +out: > + return report_summary(); > +} > diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg > index 3a9decc9..f2203069 100644 > --- a/s390x/unittests.cfg > +++ b/s390x/unittests.cfg > @@ -392,3 +392,6 @@ file = sie-dat.elf > > [pv-attest] > file = pv-attest.elf > + > +[stfle-sie] > +file = stfle-sie.elf
On Thu, 2024-06-20 at 19:25 +0200, Claudio Imbrenda wrote: > On Thu, 20 Jun 2024 16:17:00 +0200 > Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote: > > > The STFLE instruction indicates installed facilities. > > SIE can interpretively execute STFLE. > > Use a snippet guest executing STFLE to get the result of > > interpretive execution and check the result. > > > > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > > > [...] > > > > +struct guest_stfle_res { > > + uint16_t len; > > + uint64_t reg; > > you don't really use reg, do you? No, and I don't think I will either, must be some vestige. [...] > > > +int main(int argc, char **argv) > > +{ > > + struct args args = parse_args(argc, argv); > > + > > + if (!sclp_facilities.has_sief2) { > > + report_skip("SIEF2 facility unavailable"); > > + goto out; > > + } > > + > > + report_info("PRNG seed: 0x%lx", args.seed); > > + prng_s = prng_init(args.seed); > > + setup_guest(); > > + if (test_facility(7)) > > + test_stfle_format_0(); > > since STFLE is literally the feature you are testing, maybe you can > just skip, like you did for SIEF2? Yeah, that's better. > > > +out: > > + return report_summary(); > > +} > > diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg > > index 3a9decc9..f2203069 100644 > > --- a/s390x/unittests.cfg > > +++ b/s390x/unittests.cfg > > @@ -392,3 +392,6 @@ file = sie-dat.elf > > > > [pv-attest] > > file = pv-attest.elf > > + > > +[stfle-sie] > > +file = stfle-sie.elf >
On Fri Jun 21, 2024 at 12:17 AM AEST, Nina Schoetterl-Glausch wrote: > The STFLE instruction indicates installed facilities. > SIE can interpretively execute STFLE. > Use a snippet guest executing STFLE to get the result of > interpretive execution and check the result. > > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> This one is beyond me... one minor thing, you could move the prng patch to just before this one. > +++ b/s390x/snippets/c/stfle.c > @@ -0,0 +1,26 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright IBM Corp. 2023 > +++ b/s390x/stfle-sie.c > @@ -0,0 +1,134 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright IBM Corp. 2023 Time to flip your calendar page? :) Thanks, Nick
Quoting Nina Schoetterl-Glausch (2024-06-20 16:17:00) [...] > diff --git a/lib/s390x/asm/facility.h b/lib/s390x/asm/facility.h > index a66fe56a..2bad05c5 100644 > --- a/lib/s390x/asm/facility.h > +++ b/lib/s390x/asm/facility.h > @@ -27,12 +27,20 @@ static inline void stfl(void) > asm volatile(" stfl 0(0)\n" : : : "memory"); > } > > -static inline void stfle(uint64_t *fac, unsigned int nb_doublewords) > +static inline unsigned int stfle(uint64_t *fac, unsigned int nb_doublewords) Why unsigned int? [...] > diff --git a/s390x/snippets/c/stfle.c b/s390x/snippets/c/stfle.c > new file mode 100644 > index 00000000..eb024a6a > --- /dev/null > +++ b/s390x/snippets/c/stfle.c [...] > +int main(void) > +{ > + const unsigned int max_fac_len = 8; > + uint64_t res[max_fac_len + 1]; > + > + res[0] = max_fac_len - 1; > + asm volatile ( "lg 0,%[len]\n" > + " stfle %[fac]\n" > + " stg 0,%[len]\n" > + : [fac] "=QS"(*(uint64_t(*)[max_fac_len])&res[1]), Out of curiosity: Q = Memory reference without index register and with short displacement S = Memory reference without index register but with long displacement Which one is it? And: is long displacement even appropriate here? The cast also is hard to understand. Since this is not super high performance code, do we just want to clobber memory so this gets a bit easier to understand? > + [len] "+RT"(res[0]) Same question about RT as above. [...] > diff --git a/s390x/stfle-sie.c b/s390x/stfle-sie.c > new file mode 100644 > index 00000000..a3e7f1c9 > --- /dev/null > +++ b/s390x/stfle-sie.c [...] > +static struct guest_stfle_res run_guest(void) > +{ > + struct guest_stfle_res res; > + uint64_t guest_stfle_addr; > + > + sie(&vm); > + assert(snippet_is_force_exit_value(&vm)); > + guest_stfle_addr = snippet_get_force_exit_value(&vm); > + res.mem = &vm.guest_mem[guest_stfle_addr]; > + memcpy(&res.reg, res.mem, sizeof(res.reg)); > + res.len = (res.reg & 0xff) + 1; If I'm not mistaken, you subtracted 1 in the guest. Here you add it again. Is there a particular reason why?
On Tue, 2024-08-27 at 16:08 +0200, Nico Boehr wrote: > Quoting Nina Schoetterl-Glausch (2024-06-20 16:17:00) > [...] > > diff --git a/lib/s390x/asm/facility.h b/lib/s390x/asm/facility.h > > index a66fe56a..2bad05c5 100644 > > --- a/lib/s390x/asm/facility.h > > +++ b/lib/s390x/asm/facility.h > > @@ -27,12 +27,20 @@ static inline void stfl(void) > > asm volatile(" stfl 0(0)\n" : : : "memory"); > > } > > > > -static inline void stfle(uint64_t *fac, unsigned int nb_doublewords) > > +static inline unsigned int stfle(uint64_t *fac, unsigned int nb_doublewords) > > Why unsigned int? The return value is 1-256, the size of the type is a bit arbitrary I suppose. > > [...] > > diff --git a/s390x/snippets/c/stfle.c b/s390x/snippets/c/stfle.c > > new file mode 100644 > > index 00000000..eb024a6a > > --- /dev/null > > +++ b/s390x/snippets/c/stfle.c > [...] > > +int main(void) > > +{ > > + const unsigned int max_fac_len = 8; > > + uint64_t res[max_fac_len + 1]; > > + > > + res[0] = max_fac_len - 1; > > + asm volatile ( "lg 0,%[len]\n" > > + " stfle %[fac]\n" > > + " stg 0,%[len]\n" > > + : [fac] "=QS"(*(uint64_t(*)[max_fac_len])&res[1]), > > Out of curiosity: > > Q = Memory reference without index register and with short displacement > S = Memory reference without index register but with long displacement > > Which one is it? Ups, just short displacement actually. > > And: is long displacement even appropriate here? > > The cast also is hard to understand. Since this is not super high > performance code, do we just want to clobber memory so this gets a bit > easier to understand? > > > + [len] "+RT"(res[0]) > > Same question about RT as above. Long, but providing a short displacement should be fine too. Not sure if there is any benefit to letting the compiler choose. > > [...] > > diff --git a/s390x/stfle-sie.c b/s390x/stfle-sie.c > > new file mode 100644 > > index 00000000..a3e7f1c9 > > --- /dev/null > > +++ b/s390x/stfle-sie.c > [...] > > +static struct guest_stfle_res run_guest(void) > > +{ > > + struct guest_stfle_res res; > > + uint64_t guest_stfle_addr; > > + > > + sie(&vm); > > + assert(snippet_is_force_exit_value(&vm)); > > + guest_stfle_addr = snippet_get_force_exit_value(&vm); > > + res.mem = &vm.guest_mem[guest_stfle_addr]; > > + memcpy(&res.reg, res.mem, sizeof(res.reg)); > > + res.len = (res.reg & 0xff) + 1; > > If I'm not mistaken, you subtracted 1 in the guest. Here you add it again. > Is there a particular reason why? No, it's the direct result of STFLE on register 0.
On Mon, Sep 02, 2024 at 04:24:53PM +0200, Nina Schoetterl-Glausch wrote: > On Tue, 2024-08-27 at 16:08 +0200, Nico Boehr wrote: > > Quoting Nina Schoetterl-Glausch (2024-06-20 16:17:00) > > > > And: is long displacement even appropriate here? > > > > The cast also is hard to understand. Since this is not super high > > performance code, do we just want to clobber memory so this gets a bit > > easier to understand? > > > > > + [len] "+RT"(res[0]) > > > > Same question about RT as above. > > Long, but providing a short displacement should be fine too. > Not sure if there is any benefit to letting the compiler choose. There are some older gcc compilers around which fail to compile if you specify T for long displacement, but the compiler sees that a short displacement would work. So please specify RT to avoid such compile bugs. See https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=3e4be43f69da
Quoting Nina Schoetterl-Glausch (2024-09-02 16:24:53) > On Tue, 2024-08-27 at 16:08 +0200, Nico Boehr wrote: > > Quoting Nina Schoetterl-Glausch (2024-06-20 16:17:00) > > [...] > > > diff --git a/lib/s390x/asm/facility.h b/lib/s390x/asm/facility.h > > > index a66fe56a..2bad05c5 100644 > > > --- a/lib/s390x/asm/facility.h > > > +++ b/lib/s390x/asm/facility.h > > > @@ -27,12 +27,20 @@ static inline void stfl(void) > > > asm volatile(" stfl 0(0)\n" : : : "memory"); > > > } > > > > > > -static inline void stfle(uint64_t *fac, unsigned int nb_doublewords) > > > +static inline unsigned int stfle(uint64_t *fac, unsigned int nb_doublewords) > > > > Why unsigned int? > > The return value is 1-256, the size of the type is a bit arbitrary I suppose. > > > > > [...] > > > diff --git a/s390x/snippets/c/stfle.c b/s390x/snippets/c/stfle.c > > > new file mode 100644 > > > index 00000000..eb024a6a > > > --- /dev/null > > > +++ b/s390x/snippets/c/stfle.c > > [...] > > > +int main(void) > > > +{ > > > + const unsigned int max_fac_len = 8; > > > + uint64_t res[max_fac_len + 1]; > > > + > > > + res[0] = max_fac_len - 1; > > > + asm volatile ( "lg 0,%[len]\n" > > > + " stfle %[fac]\n" > > > + " stg 0,%[len]\n" > > > + : [fac] "=QS"(*(uint64_t(*)[max_fac_len])&res[1]), Nina, do you mind sending a new version where we have the constraints simplified, e.g. with just a memory clobber?
On Thu, 2024-10-10 at 10:27 +0200, Nico Boehr wrote: > Quoting Nina Schoetterl-Glausch (2024-09-02 16:24:53) > > On Tue, 2024-08-27 at 16:08 +0200, Nico Boehr wrote: > > > Quoting Nina Schoetterl-Glausch (2024-06-20 16:17:00) > > > [...] > > > > diff --git a/lib/s390x/asm/facility.h b/lib/s390x/asm/facility.h > > > > index a66fe56a..2bad05c5 100644 > > > > --- a/lib/s390x/asm/facility.h > > > > +++ b/lib/s390x/asm/facility.h > > > > @@ -27,12 +27,20 @@ static inline void stfl(void) > > > > asm volatile(" stfl 0(0)\n" : : : "memory"); > > > > } > > > > > > > > -static inline void stfle(uint64_t *fac, unsigned int nb_doublewords) > > > > +static inline unsigned int stfle(uint64_t *fac, unsigned int nb_doublewords) > > > > > > Why unsigned int? > > > > The return value is 1-256, the size of the type is a bit arbitrary I suppose. > > > > > > > > [...] > > > > diff --git a/s390x/snippets/c/stfle.c b/s390x/snippets/c/stfle.c > > > > new file mode 100644 > > > > index 00000000..eb024a6a > > > > --- /dev/null > > > > +++ b/s390x/snippets/c/stfle.c > > > [...] > > > > +int main(void) > > > > +{ > > > > + const unsigned int max_fac_len = 8; > > > > + uint64_t res[max_fac_len + 1]; > > > > + > > > > + res[0] = max_fac_len - 1; > > > > + asm volatile ( "lg 0,%[len]\n" > > > > + " stfle %[fac]\n" > > > > + " stg 0,%[len]\n" > > > > + : [fac] "=QS"(*(uint64_t(*)[max_fac_len])&res[1]), > > Nina, do you mind sending a new version where we have the constraints > simplified, e.g. with just a memory clobber? Will do
diff --git a/s390x/Makefile b/s390x/Makefile index 12445fb5..7c38d66a 100644 --- a/s390x/Makefile +++ b/s390x/Makefile @@ -44,6 +44,7 @@ tests += $(TEST_DIR)/exittime.elf tests += $(TEST_DIR)/ex.elf tests += $(TEST_DIR)/topology.elf tests += $(TEST_DIR)/sie-dat.elf +tests += $(TEST_DIR)/stfle-sie.elf pv-tests += $(TEST_DIR)/pv-diags.elf pv-tests += $(TEST_DIR)/pv-icptcode.elf @@ -129,6 +130,7 @@ snippet_lib = $(snippet_asmlib) lib/auxinfo.o $(TEST_DIR)/mvpg-sie.elf: snippets = $(SNIPPET_DIR)/c/mvpg-snippet.gbin $(TEST_DIR)/sie-dat.elf: snippets = $(SNIPPET_DIR)/c/sie-dat.gbin $(TEST_DIR)/spec_ex-sie.elf: snippets = $(SNIPPET_DIR)/c/spec_ex.gbin +$(TEST_DIR)/stfle-sie.elf: snippets = $(SNIPPET_DIR)/c/stfle.gbin $(TEST_DIR)/pv-diags.elf: pv-snippets += $(SNIPPET_DIR)/asm/pv-diag-yield.gbin $(TEST_DIR)/pv-diags.elf: pv-snippets += $(SNIPPET_DIR)/asm/pv-diag-288.gbin diff --git a/lib/s390x/asm/facility.h b/lib/s390x/asm/facility.h index a66fe56a..2bad05c5 100644 --- a/lib/s390x/asm/facility.h +++ b/lib/s390x/asm/facility.h @@ -27,12 +27,20 @@ static inline void stfl(void) asm volatile(" stfl 0(0)\n" : : : "memory"); } -static inline void stfle(uint64_t *fac, unsigned int nb_doublewords) +static inline unsigned int stfle(uint64_t *fac, unsigned int nb_doublewords) { register unsigned long r0 asm("0") = nb_doublewords - 1; asm volatile(" .insn s,0xb2b00000,0(%1)\n" : "+d" (r0) : "a" (fac) : "memory", "cc"); + return r0 + 1; +} + +static inline unsigned long stfle_size(void) +{ + uint64_t dummy; + + return stfle(&dummy, 1); } static inline void setup_facilities(void) diff --git a/s390x/snippets/c/stfle.c b/s390x/snippets/c/stfle.c new file mode 100644 index 00000000..eb024a6a --- /dev/null +++ b/s390x/snippets/c/stfle.c @@ -0,0 +1,26 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright IBM Corp. 2023 + * + * Snippet used by the STLFE interpretive execution facilities test. + */ +#include <libcflat.h> +#include <snippet-guest.h> + +int main(void) +{ + const unsigned int max_fac_len = 8; + uint64_t res[max_fac_len + 1]; + + res[0] = max_fac_len - 1; + asm volatile ( "lg 0,%[len]\n" + " stfle %[fac]\n" + " stg 0,%[len]\n" + : [fac] "=QS"(*(uint64_t(*)[max_fac_len])&res[1]), + [len] "+RT"(res[0]) + : + : "%r0", "cc" + ); + force_exit_value((uint64_t)&res); + return 0; +} diff --git a/s390x/stfle-sie.c b/s390x/stfle-sie.c new file mode 100644 index 00000000..a3e7f1c9 --- /dev/null +++ b/s390x/stfle-sie.c @@ -0,0 +1,134 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright IBM Corp. 2023 + * + * SIE with STLFE interpretive execution facilities test. + */ +#include <libcflat.h> +#include <stdlib.h> +#include <asm/facility.h> +#include <asm/time.h> +#include <snippet-host.h> +#include <alloc_page.h> +#include <sclp.h> +#include <rand.h> + +static struct vm vm; +static uint64_t (*fac)[PAGE_SIZE / sizeof(uint64_t)]; +static prng_state prng_s; + +static void setup_guest(void) +{ + extern const char SNIPPET_NAME_START(c, stfle)[]; + extern const char SNIPPET_NAME_END(c, stfle)[]; + + setup_vm(); + fac = alloc_pages_flags(0, AREA_DMA31); + + snippet_setup_guest(&vm, false); + snippet_init(&vm, SNIPPET_NAME_START(c, stfle), + SNIPPET_LEN(c, stfle), SNIPPET_UNPACK_OFF); +} + +struct guest_stfle_res { + uint16_t len; + uint64_t reg; + unsigned char *mem; +}; + +static struct guest_stfle_res run_guest(void) +{ + struct guest_stfle_res res; + uint64_t guest_stfle_addr; + + sie(&vm); + assert(snippet_is_force_exit_value(&vm)); + guest_stfle_addr = snippet_get_force_exit_value(&vm); + res.mem = &vm.guest_mem[guest_stfle_addr]; + memcpy(&res.reg, res.mem, sizeof(res.reg)); + res.len = (res.reg & 0xff) + 1; + res.mem += sizeof(res.reg); + return res; +} + +static void test_stfle_format_0(void) +{ + struct guest_stfle_res res; + + report_prefix_push("format-0"); + for (int j = 0; j < stfle_size(); j++) + WRITE_ONCE((*fac)[j], prng64(&prng_s)); + vm.sblk->fac = (uint32_t)(uint64_t)fac; + res = run_guest(); + report(res.len == stfle_size(), "stfle len correct"); + report(!memcmp(*fac, res.mem, res.len * sizeof(uint64_t)), + "Guest facility list as specified"); + report_prefix_pop(); +} + +struct args { + uint64_t seed; +}; + +static bool parse_uint64_t(const char *arg, uint64_t *out) +{ + char *end; + uint64_t num; + + if (arg[0] == '\0') + return false; + num = strtoul(arg, &end, 0); + if (end[0] != '\0') + return false; + *out = num; + return true; +} + +static struct args parse_args(int argc, char **argv) +{ + struct args args; + const char *flag; + unsigned int i; + uint64_t arg; + bool has_arg; + + stck(&args.seed); + + for (i = 1; i < argc; i++) { + if (i + 1 < argc) + has_arg = parse_uint64_t(argv[i + 1], &arg); + else + has_arg = false; + + flag = "--seed"; + if (!strcmp(flag, argv[i])) { + if (!has_arg) + report_abort("%s needs an uint64_t parameter", flag); + args.seed = arg; + ++i; + continue; + } + report_abort("Unsupported parameter '%s'", + argv[i]); + } + + return args; +} + +int main(int argc, char **argv) +{ + struct args args = parse_args(argc, argv); + + if (!sclp_facilities.has_sief2) { + report_skip("SIEF2 facility unavailable"); + goto out; + } + + report_info("PRNG seed: 0x%lx", args.seed); + prng_s = prng_init(args.seed); + setup_guest(); + if (test_facility(7)) + test_stfle_format_0(); +out: + return report_summary(); +} diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg index 3a9decc9..f2203069 100644 --- a/s390x/unittests.cfg +++ b/s390x/unittests.cfg @@ -392,3 +392,6 @@ file = sie-dat.elf [pv-attest] file = pv-attest.elf + +[stfle-sie] +file = stfle-sie.elf
The STFLE instruction indicates installed facilities. SIE can interpretively execute STFLE. Use a snippet guest executing STFLE to get the result of interpretive execution and check the result. Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> --- s390x/Makefile | 2 + lib/s390x/asm/facility.h | 10 ++- s390x/snippets/c/stfle.c | 26 ++++++++ s390x/stfle-sie.c | 134 +++++++++++++++++++++++++++++++++++++++ s390x/unittests.cfg | 3 + 5 files changed, 174 insertions(+), 1 deletion(-) create mode 100644 s390x/snippets/c/stfle.c create mode 100644 s390x/stfle-sie.c