Re: [Qemu-devel] [PATCH v2 10/19] target/s390x: use program_interrupt() in per_check_exception()

[David Hildenbrand]

On 07.09.2017 15:32, Cornelia Huck wrote:
> On Mon,  4 Sep 2017 17:43:07 +0200
> David Hildenbrand <address@hidden> wrote:
> 
>> Clean it up by reusing program_interrupt(). Add a concern regarding
>> ilen.
>>
>> Signed-off-by: David Hildenbrand <address@hidden>
>> ---
>>  target/s390x/misc_helper.c | 15 +++++++++------
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
>> index 57c02ddf1b..5096286157 100644
>> --- a/target/s390x/misc_helper.c
>> +++ b/target/s390x/misc_helper.c
>> @@ -446,14 +446,17 @@ void HELPER(chsc)(CPUS390XState *env, uint64_t inst)
>>  #ifndef CONFIG_USER_ONLY
>>  void HELPER(per_check_exception)(CPUS390XState *env)
>>  {
>> -    CPUState *cs = CPU(s390_env_get_cpu(env));
>> +    uint32_t ilen;
>>  
>>      if (env->per_perc_atmid) {
>> -        env->int_pgm_code = PGM_PER;
>> -        env->int_pgm_ilen = get_ilen(cpu_ldub_code(env, env->per_address));
>> -
>> -        cs->exception_index = EXCP_PGM;
>> -        cpu_loop_exit(cs);
>> +        /*
>> +         * FIXME: ILEN_AUTO is most probably the right thing to use. ilen
>> +         * always has to match the instruction referenced in the PSW. E.g.
>> +         * if a PER interrupt is triggered via EXECUTE, we have to use ilen
>> +         * of EXECUTE, while per_address contains the target of EXECUTE.
>> +         */
>> +        ilen = get_ilen(cpu_ldub_code(env, env->per_address));
>> +        program_interrupt(env, PGM_PER, ilen);
>>      }
>>  }
>>  
> 
> Wrapping my head around it: You preserve the current behavior, which
> should probably be changed to use ILEN_AUTO to handle cases like
> EXECUTE correctly?

Exactly, e.g. if EXECUTE executes instruction Y, and Y is to generate an
IFETCH event, AFAIK

a) per_address points at Y
b) old PGM PSW points at instruction after EXECUTE (ignoring nullifying
for now)
c) PGM ilen has to match EXECUTE (so the PSW can properly be rewound)

The case where per_address == (PGM PSW - ilen) (ignoring nullification)
should happen without EXECUTE being involved. That's why PER currently
works just fine with Linux (e.g. uprobe smoke tests when kernel boots up).

Execute handling with PER is just nasty. And one first has to find out
how that plays together with EXECUTE handling in TCG. Therefore, no easy
fix (although ILEN_AUTO might most probably really be the right thing to
do).

> 
> Do you plan to do a follow up? (Should I spend some cycles on it?)
> 

Somewhere on my list. The next thing (in this area) I want do is write
kvm-unit-tests for PER, because also KVM could benefit from that
(EXECUTE handling is just nasty).

So sure, go ahead and have a look at it if you have some spare cycles :)

-- 

Thanks,

David