Re: [PATCH v5 1/2] target/s390x: Implement the MVPG condition-code-option bit

[David Hildenbrand <david@redhat.com>]

On 11.03.21 18:02, Richard Henderson wrote:
> On 3/11/21 10:17 AM, David Hildenbrand wrote:
>> +#if !defined(CONFIG_USER_ONLY)
>> +    if (env->tlb_fill_exc) {
>> +        return env->tlb_fill_exc;
>> +    }
>> +#else
>> +    if (!haddr1) {
>> +        env->__excp_addr = vaddr1;
>> +        return PGM_ADDRESSING;
>> +    }
>> +#endif
> 
> For user-only, we can rely on TLB_INVALID_MASK, and check once for both pages.

Then, I cannot set the proper vaddr1 vs. vaddr2 (see patch #2).


>> @@ -858,13 +925,26 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2)
>>        /*
>>         * TODO:
>>         * - Access key handling
>> -     * - CC-option with surpression of page-translation exceptions
>>         * - Store r1/r2 register identifiers at real location 162
>>         */
>> -    srca = access_prepare(env, r2, TARGET_PAGE_SIZE, MMU_DATA_LOAD, mmu_idx,
>> -                          ra);
>> -    desta = access_prepare(env, r1, TARGET_PAGE_SIZE, MMU_DATA_STORE, mmu_idx,
>> -                           ra);
>> +    exc = access_prepare_nf(&srca, env, cco, r2, TARGET_PAGE_SIZE,
>> +                            MMU_DATA_LOAD, mmu_idx, ra);
>> +    if (exc) {
>> +        return 2;
>> +    }
>> +    exc = access_prepare_nf(&desta, env, cco, r1, TARGET_PAGE_SIZE,
>> +                            MMU_DATA_STORE, mmu_idx, ra);
>> +    if (exc) {
>> +#if !defined(CONFIG_USER_ONLY)
>> +        if (exc == PGM_PROTECTION) {
>> +            stq_phys(env_cpu(env)->as,
>> +                     env->psa + offsetof(LowCore, trans_exc_code),
>> +                     env->tlb_fill_tec);
>> +            tcg_s390_program_interrupt(env, PGM_PROTECTION, ra);
>> +        }
>> +#endif
>> +        return 1;
>> +    }
>>        access_memmove(env, &desta, &srca, ra);
>>        return 0; /* data moved */
>>    }
> 
> If we're going to have an ifdef at all here, it should be around the entire
> helper -- this is a privileged operation.


Privileged operation (access key specified, and
selected PSW-key-mask bit is zero in the prob-
lem state)

Without an access key in GR0, we're using the PSW key - which should 
always work, no?

What am I missing?

-- 
Thanks,

David / dhildenb