s390x/tcg: LAP support using immediate TLB invalidation

Background: s390x implements Low-Address Protection (LAP). If LAP is enabled, writing to effective addresses (before any transaltion) 0-511 and 4096-4607 triggers a protection exception.

So we have subpage protection on the first two pages of every address space (where the lowcore - the CPU private data resides).

By immediately invalidating the write entry but allowing the caller to continue, we force every write access onto these first two pages into the slow path. we will get a tlb fault with the specific accessed addresses and can then evaluate if protection applies or not.

We have to make sure to ignore the invalid bit if tlb_fill() succeeds.

Quoted 12 lines · David Hildenbrand

Background: s390x implements Low-Address Protection (LAP). If LAP is enabled, writing to effective addresses (before any transaltion) 0-511 and 4096-4607 triggers a protection exception.

So we have subpage protection on the first two pages of every address space (where the lowcore - the CPU private data resides).

By immediately invalidating the write entry but allowing the caller to continue, we force every write access onto these first two pages into the slow path. we will get a tlb fault with the specific accessed addresses and can then evaluate if protection applies or not.

Quoted 1 line

We have to make sure to ignore the invalid bit if tlb_fill() succeeds.

This is similar to a scheme I proposed to PMM wrt handling ARM v8M translation.
Reusing TLB_INVALID_MASK would appear to work, but I wonder if it wouldn't be
clearer to use another bit. I believe I had proposed a TLB_FORCE_SLOW_MASK.

Thoughts, Peter?

r~

Quoted 12 lines · David Hildenbrand

Background: s390x implements Low-Address Protection (LAP). If LAP is enabled, writing to effective addresses (before any transaltion) 0-511 and 4096-4607 triggers a protection exception.

So we have subpage protection on the first two pages of every address space (where the lowcore - the CPU private data resides).

By immediately invalidating the write entry but allowing the caller to continue, we force every write access onto these first two pages into the slow path. we will get a tlb fault with the specific accessed addresses and can then evaluate if protection applies or not.

Quoted 1 line

We have to make sure to ignore the invalid bit if tlb_fill() succeeds.

This is similar to a scheme I proposed to PMM wrt handling ARM v8M translation.
Reusing TLB_INVALID_MASK would appear to work, but I wonder if it wouldn't be
clearer to use another bit. I believe I had proposed a TLB_FORCE_SLOW_MASK.

Thoughts, Peter?

Quoted 19 lines · Richard Henderson

Quoted 7 lines · David Hildenbrand

Background: s390x implements Low-Address Protection (LAP). If LAP is enabled, writing to effective addresses (before any transaltion) 0-511 and 4096-4607 triggers a protection exception.

So we have subpage protection on the first two pages of every address space (where the lowcore - the CPU private data resides).

Quoted 6 lines

By immediately invalidating the write entry but allowing the caller to continue, we force every write access onto these first two pages into the slow path. we will get a tlb fault with the specific accessed addresses and can then evaluate if protection applies or not.

We have to make sure to ignore the invalid bit if tlb_fill() succeeds.

This is similar to a scheme I proposed to PMM wrt handling ARM v8M translation.
Reusing TLB_INVALID_MASK would appear to work, but I wonder if it wouldn't be
clearer to use another bit. I believe I had proposed a TLB_FORCE_SLOW_MASK.

Quoted 1 line

Thoughts, Peter?

As two weeks have passed:

Any further opinions? Richard, how do you want me to continue with this?

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.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/excp_helper.c |  3 +-
 target/s390x/mem_helper.c  |  8 ----
 target/s390x/mmu_helper.c  | 96 +++++++++++++++++++++++++++++-----------------
 3 files changed, 62 insertions(+), 45 deletions(-)

Quoted 22 lines · David Hildenbrand

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.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/excp_helper.c |  3 +-
 target/s390x/mem_helper.c  |  8 ----
 target/s390x/mmu_helper.c  | 96 +++++++++++++++++++++++++++++-----------------
 3 files changed, 62 insertions(+), 45 deletions(-)

[...]

target/s390x/mmu_helper.c static void trigger_page_fault(CPUS390XState *env, target_ulong vaddr, · line 113

Quoted 13 lines

target/s390x/mmu_helper.c +5 -0

index 9daa0fd8e2..44a15449d2 100644
@@ -106,6 +106,37 @@ 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);
+}

I like the check from the kernel sources better:

static inline int is_low_address(unsigned long ga)

{
    /* Check for address ranges 0..511 and 4096..4607 */
    return (ga & ~0x11fful) == 0;
}

... that might result in slightly faster code (depending on the compiler, of course).

Thomas

Quoted 40 lines · Thomas Huth

Quoted 22 lines · David Hildenbrand

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.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/excp_helper.c |  3 +-
 target/s390x/mem_helper.c  |  8 ----
 target/s390x/mmu_helper.c  | 96 +++++++++++++++++++++++++++++-----------------
 3 files changed, 62 insertions(+), 45 deletions(-)  

[...]

target/s390x/mmu_helper.c static void trigger_page_fault(CPUS390XState *env, target_ulong vaddr, · line 113

Quoted 13 lines

target/s390x/mmu_helper.c +5 -0

index 9daa0fd8e2..44a15449d2 100644
@@ -106,6 +106,37 @@ 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);
+}  

I like the check from the kernel sources better:

Quoted 8 lines

static inline int is_low_address(unsigned long ga)

{
    /* Check for address ranges 0..511 and 4096..4607 */
    return (ga & ~0x11fful) == 0;
}

... that might result in slightly faster code (depending on the compiler, of course).

I think that lim (readability) -> 0. Without that comment you're at first sight really clueless what this is about.

My check exactly corresponds to the wording in the PoP (and smart compilers should be able to optimize).

But I don't have a strong opinion on this micro optimization.

Quoted 53 lines · On Thu, 28 Sep 2017 15:08:11 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 40 lines · Thomas Huth

Quoted 22 lines · David Hildenbrand

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.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/excp_helper.c |  3 +-
 target/s390x/mem_helper.c  |  8 ----
 target/s390x/mmu_helper.c  | 96 +++++++++++++++++++++++++++++-----------------
 3 files changed, 62 insertions(+), 45 deletions(-)  

[...]

target/s390x/mmu_helper.c static void trigger_page_fault(CPUS390XState *env, target_ulong vaddr, · line 113

Quoted 13 lines

target/s390x/mmu_helper.c +5 -0

index 9daa0fd8e2..44a15449d2 100644
@@ -106,6 +106,37 @@ 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);
+}  

I like the check from the kernel sources better:

Quoted 8 lines

static inline int is_low_address(unsigned long ga)

{
    /* Check for address ranges 0..511 and 4096..4607 */
    return (ga & ~0x11fful) == 0;
}

... that might result in slightly faster code (depending on the compiler, of course).

I think that lim (readability) -> 0. Without that comment you're at first sight really clueless what this is about.

Quoted 4 lines

My check exactly corresponds to the wording in the PoP (and smart compilers should be able to optimize).

But I don't have a strong opinion on this micro optimization.

FWIW, I'd be happy with both, but has anyone actually looked at the generated code?

Quoted 56 lines · Cornelia Huck

Quoted 53 lines · On Thu, 28 Sep 2017 15:08:11 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 40 lines · Thomas Huth

Quoted 22 lines · David Hildenbrand

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.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/excp_helper.c |  3 +-
 target/s390x/mem_helper.c  |  8 ----
 target/s390x/mmu_helper.c  | 96 +++++++++++++++++++++++++++++-----------------
 3 files changed, 62 insertions(+), 45 deletions(-)  

[...]

target/s390x/mmu_helper.c static void trigger_page_fault(CPUS390XState *env, target_ulong vaddr, · line 113

Quoted 13 lines

target/s390x/mmu_helper.c +5 -0

index 9daa0fd8e2..44a15449d2 100644
@@ -106,6 +106,37 @@ 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);
+}  

I like the check from the kernel sources better:

Quoted 8 lines

static inline int is_low_address(unsigned long ga)

{
    /* Check for address ranges 0..511 and 4096..4607 */
    return (ga & ~0x11fful) == 0;
}

... that might result in slightly faster code (depending on the compiler, of course).

I think that lim (readability) -> 0. Without that comment you're at first sight really clueless what this is about.

Quoted 7 lines

Quoted 4 lines

My check exactly corresponds to the wording in the PoP (and smart compilers should be able to optimize).

But I don't have a strong opinion on this micro optimization.

FWIW, I'd be happy with both, but has anyone actually looked at the generated code?

This is what I get for David's original code:

    80000510:   c4 18 00 00 0d a4       lgrl    %r1,80002058 <x1>
    80000516:   a7 29 01 ff             lghi    %r2,511
    8000051a:   ec 12 00 4f c0 65       clgrjnh %r1,%r2,800005b8 <main+0xd8>
    80000520:   a7 1b f0 00             aghi    %r1,-4096
    80000524:   c2 1e 00 00 01 fe       clgfi   %r1,510
    8000052a:   a7 18 00 00             lhi     %r1,0
    8000052e:   b9 99 00 11             slbr    %r1,%r1
    80000532:   13 11                   lcr     %r1,%r1
    80000534:   c4 1f 00 00 0d 96       strl    %r1,80002060 <b1>

And this is the optimized kernel version:

    8000054a:   c4 18 00 00 0d 7f       lgrl    %r1,80002048 <x2>
    80000550:   a5 17 ee 00             nill    %r1,60928
    80000554:   b9 00 00 11             lpgr    %r1,%r1
    80000558:   a7 1b ff ff             aghi    %r1,-1
    8000055c:   eb 11 00 3f 00 0c       srlg    %r1,%r1,63
    80000562:   c4 1f 00 00 0d 77       strl    %r1,80002050 <b2>

So that's indeed a little bit better :-)
(I was using GCC 4.8.5 from RHEL7, with -O2)

By the way, I think there's a bug in David's code: It should either be "addr <= 4607" or "addr < 4608" instead of "addr < 4607".

With that bug fixed, David's version get's optimized even more:

    80000510:   c4 18 00 00 0d a4       lgrl    %r1,80002058 <x1>
    80000516:   a5 17 ef ff             nill    %r1,61439
    8000051a:   c2 1e 00 00 01 ff       clgfi   %r1,511
    80000520:   a7 18 00 00             lhi     %r1,0
    80000524:   b9 99 00 11             slbr    %r1,%r1
    80000528:   13 11                   lcr     %r1,%r1
    8000052a:   c4 1f 00 00 0d 9b       strl    %r1,80002060 <b1>

... so the difference is really very minimal in that case --> We could really use the more readable version, I think.

Thomas

Using virtual memory access is wrong and will soon include low-address protection checks, which is to be bypassed for STFL.

This was originally part of a bigger STFL(E) refactoring.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/helper.h      | 2 +-
 target/s390x/misc_helper.c | 7 ++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

Quoted 5 lines · David Hildenbrand

Using virtual memory access is wrong and will soon include low-address protection checks, which is to be bypassed for STFL.

This was originally part of a bigger STFL(E) refactoring.

Quoted 5 lines

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/helper.h      | 2 +-
 target/s390x/misc_helper.c | 7 ++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

Need to sort this patch first, so that the series is bisectable.

Quoted 9 lines

DEF_HELPER_FLAGS_2(sfpc, TCG_CALL_NO_RWG, void, env, i64) DEF_HELPER_FLAGS_2(sfas, TCG_CALL_NO_WG, void, env, i64) DEF_HELPER_FLAGS_1(popcnt, TCG_CALL_NO_RWG_SE, i64, i64)

target/s390x/helper.h +0 -1

-DEF_HELPER_FLAGS_1(stfl, TCG_CALL_NO_RWG, void, env)
 DEF_HELPER_2(stfle, i32, env, i64)
 DEF_HELPER_FLAGS_2(lpq, TCG_CALL_NO_WG, i64, env, i64)
 DEF_HELPER_FLAGS_4(stpq, TCG_CALL_NO_WG, void, env, i64, i64, i64)

target/s390x/helper.h DEF_HELPER_FLAGS_3(sturg, TCG_CALL_NO_WG, void, env, i64, i64) · line 156

Quoted 5 lines

target/s390x/helper.h +1 -0

@@ -153,6 +152,7 @@ DEF_HELPER_FLAGS_3(sturg, TCG_CALL_NO_WG, void, env, i64, i64)
 DEF_HELPER_1(per_check_exception, void, env)
 DEF_HELPER_FLAGS_3(per_branch, TCG_CALL_NO_RWG, void, env, i64, i64)
 DEF_HELPER_FLAGS_2(per_ifetch, TCG_CALL_NO_RWG, void, env, i64)
+DEF_HELPER_FLAGS_1(stfl, TCG_CALL_NO_RWG, void, env)
 

Why? Otherwise,

Reviewed-by Richard Henderson <[email protected]>

r~

Quoted 6 lines · Richard Henderson

Quoted 5 lines · David Hildenbrand

Using virtual memory access is wrong and will soon include low-address protection checks, which is to be bypassed for STFL.

This was originally part of a bigger STFL(E) refactoring.

Quoted 7 lines

Quoted 5 lines

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/helper.h      | 2 +-
 target/s390x/misc_helper.c | 7 ++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

Need to sort this patch first, so that the series is bisectable.

Right, this should become #2.

Quoted 16 lines

Quoted 9 lines

DEF_HELPER_FLAGS_2(sfpc, TCG_CALL_NO_RWG, void, env, i64) DEF_HELPER_FLAGS_2(sfas, TCG_CALL_NO_WG, void, env, i64) DEF_HELPER_FLAGS_1(popcnt, TCG_CALL_NO_RWG_SE, i64, i64)

target/s390x/helper.h +0 -1

-DEF_HELPER_FLAGS_1(stfl, TCG_CALL_NO_RWG, void, env)
 DEF_HELPER_2(stfle, i32, env, i64)
 DEF_HELPER_FLAGS_2(lpq, TCG_CALL_NO_WG, i64, env, i64)
 DEF_HELPER_FLAGS_4(stpq, TCG_CALL_NO_WG, void, env, i64, i64, i64)

target/s390x/helper.h DEF_HELPER_FLAGS_3(sturg, TCG_CALL_NO_WG, void, env, i64, i64) · line 156

Quoted 5 lines

target/s390x/helper.h +1 -0

@@ -153,6 +152,7 @@ DEF_HELPER_FLAGS_3(sturg, TCG_CALL_NO_WG, void, env, i64, i64)
 DEF_HELPER_1(per_check_exception, void, env)
 DEF_HELPER_FLAGS_3(per_branch, TCG_CALL_NO_RWG, void, env, i64, i64)
 DEF_HELPER_FLAGS_2(per_ifetch, TCG_CALL_NO_RWG, void, env, i64)
+DEF_HELPER_FLAGS_1(stfl, TCG_CALL_NO_RWG, void, env)
 

Why? Otherwise,

struct LowCore is only available for !CONFIG_USER_ONLY. Therefore I also have to move the helper declaration into !CONFIG_USER_ONLY.

Using virtual memory access is wrong and will soon include low-address protection checks, which is to be bypassed for STFL.

This was originally part of a bigger STFL(E) refactoring.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/helper.h      | 2 +-
 target/s390x/misc_helper.c | 7 ++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

Details about Low-Address Protection can be found in description of patch 1 and 2. It is basically a subpage protection of the first two pages of every address space (for which it is enabled).

We can achieve this by simply directly invalidating the TLB entry and therefore forcing every write accesses onto these two pages into the slow path.

With this patch, I can boot Linux just fine (which uses LAP). This also makes all related kvm-unit-tests that we have pass.

The checks are working that good, that I discovered a STFL bug. STFL stores into the low addresses but low-address protection does explicitly not apply. The Linux kernel calls STFL while LAP is active. So without patch nr 3, booting Linux will fail. (this change is also part of a patch of my SMP series).

Based on: https://github.com/cohuck/qemu.git s390-next Available on: https://github.com/dhildenb/qemu.git s390x_lap

David Hildenbrand (3):
accel/tcg: allow to invalidate a write TLB entry immediately s390x/tcg: low-address protection support s390x/tcg: make STFL store into the lowcore

 accel/tcg/cputlb.c           |  5 ++-
 accel/tcg/softmmu_template.h |  4 +-
 include/exec/cpu-all.h       |  3 ++
 target/s390x/excp_helper.c   |  3 +-
 target/s390x/helper.h        |  2 +-
 target/s390x/mem_helper.c    |  8 ----
 target/s390x/misc_helper.c   |  7 +++-
 target/s390x/mmu_helper.c    | 96 ++++++++++++++++++++++++++++----------------
 8 files changed, 78 insertions(+), 50 deletions(-)

Quoted 8 lines · On Wed, 27 Sep 2017 19:00:24 +0200 David Hildenbrand <[email protected]> wrote:

Details about Low-Address Protection can be found in description of patch 1 and 2. It is basically a subpage protection of the first two pages of every address space (for which it is enabled).

We can achieve this by simply directly invalidating the TLB entry and therefore forcing every write accesses onto these two pages into the slow path.

Quoted 8 lines

With this patch, I can boot Linux just fine (which uses LAP). This also makes all related kvm-unit-tests that we have pass.

The checks are working that good, that I discovered a STFL bug. STFL stores into the low addresses but low-address protection does explicitly not apply. The Linux kernel calls STFL while LAP is active. So without patch nr 3, booting Linux will fail. (this change is also part of a patch of my SMP series).

I fear I have lost track a bit with all those patches floating around. IIUC, patch 3 fixes a real bug that is only exposed by your LAP changes. It used to be part of the stfl changes in v1 of your smp series but is no longer in v2. So, is this a patch that can be applied to current s390-next?

Quoted 20 lines

Based on: https://github.com/cohuck/qemu.git s390-next Available on: https://github.com/dhildenb/qemu.git s390x_lap

David Hildenbrand (3):
accel/tcg: allow to invalidate a write TLB entry immediately s390x/tcg: low-address protection support s390x/tcg: make STFL store into the lowcore

 accel/tcg/cputlb.c           |  5 ++-
 accel/tcg/softmmu_template.h |  4 +-
 include/exec/cpu-all.h       |  3 ++
 target/s390x/excp_helper.c   |  3 +-
 target/s390x/helper.h        |  2 +-
 target/s390x/mem_helper.c    |  8 ----
 target/s390x/misc_helper.c   |  7 +++-
 target/s390x/mmu_helper.c    | 96 ++++++++++++++++++++++++++++----------------
 8 files changed, 78 insertions(+), 50 deletions(-)

Quoted 20 lines · Cornelia Huck

Quoted 8 lines · On Wed, 27 Sep 2017 19:00:24 +0200 David Hildenbrand <[email protected]> wrote:

Details about Low-Address Protection can be found in description of patch 1 and 2. It is basically a subpage protection of the first two pages of every address space (for which it is enabled).

We can achieve this by simply directly invalidating the TLB entry and therefore forcing every write accesses onto these two pages into the slow path.

Quoted 8 lines

With this patch, I can boot Linux just fine (which uses LAP). This also makes all related kvm-unit-tests that we have pass.

The checks are working that good, that I discovered a STFL bug. STFL stores into the low addresses but low-address protection does explicitly not apply. The Linux kernel calls STFL while LAP is active. So without patch nr 3, booting Linux will fail. (this change is also part of a patch of my SMP series).

Quoted 6 lines

I fear I have lost track a bit with all those patches floating around. IIUC, patch 3 fixes a real bug that is only exposed by your LAP changes. It used to be part of the stfl changes in v1 of your smp series but is no longer in v2. So, is this a patch that can be applied to current s390-next?

The SMP series is based on both, this series and the CPU cleanup series you already picked up.

You can apply Patch 3 with the following modified description (requested by Thomas):

s390x/tcg: make STFL store into the lowcore

Using virtual memory access is wrong and will soon include low-address protection checks, which is to be bypassed for STFL.

STFL is a privileged instruction and using LowCore requires !CONFIG_USER_ONLY, so add the ifdef and move the declaration to the right place.

This was originally part of a bigger STFL(E) refactoring.

Reviewed-by Richard Henderson <[email protected]>

Reviewed-by Thomas Huth <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

The other two patches, I will resend once we know how to handle the TLB invalidation.