Re: [qemu-s390x] [PATCH RFC 2/3] s390x/tcg: low-address protection support

[David Hildenbrand]

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 <address@hidden> 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 <address@hidden>
>>>>> ---
>>>>>  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