| Message ID | 20170927170027.8539-3-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x/tcg: LAP support using immediate TLB invalidation | expand |
On 09/27/2017 10:00 AM, David Hildenbrand wrote: > + 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); I think g_assert_not_reached() is sufficient for cases like this. Oh, it's just code movement. Nevermind, perhaps. Modulo the outcome of discussion on patch 1, Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 27.09.2017 19:00, 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. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > 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(-) [...] > diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c > index 9daa0fd8e2..44a15449d2 100644 > --- a/target/s390x/mmu_helper.c > +++ b/target/s390x/mmu_helper.c > @@ -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
On 28.09.2017 06:50, Thomas Huth wrote: > On 27.09.2017 19:00, 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. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> 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(-) > [...] >> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c >> index 9daa0fd8e2..44a15449d2 100644 >> --- a/target/s390x/mmu_helper.c >> +++ b/target/s390x/mmu_helper.c >> @@ -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). 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. Thanks > > Thomas >
On 29.09.2017 13:27, Cornelia Huck wrote: > On Thu, 28 Sep 2017 15:08:11 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 28.09.2017 06:50, Thomas Huth wrote: >>> On 27.09.2017 19:00, 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. >>>> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> 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(-) >>> [...] >>>> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c >>>> index 9daa0fd8e2..44a15449d2 100644 >>>> --- a/target/s390x/mmu_helper.c >>>> +++ b/target/s390x/mmu_helper.c >>>> @@ -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). >> >> 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. > > 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
On 12.10.2017 10:41, Thomas Huth wrote: > On 29.09.2017 13:27, Cornelia Huck wrote: >> On Thu, 28 Sep 2017 15:08:11 +0200 >> David Hildenbrand <david@redhat.com> wrote: >> >>> On 28.09.2017 06:50, Thomas Huth wrote: >>>> On 27.09.2017 19:00, 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. >>>>> >>>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>>> --- >>>>> 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(-) >>>> [...] >>>>> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c >>>>> index 9daa0fd8e2..44a15449d2 100644 >>>>> --- a/target/s390x/mmu_helper.c >>>>> +++ b/target/s390x/mmu_helper.c >>>>> @@ -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). >>> >>> 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. >> >> 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 > Very right, I'll fix that. Nice way to find BUGs - comparing generated code :) Thanks!
diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c index 3e4349d00b..aa0cbf67ac 100644 --- a/target/s390x/excp_helper.c +++ b/target/s390x/excp_helper.c @@ -95,7 +95,6 @@ int s390_cpu_handle_mmu_fault(CPUState *cs, vaddr orig_vaddr, DPRINTF("%s: address 0x%" VADDR_PRIx " rw %d mmu_idx %d\n", __func__, orig_vaddr, rw, mmu_idx); - orig_vaddr &= TARGET_PAGE_MASK; vaddr = orig_vaddr; if (mmu_idx < MMU_REAL_IDX) { @@ -127,7 +126,7 @@ int s390_cpu_handle_mmu_fault(CPUState *cs, vaddr orig_vaddr, qemu_log_mask(CPU_LOG_MMU, "%s: set tlb %" PRIx64 " -> %" PRIx64 " (%x)\n", __func__, (uint64_t)vaddr, (uint64_t)raddr, prot); - tlb_set_page(cs, orig_vaddr, raddr, prot, + tlb_set_page(cs, orig_vaddr & TARGET_PAGE_MASK, raddr, prot, mmu_idx, TARGET_PAGE_SIZE); return 0; diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index bbbe1c62b3..69a16867d4 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -1687,18 +1687,10 @@ void HELPER(stctl)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3) uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr) { uintptr_t ra = GETPC(); - CPUState *cs = CPU(s390_env_get_cpu(env)); int i; real_addr = wrap_address(env, real_addr) & TARGET_PAGE_MASK; - /* Check low-address protection */ - if ((env->cregs[0] & CR0_LOWPROT) && real_addr < 0x2000) { - cpu_restore_state(cs, ra); - program_interrupt(env, PGM_PROTECTION, 4); - return 1; - } - for (i = 0; i < TARGET_PAGE_SIZE; i += 8) { cpu_stq_real_ra(env, real_addr + i, 0, ra); } diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c index 9daa0fd8e2..44a15449d2 100644 --- a/target/s390x/mmu_helper.c +++ b/target/s390x/mmu_helper.c @@ -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); +} + +/* 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: + /* We don't support access register mode */ + error_report("unsupported addressing mode"); + exit(1); + } +} + /** * Translate real address to absolute (= physical) * address by taking care of the prefix mapping. @@ -323,6 +354,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 +441,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); - } -} - -/** * translate_pages: Translate a set of consecutive logical page addresses * to absolute addresses */ static int translate_pages(S390CPU *cpu, vaddr addr, int nr_pages, target_ulong *pages, bool is_write) { - bool lowprot = is_write && lowprot_enabled(&cpu->env); uint64_t asc = cpu->env.psw.mask & PSW_MASK_ASC; CPUS390XState *env = &cpu->env; int ret, i, pflags; for (i = 0; i < nr_pages; i++) { - /* Low-address protection? */ - if (lowprot && (addr < 512 || (addr >= 4096 && addr < 4096 + 512))) { - trigger_access_exception(env, PGM_PROTECTION, ILEN_AUTO, 0); - return -EACCES; - } ret = mmu_translate(env, addr, is_write, asc, &pages[i], &pflags, true); if (ret) { return ret; @@ -509,9 +525,19 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf, int mmu_translate_real(CPUS390XState *env, target_ulong raddr, int rw, target_ulong *addr, int *flags) { - /* TODO: low address protection once we flush the tlb on cr changes */ + const bool lowprot_enabled = env->cregs[0] & CR0_LOWPROT; + *flags = PAGE_READ | PAGE_WRITE; - *addr = mmu_real2abs(env, raddr); + if (is_low_address(raddr & TARGET_PAGE_MASK) && lowprot_enabled) { + /* see comment in mmu_translate() how this works */ + *flags |= PAGE_WRITE_INV; + if (is_low_address(raddr) && rw == MMU_DATA_STORE) { + trigger_access_exception(env, PGM_PROTECTION, ILEN_AUTO, 0); + return -EACCES; + } + } + + *addr = mmu_real2abs(env, raddr & TARGET_PAGE_MASK); /* TODO: storage key handling */ return 0;
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 <david@redhat.com> --- 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(-)