Re: [Qemu-devel] [PATCH v1 2/3] target/s390x: implement mvcos instruction

[Richard Henderson]

On 06/14/2017 12:22 AM, David Hildenbrand wrote:
> > > +    if (!(env->psw.mask & PSW_MASK_DAT)) {
> > > +        program_interrupt(env, PGM_SPECIAL_OP, 6);
> > > +    }
> >
> > You should use restore_program_state before program_interrupt (or add a new
> > entry-point to do both).  Then you can drop ...
>
> restore_program_state -  you mean cpu_restore_state() I assume.

Yes, sorry.

> Would it makes sense to
>
> a) move cpu_restore_state() into program_interrupt()
> b) make all callers forward ra from GETPC() (problem with kvm code that
> share handlers?)
> c) fixup callers that already do the cpu_restore_state()
> d) drop potential_page_fault() completely

Yes, that makes sense.  For B, kvm can pass 0 for RA and nothing will happen. 
For C, that project is on-going but not complete; D is indeed the ultimate goal.

> Two questions:
> a) Could we avoid having to forward the ra by doing GETPC directly in
> program_interrupt()? In mem_helper, I can see that we do GETPC on
> several places and pass it around, so I assume GETPC() has to be called
> in the first handler?

You must use GETPC in the first handler.  We're looking for the address of the 
TCG generated code from where we were called.  So, no, you can't use GETPC from 
program_interrupt.

> b) With cpu_restore_state(), there is no need for update_psw_addr() +
> update_cc_op(), correct?

Correct.

> > > +    potential_page_fault(s);
> > > +    gen_helper_mvcos(cc_op, cpu_env, o->addr1, o->in2, regs[r3]);
> >
> > ... the potential_page_fault.
>
> I would suggest to leave it in this patch as it and then clean it up all
> together in one shot (adding 5 cpu_restore_state() vs. one
> potential_page_fault() temporarily looks better to me).

I would say the opposite, since the code generated by potential_page_fault is 
always executed, whereas the cpu_restore_state is on an error path which for a 
well-behaved guest will never be executed.


r~