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 <[email protected]> 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 <[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(-) >>>> [...] >>>>> 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! -- Thanks, David
