On Fri, 25 Apr 2014 14:36:11 +0200 Alexander Graf <ag...@suse.de> wrote: > > On 25.04.14 14:15, Thomas Huth wrote: > > > > On Thu, 24 Apr 2014 18:36:18 +0200 > > Alexander Graf <ag...@suse.de> wrote: [...] > >> I also think we should rather align the > >> code with the PTE handling somehow. This way it gets pretty confusing to > >> follow. How about something like this (untested)? > > I gave it a try ... works fine with two small modifications... > > > >> diff --git a/target-s390x/helper.c b/target-s390x/helper.c > >> index aa628b8..96c1c66 100644 > >> --- a/target-s390x/helper.c > >> +++ b/target-s390x/helper.c > >> @@ -170,6 +170,48 @@ static void trigger_page_fault(CPUS390XState *env, > >> target_ulong vaddr, > >> trigger_pgm_exception(env, type, ilen); > >> } > >> > >> +static int mmu_translate_pte(CPUS390XState *env, target_ulong vaddr, > >> + uint64_t asc, uint64_t asce, > >> + target_ulong *raddr, int *flags, int rw) > >> +{ > >> + if (asce & _PAGE_INVALID) { > >> + DPRINTF("%s: PTE=0x%" PRIx64 " invalid\n", __func__, asce); > >> + trigger_page_fault(env, vaddr, PGM_PAGE_TRANS, asc, rw); > >> + return -1; > >> + } > >> + > >> + if (asce & _PAGE_RO) { > >> + *flags &= ~PAGE_WRITE; > >> + } > >> + > >> + *raddr = asce & _ASCE_ORIGIN; > >> + > >> + PTE_DPRINTF("%s: PTE=0x%" PRIx64 "\n", __func__, asce); > >> + > >> + return 0; > >> +} > >> + > >> +static int mmu_translate_segpte(CPUS390XState *env, target_ulong vaddr, > >> + uint64_t asc, uint64_t asce, > >> + target_ulong *raddr, int *flags, int rw) > >> +{ > >> + if (asce & _SEGMENT_ENTRY_INV) { > >> + DPRINTF("%s: SEG=0x%" PRIx64 " invalid\n", __func__, asce); > >> + trigger_page_fault(env, vaddr, PGM_SEGMENT_TRANS, asc, rw); > >> + return -1; > >> + } > >> + > >> + if (asce & _PAGE_RO) { /* XXX is this correct? */ > > You can remove the XXX comment, the protection bit is the same for > > both, page table entries and segment table entries. > > > >> + *flags &= ~PAGE_WRITE; > >> + } > >> + > >> + *raddr = (asce & 0xfffffffffff00000ULL) | (vaddr & 0xfffff); > >> + > >> + PTE_DPRINTF("%s: SEG=0x%" PRIx64 "\n", __func__, asce); > >> + > >> + return 0; > >> +} > >> + > >> static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr, > >> uint64_t asc, uint64_t asce, int level, > >> target_ulong *raddr, int *flags, int rw) > >> @@ -229,28 +271,19 @@ static int mmu_translate_asce(CPUS390XState *env, > >> target_ulong vaddr, > >> PTE_DPRINTF("%s: 0x%" PRIx64 " + 0x%" PRIx64 " => 0x%016" PRIx64 > >> "\n", > >> __func__, origin, offs, new_asce); > >> > >> - if (level != _ASCE_TYPE_SEGMENT) { > >> + } if (level == _ASCE_TYPE_SEGMENT) { > > I had to remove the "}" at the beginning of above line. > > > >> + /* 4KB page */ > >> + return mmu_translate_pte(env, vaddr, asc, new_asce, raddr, > >> flags, rw); > >> + } else if (((level - 4) == _ASCE_TYPE_SEGMENT) && > >> + (env->cregs[0] & CR0_EDAT) && > >> + (asce & _SEGMENT_ENTRY_FC)) { > > That's got to be "(new_asce & _SEGMENT_ENTRY_FC)" instead. > > > >> + /* 1MB page */ > >> + return mmu_translate_segpte(env, vaddr, asc, new_asce, raddr, > >> flags, rw); > >> + } else { > >> /* yet another region */ > >> return mmu_translate_asce(env, vaddr, asc, new_asce, level - > >> 4, raddr, > >> flags, rw); > >> } > >> - > >> - /* PTE */ > >> - if (new_asce & _PAGE_INVALID) { > >> - DPRINTF("%s: PTE=0x%" PRIx64 " invalid\n", __func__, new_asce); > >> - trigger_page_fault(env, vaddr, PGM_PAGE_TRANS, asc, rw); > >> - return -1; > >> - } > >> - > >> - if (new_asce & _PAGE_RO) { > >> - *flags &= ~PAGE_WRITE; > >> - } > >> - > >> - *raddr = new_asce & _ASCE_ORIGIN; > >> - > >> - PTE_DPRINTF("%s: PTE=0x%" PRIx64 "\n", __func__, new_asce); > >> - > >> - return 0; > >> } > >> > >> static int mmu_translate_asc(CPUS390XState *env, target_ulong vaddr, > > I'm fine with these changes, too. So how shall we continue? Could you > > assemble a complete patch or shall I prepare one? > > You go ahead and do one. If you can think of a good way to combine > mmu_translate_pte and mmu_translate_seg into a single function I'd be > happy to see that too :). > > With the RO flag identical the only differences left are the page mask > (4k is implicit for QEMU's TLB, that's why it's omitted for normal PTEs) > and the page fault injection. Do 1MB PTEs fault with a page fault or a > segment fault? If they fault with a page fault, it's probably the best > to just have one mmu_translate_pte() function with a mask parameter.
No, you always get segment translation exceptions instead. And there is another difference: The entry-is-invalid bit is at a different location (_PAGE_INVALID vs. _SEGMENT_ENTRY_INV). So I think it's nicer to keep the functions separated instead of adding too much if-statements or functions parameters there, ok? Thomas