On 16.10.2017 22:23, David Hildenbrand wrote:
> This is a neat way to implement low address protection, whereby
> only the first 512 bytes of the first two pages (each 4096 bytes) of
> every address space are protected.
>
> Store a tec of 0 for the access exception, this is what is defined by
> Enhanced Suppression on Protection in case of a low address protection
> (Bit 61 set to 0, rest undefined).
>
> We have to make sure to to pass the access address, not the masked page
> address into mmu_translate*().
>
> Drop the check from testblock. So we can properly test this via
> kvm-unit-tests.
>
> This will check every access going through one of the MMUs.
>
> Reviewed-by: Richard Henderson <richard.hender...@linaro.org>
> Signed-off-by: David Hildenbrand <da...@redhat.com>
> ---
> target/s390x/excp_helper.c | 3 +-
> target/s390x/mem_helper.c | 8 ----
> target/s390x/mmu_helper.c | 94
> +++++++++++++++++++++++++++++-----------------
> 3 files changed, 60 insertions(+), 45 deletions(-)
[...]
> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
> index 9daa0fd8e2..9806685bee 100644
> --- a/target/s390x/mmu_helper.c
> +++ b/target/s390x/mmu_helper.c
> @@ -106,6 +106,35 @@ static void trigger_page_fault(CPUS390XState *env,
> target_ulong vaddr,
> trigger_access_exception(env, type, ilen, tec);
> }
>
> +/* check whether the address would be proteted by Low-Address Protection */
> +static bool is_low_address(uint64_t addr)
> +{
> + return addr < 512 || (addr >= 4096 && addr <= 4607);
> +}
Just cosmetic, but I'd rather either use "<=" or "<" both times, so:
return addr <= 511 || (addr >= 4096 && addr <= 4607);
or:
return addr < 512 || (addr >= 4096 && addr < 4608);
> +/* check whether Low-Address Protection is enabled for mmu_translate() */
> +static bool lowprot_enabled(const CPUS390XState *env, uint64_t asc)
> +{
> + if (!(env->cregs[0] & CR0_LOWPROT)) {
> + return false;
> + }
> + if (!(env->psw.mask & PSW_MASK_DAT)) {
> + return true;
> + }
> +
> + /* Check the private-space control bit */
> + switch (asc) {
> + case PSW_ASC_PRIMARY:
> + return !(env->cregs[1] & _ASCE_PRIVATE_SPACE);
> + case PSW_ASC_SECONDARY:
> + return !(env->cregs[7] & _ASCE_PRIVATE_SPACE);
> + case PSW_ASC_HOME:
> + return !(env->cregs[13] & _ASCE_PRIVATE_SPACE);
> + default:
> + g_assert_not_reached();
Well, this is certainly reachable - if the guest was running in access
register mode. So it might be nicer to the user if you keep the
error_report() here?
> + }
> +}
> +
> /**
> * Translate real address to absolute (= physical)
> * address by taking care of the prefix mapping.
> @@ -323,6 +352,24 @@ int mmu_translate(CPUS390XState *env, target_ulong
> vaddr, int rw, uint64_t asc,
> }
>
> *flags = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> + if (is_low_address(vaddr & TARGET_PAGE_MASK) && lowprot_enabled(env,
> asc)) {
> + /*
> + * If any part of this page is currently protected, make sure the
> + * TLB entry will not be reused.
> + *
> + * As the protected range is always the first 512 bytes of the
> + * two first pages, we are able to catch all writes to these areas
> + * just by looking at the start address (triggering the tlb miss).
> + */
> + *flags |= PAGE_WRITE_INV;
> + if (is_low_address(vaddr) && rw == MMU_DATA_STORE) {
> + if (exc) {
> + trigger_access_exception(env, PGM_PROTECTION, ILEN_AUTO, 0);
> + }
> + return -EACCES;
> + }
> + }
> +
> vaddr &= TARGET_PAGE_MASK;
>
> if (!(env->psw.mask & PSW_MASK_DAT)) {
> @@ -392,50 +439,17 @@ int mmu_translate(CPUS390XState *env, target_ulong
> vaddr, int rw, uint64_t asc,
> }
>
> /**
> - * lowprot_enabled: Check whether low-address protection is enabled
> - */
> -static bool lowprot_enabled(const CPUS390XState *env)
> -{
> - if (!(env->cregs[0] & CR0_LOWPROT)) {
> - return false;
> - }
> - if (!(env->psw.mask & PSW_MASK_DAT)) {
> - return true;
> - }
> -
> - /* Check the private-space control bit */
> - switch (env->psw.mask & PSW_MASK_ASC) {
> - case PSW_ASC_PRIMARY:
> - return !(env->cregs[1] & _ASCE_PRIVATE_SPACE);
> - case PSW_ASC_SECONDARY:
> - return !(env->cregs[7] & _ASCE_PRIVATE_SPACE);
> - case PSW_ASC_HOME:
> - return !(env->cregs[13] & _ASCE_PRIVATE_SPACE);
> - default:
> - /* We don't support access register mode */
> - error_report("unsupported addressing mode");
> - exit(1);
> - }
> -}
Apart from the nits, the patch looks fine to me.
Thomas
