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

[Cornelia Huck]

On Thu, 7 Sep 2017 15:52:19 +0200
David Hildenbrand <address@hidden> wrote:

> 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).

That really sounds like a lot of fun...

> 
> > 
> > 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 :)

Spare cycles? Me? :)

I think we can live with the current state a bit longer. Adding the
FIXME is actually an improvement...