On 08.06.2018 16:01, Janosch Frank wrote:
> From: Janosch Frank <frankja@xxxxxxxxxxxxxxxxxx>
>
> Tests no-execute (Instruction Execution Protection) DAT protection.
>
> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxxxxxxx>
> ---
> lib/s390x/interrupt.c | 4 ++++
> lib/s390x/mmu.c | 46 ++++++++++++++++++++++++++++++++++--
> lib/s390x/mmu.h | 20 ++++++++++++++++
> s390x/Makefile | 1 +
> s390x/iep.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++
> s390x/unittests.cfg | 4 ++++
> 6 files changed, 138 insertions(+), 2 deletions(-)
> create mode 100644 lib/s390x/mmu.h
> create mode 100644 s390x/iep.c
>
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 56c7603..2e2610e 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -51,6 +51,10 @@ static void fixup_pgm_int(void)
> */
> lc->pgm_old_psw.mask &= ~PSW_MASK_PSTATE;
> break;
> + case PGM_INT_CODE_PROTECTION:
> + if (lc->trans_exc_id & 0x80UL && lc->trans_exc_id & 0x04UL)
> + lc->pgm_old_psw.addr = lc->sw_int_grs[14];
I think you should better also check bit 60 here to make sure that it is
0. Please also add a comment that this r14 handling is special here for
the iep.c test (otherwise this could be very confusing for someone who
did not look at iep.c first).
> + break;
> case PGM_INT_CODE_SEGMENT_TRANSLATION:
> case PGM_INT_CODE_PAGE_TRANSLATION:
> case PGM_INT_CODE_TRACE_TABLE:
[...]
> diff --git a/s390x/iep.c b/s390x/iep.c
> new file mode 100644
> index 0000000..1f07741
> --- /dev/null
> +++ b/s390x/iep.c
> @@ -0,0 +1,65 @@
> +/*
> + * Instruction Execution Prevention (IEP) DAT test.
> + *
> + * Copyright (c) 2018 IBM Corp
> + *
> + * Authors:
> + * Janosch Frank <frankja@xxxxxxxxxx>
> + *
> + * 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 <vmalloc.h>
> +#include <asm/facility.h>
> +#include <asm/interrupt.h>
> +#include <mmu.h>
> +#include <asm/pgtable.h>
> +#include <asm-generic/barrier.h>
> +
> +static void test_iep(void)
> +{
> + uint16_t *code;
> + uint8_t *iepbuf = 0;
Nit: I'd prefer NULL instead of 0 here.
> + void (*fn)(void);
> +
> + /* Enable IEP */
> + ctl_set_bit(0, 20);
> +
> + /* Get and protect a page with the IEP bit */
> + iepbuf = alloc_page();
> + protect_page(iepbuf, PAGE_ENTRY_IEP);
> + mb();
> +
> + /* Code branches into r14 which contains the return address. */
> + code = (uint16_t *)iepbuf;
> + *code = 0x07fe;
> + asm volatile("" : : "m" (code));
What's this asm-volatile statement here good for? Mybe add a comment to
the code?
> + fn = (void *)code;
> + mb();
(having done a lot of ppc stuff in the past, let me ask just to be sure:
We don't have to flush the data cache on s390x before doing stuff like
this, do we?)
> + expect_pgm_int();
> + /* Jump into protected page */
> + fn();
> + check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> + unprotect_page(iepbuf, PAGE_ENTRY_IEP);
> + ctl_clear_bit(0, 20);
> +}
> +
> +int main(void)
> +{
> + bool has_iep = test_facility(130);
> +
> + report_prefix_push("iep");
> + report_xfail("DAT IEP available", !has_iep, has_iep);
> + if (!has_iep)
> + goto done;
> +
> + /* Setup DAT 1:1 mapping and memory management */
> + setup_vm();
> + test_iep();
> +
> +done:
> + report_prefix_pop();
> + return report_summary();
> +}
Apart from the nits, the patch looks quite good to me already.
Thomas
