Re: [Qemu-devel] [PATCH 3/3] s390x/helper: Added format control bit to MMU translation

[Thomas Huth]

 Hi Alex,

On Thu, 24 Apr 2014 18:36:18 +0200
Alexander Graf <address@hidden> wrote:
>
> On 24.04.14 17:34, Jens Freimann wrote:
> > From: Thomas Huth <address@hidden>
> >
> > With the EDAT-1 facility, the MMU translation can stop at the
> > segment table already, pointing to a 1 MB block.
... 
> > diff --git a/target-s390x/helper.c b/target-s390x/helper.c
> > index aa628b8..89dc6e7 100644
> > --- a/target-s390x/helper.c
> > +++ b/target-s390x/helper.c
> > @@ -217,6 +217,10 @@ static int mmu_translate_asce(CPUS390XState *env, 
> > target_ulong vaddr,
> >           offs = (vaddr >> 17) & 0x3ff8;
> >           break;
> >       case _ASCE_TYPE_SEGMENT:
> > +        if (env && (env->cregs[0] & CR0_EDAT) && (asce & 
> > _SEGMENT_ENTRY_FC)) {
> > +            *raddr = (asce & 0xfffffffffff00000ULL) | (vaddr & 0xfffff);
> > +            return 0;
> > +        }
> 
> This is missing the page flags.

D'oh, missed that :-(

> 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?

 Thomas