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

[Thomas Huth <thuth@redhat.com>]

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