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

[David Hildenbrand <david@redhat.com>]

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

Thanks for the clarification!

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

By temporary I meant:
I will be looking into cleaning this all up and getting rid of
potential_page_fault() soon :)

However, in v2 I avoided potential_page_fault().

> 
> 
> r~
> 

Thanks!

-- 

Thanks,

David