On 01.10.19 10:55, Thomas Huth wrote: > On 01/10/2019 10.51, David Hildenbrand wrote: >> On 01.10.19 10:41, Thomas Huth wrote: >>> On 26/09/2019 12.18, David Hildenbrand wrote: >>>> On 26.09.19 12:16, David Hildenbrand wrote: >>>>> This only adds basic support to the DAT translation, but no EDAT2 support >>>>> for TCG. E.g., the gdbstub under kvm uses this function, too, to >>>>> translate virtual addresses. >>>>> >>>>> Reviewed-by: Thomas Huth <th...@redhat.com> >>>>> Signed-off-by: David Hildenbrand <da...@redhat.com> >>>>> --- >>>>> target/s390x/mmu_helper.c | 9 +++++++++ >>>>> 1 file changed, 9 insertions(+) >>>>> >>>>> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c >>>>> index 6b34c4c7b4..54f54137ec 100644 >>>>> --- a/target/s390x/mmu_helper.c >>>>> +++ b/target/s390x/mmu_helper.c >>>>> @@ -120,6 +120,7 @@ static int mmu_translate_asce(CPUS390XState *env, >>>>> target_ulong vaddr, >>>>> { >>>>> const bool edat1 = (env->cregs[0] & CR0_EDAT) && >>>>> s390_has_feat(S390_FEAT_EDAT); >>>>> + const bool edat2 = edat1 && s390_has_feat(S390_FEAT_EDAT_2); >>>>> const int asce_tl = asce & ASCE_TABLE_LENGTH; >>>>> const int asce_p = asce & ASCE_PRIVATE_SPACE; >>>>> hwaddr gaddr = asce & ASCE_ORIGIN; >>>>> @@ -219,9 +220,17 @@ static int mmu_translate_asce(CPUS390XState *env, >>>>> target_ulong vaddr, >>>>> if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION3) { >>>>> return PGM_TRANS_SPEC; >>>>> } >>>>> + if (edat2 && (entry & REGION3_ENTRY_CR) && asce_p) { >>>>> + return PGM_TRANS_SPEC; >>>>> + } >>>>> if (edat1 && (entry & REGION_ENTRY_P)) { >>>>> *flags &= ~PAGE_WRITE; >>>>> } >>>>> + if (edat2 && (entry & REGION3_ENTRY_FC)) { >>>>> + *raddr = (entry & REGION3_ENTRY_RFAA) | >>>>> + (vaddr & REGION3_ENTRY_RFAA); >>>> >>>> Messed up >>>> >>>> (vaddr & ~REGION3_ENTRY_RFAA) >>>> >>>> it is. >>> >>> With that fix: >>> >>> Reviewed-by: Thomas Huth <th...@redhat.com> >>> >> >> BTW, this change explains the different order of checks you mentioned. I now >> have here: >> >> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c >> index dc33c63b1d..dcbffb682f 100644 >> --- a/target/s390x/mmu_helper.c >> +++ b/target/s390x/mmu_helper.c >> @@ -120,6 +120,7 @@ static int mmu_translate_asce(CPUS390XState *env, >> target_ulong vaddr, >> { >> const bool edat1 = (env->cregs[0] & CR0_EDAT) && >> s390_has_feat(S390_FEAT_EDAT); >> + const bool edat2 = edat1 && s390_has_feat(S390_FEAT_EDAT_2); >> const int asce_tl = asce & ASCE_TABLE_LENGTH; >> const int asce_p = asce & ASCE_PRIVATE_SPACE; >> hwaddr gaddr = asce & ASCE_ORIGIN; >> @@ -217,6 +218,17 @@ static int mmu_translate_asce(CPUS390XState *env, >> target_ulong vaddr, >> if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION3) { >> return PGM_TRANS_SPEC; >> } >> + if (edat2 && (entry & REGION3_ENTRY_CR) && asce_p) { >> + return PGM_TRANS_SPEC; >> + } >> + if (edat2 && (entry & REGION3_ENTRY_FC)) { >> + if (entry & REGION_ENTRY_P) { >> + *flags &= ~PAGE_WRITE; >> + } >> + *raddr = (entry & REGION3_ENTRY_RFAA) | >> + (vaddr & ~REGION3_ENTRY_RFAA); >> + return 0; >> + } >> if (VADDR_SEGMENT_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 || >> VADDR_SEGMENT_TL(vaddr) > (entry & REGION_ENTRY_TL)) { >> return PGM_SEGMENT_TRANS; > > Ah, ok, and the *flags have to be set first, of course. So better keep > it the original way round in your other patch.
I can just move it in this patch, then it's clearer why a different order is needed. -- Thanks, David / dhildenb