Re: [PATCH v3 14/18] target/s390x: Rely on unwinding in s390_cpu_tlb_fill

[David Hildenbrand]

On 27.09.19 18:16, Richard Henderson wrote:
> On 9/27/19 4:02 AM, David Hildenbrand wrote:
>> On 26.09.19 18:26, Richard Henderson wrote:
>>> We currently set ilen to AUTO, then overwrite that during
>>> unwinding, then overwrite that for the code access case.
>>>
>>> This can be simplified to setting ilen to our arbitrary
>>> value for the (undefined) code access case, then rely on
>>> unwinding to overwrite that with the correct value for
>>> the data access case.
>>>
>>> Signed-off-by: Richard Henderson <address@hidden>
>>> ---
>>>  target/s390x/excp_helper.c | 23 +++++++----------------
>>>  1 file changed, 7 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
>>> index 98a1ee8317..8ce992e639 100644
>>> --- a/target/s390x/excp_helper.c
>>> +++ b/target/s390x/excp_helper.c
>>> @@ -96,7 +96,7 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int 
>>> size,
>>>  {
>>>      S390CPU *cpu = S390_CPU(cs);
>>>  
>>> -    trigger_pgm_exception(&cpu->env, PGM_ADDRESSING, ILEN_AUTO);
>>> +    trigger_pgm_exception(&cpu->env, PGM_ADDRESSING, ILEN_UNWIND);
>>
>> Hmm, we always trigger a pgm exceptions, meaning we set
>> cs->exception_index even if we have probe = true. Confused by that.
> 
> This is the CONFIG_USER_ONLY version, for which probe is always false.  
> Perhaps
> I shouldn't have made the function interface identical, but it did appear to
> make things cleaner for most targets.
> 
>>> +    trigger_pgm_exception(env, excp, 2);
>>
>> I wonder if it is still worth setting this only conditionally. Most
>> probably not.
> 
> I don't see that it would be.  I hope the comment is clear about this 
> arbitrary
> value is overwritten during unwinding.

It's confusing, but I get it :)

-- 

Thanks,

David / dhildenb