Re: [Qemu-devel] [PATCH v2 27/32] s390x/tcg: Provide probe_write helper

[David Hildenbrand <david@redhat.com>]

On 01.03.19 18:54, Richard Henderson wrote:
> On 3/1/19 3:54 AM, David Hildenbrand wrote:
>> Instead of checking e.g. the first access on every touched page, we should
>> check the actual access, otherwise we might get false positives when Low
>> Address Protection (LAP) is active. As probe_write() can only deal with
>> accesses to one page, we have to loop.
>>
>> Use i64 for the length, although not needed - easier to reuse
>> TCG temps we already have in the translation functions where this will
>> be used. Also allow it to be used from other helpers.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  target/s390x/helper.h     |  1 +
>>  target/s390x/internal.h   |  2 ++
>>  target/s390x/mem_helper.c | 21 +++++++++++++++++++++
>>  3 files changed, 24 insertions(+)
>>
>> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
>> index 577edb384f..e2710f4fb3 100644
>> --- a/target/s390x/helper.h
>> +++ b/target/s390x/helper.h
>> @@ -123,6 +123,7 @@ DEF_HELPER_4(cu42, i32, env, i32, i32, i32)
>>  DEF_HELPER_5(msa, i32, env, i32, i32, i32, i32)
>>  DEF_HELPER_FLAGS_1(stpt, TCG_CALL_NO_RWG, i64, env)
>>  DEF_HELPER_FLAGS_1(stck, TCG_CALL_NO_RWG_SE, i64, env)
>> +DEF_HELPER_FLAGS_3(probe_write_access, TCG_CALL_NO_WG, void, env, i64, i64)
>>  
>>  /* === Vector Support Instructions === */
>>  DEF_HELPER_FLAGS_4(vll, TCG_CALL_NO_WG, void, env, ptr, i64, i64)
>> diff --git a/target/s390x/internal.h b/target/s390x/internal.h
>> index 7baf0e2404..848d6c36d0 100644
>> --- a/target/s390x/internal.h
>> +++ b/target/s390x/internal.h
>> @@ -386,6 +386,8 @@ void ioinst_handle_sal(S390CPU *cpu, uint64_t reg1, uintptr_t ra);
>>  
>>  /* mem_helper.c */
>>  target_ulong mmu_real2abs(CPUS390XState *env, target_ulong raddr);
>> +void probe_write_access(CPUS390XState *env, uint64_t addr, uint64_t len,
>> +                        uintptr_t ra);
>>  
>>  
>>  /* mmu_helper.c */
>> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
>> index a506d9ef99..efd5256ebf 100644
>> --- a/target/s390x/mem_helper.c
>> +++ b/target/s390x/mem_helper.c
>> @@ -2623,3 +2623,24 @@ uint32_t HELPER(cu42)(CPUS390XState *env, uint32_t r1, uint32_t r2, uint32_t m3)
>>      return convert_unicode(env, r1, r2, m3, GETPC(),
>>                             decode_utf32, encode_utf16);
>>  }
>> +
>> +void probe_write_access(CPUS390XState *env, uint64_t addr, uint64_t len,
>> +                        uintptr_t ra)
>> +{
>> +#ifndef CONFIG_USER_ONLY
>> +    /* test the actual access, not just any access to the page due to LAP */
>> +    while (len) {
>> +        uint64_t curlen = MIN(TARGET_PAGE_SIZE - (addr % TARGET_PAGE_SIZE),
>> +                              len);
> 
> pagelen = -(addr | TARGET_PAGE_MASK);
> curlen = MIN(pagelen, len);
> 
>> +
>> +        probe_write(env, addr, curlen, cpu_mmu_index(env, false), ra);
>> +        addr = wrap_address(env, addr + curlen);
>> +        len -= curlen;
>> +    }
>> +#endif
> 
> That's annoying.  I thought we defined probe_write for user-only as well.
> 
> The user-only version would look something like
> 
>   uint64_t end = addr + len - 1;
> 
>   if (!h2g_valid(addr) ||
>       !h2g_valid(end) ||
>       page_check_range(addr, len, PAGE_WRITE) < 0) {
>     // raise PGM_ADDRESSING
>   }
> 
> Ah, I see why -- there's no user-only equivalent of tlb_fill with which to
> produce the exception in the failure case.
> 

So I guess I can just implement that manually as you suggested. Can
later be factored out. Thanks!


-- 

Thanks,

David / dhildenb