On 01.10.19 10:23, Thomas Huth wrote:
> On 01/10/2019 10.17, David Hildenbrand wrote:
>>
>>>> break;
>>>> case ASCE_TYPE_SEGMENT:
>>>> if (VADDR_REGION1_TX(vaddr) || VADDR_REGION2_TX(vaddr) ||
>>>> @@ -253,11 +164,112 @@ static int mmu_translate_asce(CPUS390XState *env,
>>>> target_ulong vaddr,
>>>> if (VADDR_SEGMENT_TL(vaddr) > asce_tl) {
>>>> return PGM_SEGMENT_TRANS;
>>>> }
>>>> + gaddr += VADDR_SEGMENT_TX(vaddr) * 8;
>>>> + break;
>>>> + default:
>>>> + g_assert_not_reached();
>>>
>>> As far as I can see, all four cases are handled above, so this default
>>> case should really not be necessary here.
>>
>> Yes, can drop.
>>
>>>
>>>> + }
>>>> +
>>>> + switch (asce & ASCE_TYPE_MASK) {
>>>> + case ASCE_TYPE_REGION1:
>>>> + if (!read_table_entry(env, gaddr, &entry)) {
>>>> + return PGM_ADDRESSING;
>>>> + }
>>>> + if (entry & REGION_ENTRY_I) {
>>>> + return PGM_REG_FIRST_TRANS;
>>>> + }
>>>> + if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION1) {
>>>> + return PGM_TRANS_SPEC;
>>>> + }
>>>> + if (VADDR_REGION2_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
>>>> + VADDR_REGION2_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
>>>> + return PGM_REG_SEC_TRANS;
>>>> + }
>>>> + if (edat1 && (entry & REGION_ENTRY_P)) {
>>>> + *flags &= ~PAGE_WRITE;
>>>> + }
>>>> + gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION2_TX(vaddr) *
>>>> 8;
>>>> + /* fall through */
>>>> + case ASCE_TYPE_REGION2:
>>>> + if (!read_table_entry(env, gaddr, &entry)) {
>>>> + return PGM_ADDRESSING;
>>>> + }
>>>> + if (entry & REGION_ENTRY_I) {
>>>> + return PGM_REG_SEC_TRANS;
>>>> + }
>>>> + if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION2) {
>>>> + return PGM_TRANS_SPEC;
>>>> + }
>>>> + if (VADDR_REGION3_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
>>>> + VADDR_REGION3_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
>>>> + return PGM_REG_THIRD_TRANS;
>>>> + }
>>>> + if (edat1 && (entry & REGION_ENTRY_P)) {
>>>> + *flags &= ~PAGE_WRITE;
>>>> + }
>>>> + gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION3_TX(vaddr) *
>>>> 8;
>>>> + /* fall through */
>>>> + case ASCE_TYPE_REGION3:
>>>> + if (!read_table_entry(env, gaddr, &entry)) {
>>>> + return PGM_ADDRESSING;
>>>> + }
>>>> + if (entry & REGION_ENTRY_I) {
>>>> + return PGM_REG_THIRD_TRANS;
>>>> + }
>>>> + if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION3) {
>>>> + return PGM_TRANS_SPEC;
>>>> + }
>>>> + if (edat1 && (entry & REGION_ENTRY_P)) {
>>>> + *flags &= ~PAGE_WRITE;
>>>> + }
>>>
>>> Shouldn't that check be done below the next if-statement?
>>
>> Does it matter? The flags are irrelevant in case we return an exception,
>> so the order shouldn't matter.
>
> Hmm, it likely does not matter, but you've got it the other way round in
> all other cases, so I'd vote for doing it here this way, too, for
> consistency.
Oh, in this case, sure! Thanks!
>
> Thomas
>
--
Thanks,
David / dhildenb
