On 14.06.2017 06:41, Richard Henderson wrote:
> On 06/13/2017 02:47 PM, David Hildenbrand wrote:
>> +static inline bool psw_key_valid(CPUS390XState *env, uint8_t psw_key)
>> +{
>> + uint16_t pkm = ((env->cregs[3] & CR3_PKM) >> 16);
>> +
>> + if (env->psw.mask & PSW_MASK_PSTATE) {
>> + /* PSW key has range 0..15, it is valid if the bit is 1 in the PKM
>> */
>> + return pkm & (1 << (psw_key & 0xff));
>
> Did you intend to write & 0xf? Otherwise this mask is pointless...
Yes, we will always get only 4 bits set ... but I don't want any
compiler to complain about the shift. So 0xf it is. Thanks!
>
>
>> + switch (src_as) {
>> + case 0x0:
>> + x = cpu_ldub_primary_ra(env, src, ra);
>> + break;
>> + case 0x2:
>> + x = cpu_ldub_secondary_ra(env, src, ra);
>> + break;
>> + case 0x3:
>> + x = cpu_ldub_home_ra(env, src, ra);
>> + break;
>> + }
>> + switch (dest_as) {
>> + case 0x0:
>> + cpu_stb_primary_ra(env, dest, x, ra);
>> + break;
>> + case 0x2:
>> + cpu_stb_secondary_ra(env, dest, x, ra);
>> + break;
>> + case 0x3:
>> + cpu_stb_home_ra(env, dest, x, ra);
>> + break;
>> + }
>
> Rather than these switches, you can use helper_ret_ldub_mmu. Of course, that
> will only work for SOFTMMU. But for CONFIG_USER_ONLY, there's surely only
> one
> address space that's legal, so you could simply forward to fast_memmove.
That sounds like a good idea. I will look into it. This would then allow
to expose the facility also for CONFIG_USER_ONLY.
>
>> + 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.
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()
c) drop potential_page_fault() completely
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?
b) With cpu_restore_state(), there is no need for update_psw_addr() +
update_cc_op(), correct?
Any other nice solution for cleaning potential_page_fault() up?
>
>> + 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).
>
>
> r~
>
--
Thanks,
David
