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