On Tue, 19 Nov 2024 16:48:37 +0100
"Christoph Schlameuss" <schlameuss@xxxxxxxxxxxxx> wrote:
> Sorry for not seeing this on the first review, but there still is something.
>
> On Mon Nov 11, 2024 at 1:15 PM CET, Claudio Imbrenda wrote:
> > Add a new test to check that the host can use 1M large pages to back
> > protected guests when the corresponding feature is present.
> >
> > Signed-off-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx>
> > ---
> > s390x/Makefile | 2 +
> > lib/s390x/asm/arch_def.h | 1 +
> > lib/s390x/asm/uv.h | 18 ++
> > s390x/pv-edat1.c | 463 +++++++++++++++++++++++++++++++++++
> > s390x/snippets/c/pv-memhog.c | 59 +++++
> > 5 files changed, 543 insertions(+)
> > create mode 100644 s390x/pv-edat1.c
> > create mode 100644 s390x/snippets/c/pv-memhog.c
>
> [...]
>
> > +static void timings(void)
> > +{
> > + const uint64_t seqs[] = { 0, 1, 3, 7, 17, 27, 63, 127};
>
> Missing space in the end
will fix
> [...]
>
> > +static void test_one_export(struct vm *vm, int pre1m, bool exportfirst, int page, bool post1m)
> > +{
> > + uint64_t addr = SZ_1M + page * PAGE_SIZE;
> > + int expected = FIRST;
> > +
> > + report_prefix_pushf("page %d", page);
> > +
> > + map_identity_all(vm, pre1m == 1);
> > + init_snippet(vm);
> > + import_all(vm);
> > + merge_all(vm);
> > +
> > + if (pre1m != -1) {
> > + sie(vm);
> > + assert_diag500_val(vm, expected);
> > + vm->save_area.guest.grs[2] = PARAM(4, 256 + 42);
> > + expected = SECOND;
> > + }
> > +
> > + if (exportfirst) {
> > + export_range(vm, addr, addr + PAGE_SIZE);
> > + if (pre1m != 1)
> > + map_identity_all(vm, true);
> > + } else {
> > + if (pre1m != 1)
> > + map_identity_all(vm, true);
> > + export_range(vm, addr, addr + PAGE_SIZE);
> > + }
>
> The tree checks here "pre1m == 1" and "pre1m != 1" do not quite add up. pre1m
> is only ever called with pre1m = -1 and pre1m = 0.
> With this the first of the three map_identity_all calls here will always map
> normal pages and the next two calls will never happen.
yeah, because the loop used to go to 1, and I don't remember why I
changed it... maybe it should actually go all the way to 1 again...
>
> > + expect_pgm_int();
> > + sie(vm);
> > + assert(pgm_3c_addr_is(vm, MAGIC_ADDRESS));
> > +
> > + import_range(vm, addr, addr + PAGE_SIZE);
> > + if (post1m) {
> > + merge_range(vm, SZ_1M, 2 * SZ_1M);
> > + } else {
> > + map_identity_all(vm, false);
> > + }
> > + sie(vm);
> > + assert_diag500_val(vm, expected);
> > + report_pass("Successful");
> > +
> > + uv_destroy_guest(vm);
> > + report_prefix_pop();
> > +}
> > +
> > +static void test_export(void)
> > +{
> > + int pre1m, post1m, exportfirst;
> > +
> > + report_prefix_push("export");
> > +
> > + for (pre1m = -1; pre1m < 1; pre1m++) {
> > + for (post1m = 0; post1m < 2; post1m++) {
> > + for (exportfirst = 0; exportfirst < 2; exportfirst++) {
> > + report_prefix_pushf("%s pre-run, %s post-run, export %s remap",
> > + STR3(pre1m), STR(post1m), exportfirst ? "before" : "after");
> > +
> > + test_one_export(&vm, pre1m, exportfirst, 0, post1m);
> > + test_one_export(&vm, pre1m, exportfirst, 1, post1m);
> > + test_one_export(&vm, pre1m, exportfirst, 42, post1m);
> > + test_one_export(&vm, pre1m, exportfirst, 128, post1m);
> > + test_one_export(&vm, pre1m, exportfirst, 254, post1m);
> > + test_one_export(&vm, pre1m, exportfirst, 255, post1m);
> > +
> > + report_prefix_pop();
> > + }
> > + }
> > + }
> > +
> > + report_prefix_pop();
> > +}
> > +
> > +static bool check_facilities(void)
> > +{
> > + if (!uv_host_requirement_checks())
> > + return false;
> > + if (!test_facility(8) || !test_facility(78)) {
> > + report_skip("EDAT1 and EDAT2 not available in the host.");
>
> s/and/or/
will fix differently
>
> > + return false;
> > + }
> > + if (!uv_query_test_call(BIT_UVC_CMD_VERIFY_LARGE_FRAME)) {
> > + report_skip("Verify Large Frame UVC not supported.");
> > + return false;
> > + }
> > + if (!uv_query_test_feature(BIT_UV_1M_BACKING)) {
> > + report_skip("Large frames not supported for Secure Execution.");
> > + return false;
> > + }
> > + return true;
> > +}
>
> [...]
>
