[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/3] s390x/helper: Added format control bit to M
From: |
Thomas Huth |
Subject: |
Re: [Qemu-devel] [PATCH 3/3] s390x/helper: Added format control bit to MMU translation |
Date: |
Fri, 25 Apr 2014 14:56:13 +0200 |
On Fri, 25 Apr 2014 14:36:11 +0200
Alexander Graf <address@hidden> wrote:
>
> On 25.04.14 14:15, Thomas Huth wrote:
> >
> > On Thu, 24 Apr 2014 18:36:18 +0200
> > Alexander Graf <address@hidden> 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
- [Qemu-devel] [PATCH 0/3] qemu: pending s390x patches, Jens Freimann, 2014/04/24
- [Qemu-devel] [PATCH 3/3] s390x/helper: Added format control bit to MMU translation, Jens Freimann, 2014/04/24
- Re: [Qemu-devel] [PATCH 3/3] s390x/helper: Added format control bit to MMU translation, Alexander Graf, 2014/04/24
- [Qemu-devel] [PATCH 3/3] s390x/helper: Added format control bit to MMU translation, Jens Freimann, 2014/04/24
- Re: [Qemu-devel] [PATCH 3/3] s390x/helper: Added format control bit to MMU translation, Alexander Graf, 2014/04/24
- Re: [Qemu-devel] [PATCH 3/3] s390x/helper: Added format control bit to MMU translation, Thomas Huth, 2014/04/25
- Re: [Qemu-devel] [PATCH 3/3] s390x/helper: Added format control bit to MMU translation, Alexander Graf, 2014/04/25
- Re: [Qemu-devel] [PATCH 3/3] s390x/helper: Added format control bit to MMU translation,
Thomas Huth <=
- Re: [Qemu-devel] [PATCH 3/3] s390x/helper: Added format control bit to MMU translation, Alexander Graf, 2014/04/25
- [Qemu-devel] [PATCH] s390x/helper: Added format control bit to MMU translation, Thomas Huth, 2014/04/25
- Re: [Qemu-devel] [PATCH] s390x/helper: Added format control bit to MMU translation, Alexander Graf, 2014/04/25
- Re: [Qemu-devel] [PATCH] s390x/helper: Added format control bit to MMU translation, Cornelia Huck, 2014/04/30
- Re: [Qemu-devel] [PATCH 3/3] s390x/helper: Added format control bit to MMU translation, Jens Freimann, 2014/04/24
[Qemu-devel] [PATCH 1/3] s390x: empty function stubs in preparation for __KVM_HAVE_GUEST_DEBUG, Jens Freimann, 2014/04/24
[Qemu-devel] [PATCH 2/3] s390x/helper: Fixed real-to-absolute address translation, Jens Freimann, 2014/04/24