Re: [Qemu-devel] [PATCH 8/9] target-mips: Implement segmentation control

[James Hogan]

On Thu, Oct 13, 2016 at 02:06:25PM +0100, Yongbok Kim wrote:
> 
> 
> On 06/09/2016 12:03, James Hogan wrote:
> > Implement the optional segmentation control feature in the virtual to
> > physical address translation code.
> > 
> > The fixed legacy segment and XKPhys handling is replaced with a dynamic
> > layout based on the segmentation control registers (which should be set
> > up even when the feature is not exposed to the guest).
> > 
> > Signed-off-by: James Hogan <address@hidden>
> > Cc: Leon Alrae <address@hidden>
> > Cc: Aurelien Jarno <address@hidden>
> > ---
> >  target-mips/helper.c | 154 +++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 123 insertions(+), 31 deletions(-)
> > 
> > diff --git a/target-mips/helper.c b/target-mips/helper.c
> > index 2065fc3ec119..63d709bd620f 100644
> > --- a/target-mips/helper.c
> > +++ b/target-mips/helper.c
> > @@ -107,15 +107,107 @@ int r4k_map_address (CPUMIPSState *env, hwaddr 
> > *physical, int *prot,
> >      return TLBRET_NOMATCH;
> >  }
> >  
> > +static int is_seg_am_mapped(unsigned int am, bool eu, int mmu_idx)
> > +{
> 
> Nice :) I like this simplified logic to decode access controls.
> 
> > +    /*
> > +     * Interpret access control mode and mmu_idx.
> > +     *           AdE?     TLB?
> > +     *      AM  K S U E  K S U E
> > +     * UK    0  0 1 1 0  0 - - 0
> > +     * MK    1  0 1 1 0  1 - - !eu
> > +     * MSK   2  0 0 1 0  1 1 - !eu
> > +     * MUSK  3  0 0 0 0  1 1 1 !eu
> > +     * MUSUK 4  0 0 0 0  0 1 1 0
> > +     * USK   5  0 0 1 0  0 0 - 0
> > +     * -     6  - - - -  - - - -
> > +     * UUSK  7  0 0 0 0  0 0 0 0
> > +     */
> > +    int32_t adetlb_mask;
> > +
> > +    switch (mmu_idx) {
> > +    case 3 /* ERL */:
> > +        /* If EU is set, always unmapped */
> > +        if (eu) {
> > +            return 0;
> > +        }
> 
> I guess checking ERL and EU has to be outside of the switch case.
> If ERL = 1 and EU = 0 then it has to check with the current Privilege level
> but in this case it falls through and always checks against Kernel mode.

No, "The processor is in Kernel Mode when the DM bit in the Debug
register is 0... and any of the following is true... The ERL bit in the
Status register is 1"

See how compute_hflags() leaves hflags & KSU == KM in the specified
cases.

> 
> > +        /* fall through */
> > +    case MIPS_HFLAG_KM:
> > +        /* Never AdE, TLB mapped if AM={1,2,3} */
> > +        adetlb_mask = 0x70000000;
> > +        goto check_tlb;
> > +
> > +    case MIPS_HFLAG_SM:
> > +        /* AdE if AM={0,1}, TLB mapped if AM={2,3,4} */
> > +        adetlb_mask = 0xc0380000;
> > +        goto check_ade;
> > +
> > +    case MIPS_HFLAG_UM:
> > +        /* AdE if AM={0,1,2,5}, TLB mapped if AM={3,4} */
> > +        adetlb_mask = 0xe4180000;
> > +        /* fall through */
> > +    check_ade:
> > +        /* does this AM cause AdE in current execution mode */
> > +        if ((adetlb_mask << am) < 0) {
> > +            return TLBRET_BADADDR;
> > +        }
> > +        adetlb_mask <<= 8;
> > +        /* fall through */
> > +    check_tlb:
> > +        /* is this AM mapped in current execution mode */
> > +        return ((adetlb_mask << am) < 0);
> > +    default:
> > +        assert(0);
> > +        return TLBRET_BADADDR;
> > +    };
> > +}
> > +
> > +static int get_seg_physical_address(CPUMIPSState *env, hwaddr *physical,
> > +                                    int *prot, target_ulong real_address,
> > +                                    int rw, int access_type, int mmu_idx,
> > +                                    unsigned int am, bool eu,
> > +                                    target_ulong segmask,
> > +                                    target_ulong physical_base)
> 
> uint64_t for physical_base. see below comment.
> 
> > +{
> > +    int mapped = is_seg_am_mapped(am, eu, mmu_idx);
> > +
> > +    if (mapped < 0) {
> > +        /* is_seg_am_mapped can report TLBRET_BADADDR */
> > +        return mapped;
> > +    } else if (mapped) {
> > +        /* The segment is TLB mapped */
> > +        return env->tlb->map_address(env, physical, prot, real_address, rw,
> > +                                     access_type);
> > +    } else {
> > +        /* The segment is unmapped */
> > +        *physical = physical_base | (real_address & segmask);
> > +        *prot = PAGE_READ | PAGE_WRITE;
> > +        return TLBRET_MATCH;
> > +    }
> > +}
> > +
> > +static int get_segctl_physical_address(CPUMIPSState *env, hwaddr *physical,
> > +                                       int *prot, target_ulong 
> > real_address,
> > +                                       int rw, int access_type, int 
> > mmu_idx,
> > +                                       uint16_t segctl, target_ulong 
> > segmask)
> > +{
> > +    unsigned int am = (segctl & CP0SC_AM_MASK) >> CP0SC_AM;
> > +    bool eu = (segctl >> CP0SC_EU) & 1;
> > +    target_ulong pa = ((target_ulong)segctl & CP0SC_PA_MASK) << 20;
> 
> According to the architecture spec, PA supports mapping of up to a 36-bit
> physical address. uint64_t would be better for PA and physical_base.

Yes, good catch.

> 
> > +
> > +    return get_seg_physical_address(env, physical, prot, real_address, rw,
> > +                                    access_type, mmu_idx, am, eu, segmask,
> > +                                    pa & ~segmask);
> > +}
> > +
> >  static int get_physical_address (CPUMIPSState *env, hwaddr *physical,
> >                                  int *prot, target_ulong real_address,
> >                                  int rw, int access_type, int mmu_idx)
> >  {
> >      /* User mode can only access useg/xuseg */
> > +#if defined(TARGET_MIPS64)
> >      int user_mode = mmu_idx == MIPS_HFLAG_UM;
> >      int supervisor_mode = mmu_idx == MIPS_HFLAG_SM;
> >      int kernel_mode = !user_mode && !supervisor_mode;
> > -#if defined(TARGET_MIPS64)
> >      int UX = (env->CP0_Status & (1 << CP0St_UX)) != 0;
> >      int SX = (env->CP0_Status & (1 << CP0St_SX)) != 0;
> >      int KX = (env->CP0_Status & (1 << CP0St_KX)) != 0;
> > @@ -148,12 +240,16 @@ static int get_physical_address (CPUMIPSState *env, 
> > hwaddr *physical,
> >  
> >      if (address <= USEG_LIMIT) {
> >          /* useg */
> > -        if (env->CP0_Status & (1 << CP0St_ERL)) {
> > -            *physical = address & 0xFFFFFFFF;
> > -            *prot = PAGE_READ | PAGE_WRITE;
> > +        uint16_t segctl;
> > +
> > +        if (address >= 0x40000000UL) {
> > +            segctl = env->CP0_SegCtl2;
> >          } else {
> > -            ret = env->tlb->map_address(env, physical, prot, real_address, 
> > rw, access_type);
> > +            segctl = env->CP0_SegCtl2 >> 16;
> >          }
> > +        ret = get_segctl_physical_address(env, physical, prot, 
> > real_address, rw,
> > +                                          access_type, mmu_idx, segctl,
> > +                                          0x3FFFFFFF);
> >  #if defined(TARGET_MIPS64)
> >      } else if (address < 0x4000000000000000ULL) {
> >          /* xuseg */
> > @@ -172,10 +268,16 @@ static int get_physical_address (CPUMIPSState *env, 
> > hwaddr *physical,
> >          }
> >      } else if (address < 0xC000000000000000ULL) {
> >          /* xkphys */
> > -        if (kernel_mode && KX &&
> > -            (address & 0x07FFFFFFFFFFFFFFULL) <= env->PAMask) {
> > -            *physical = address & env->PAMask;
> > -            *prot = PAGE_READ | PAGE_WRITE;
> > +        if (KX && (address & 0x07FFFFFFFFFFFFFFULL) <= env->PAMask) {
> 
> KX should be dropped from here.
> More over, we need to check KX, SX and UX whether it meets minimum
> privilege level according to AM.

Oh, hmm... good point!

Cheers
James

> 
> > +            unsigned int am = CP0SC_AM_UK;
> 
> When the region is disabled, it is valid for Kernel mode only.
> 
> > +            unsigned int xr = (env->CP0_SegCtl2 & CP0SC2_XR_MASK) >> 
> > CP0SC2_XR;
> > +
> > +            if (xr & (1 << ((address >> 59) & 0x7))) {
> > +                am = (env->CP0_SegCtl1 & CP0SC1_XAM_MASK) >> CP0SC1_XAM;
> > +            }
> > +            ret = get_seg_physical_address(env, physical, prot, 
> > real_address,
> > +                                           rw, access_type, mmu_idx, am, 
> > false,
> > +                                           env->PAMask, 0);
> >          } else {
> >              ret = TLBRET_BADADDR;
> >          }
> > @@ -190,35 +292,25 @@ static int get_physical_address (CPUMIPSState *env, 
> > hwaddr *physical,
> >  #endif
> >      } else if (address < (int32_t)KSEG1_BASE) {
> >          /* kseg0 */
> > -        if (kernel_mode) {
> > -            *physical = address - (int32_t)KSEG0_BASE;
> > -            *prot = PAGE_READ | PAGE_WRITE;
> > -        } else {
> > -            ret = TLBRET_BADADDR;
> > -        }
> > +        ret = get_segctl_physical_address(env, physical, prot, 
> > real_address, rw,
> > +                                          access_type, mmu_idx,
> > +                                          env->CP0_SegCtl1 >> 16, 
> > 0x1FFFFFFF);
> >      } else if (address < (int32_t)KSEG2_BASE) {
> >          /* kseg1 */
> > -        if (kernel_mode) {
> > -            *physical = address - (int32_t)KSEG1_BASE;
> > -            *prot = PAGE_READ | PAGE_WRITE;
> > -        } else {
> > -            ret = TLBRET_BADADDR;
> > -        }
> > +        ret = get_segctl_physical_address(env, physical, prot, 
> > real_address, rw,
> > +                                          access_type, mmu_idx,
> > +                                          env->CP0_SegCtl1, 0x1FFFFFFF);
> >      } else if (address < (int32_t)KSEG3_BASE) {
> >          /* sseg (kseg2) */
> > -        if (supervisor_mode || kernel_mode) {
> > -            ret = env->tlb->map_address(env, physical, prot, real_address, 
> > rw, access_type);
> > -        } else {
> > -            ret = TLBRET_BADADDR;
> > -        }
> > +        ret = get_segctl_physical_address(env, physical, prot, 
> > real_address, rw,
> > +                                          access_type, mmu_idx,
> > +                                          env->CP0_SegCtl0 >> 16, 
> > 0x1FFFFFFF);
> >      } else {
> >          /* kseg3 */
> >          /* XXX: debug segment is not emulated */
> > -        if (kernel_mode) {
> > -            ret = env->tlb->map_address(env, physical, prot, real_address, 
> > rw, access_type);
> > -        } else {
> > -            ret = TLBRET_BADADDR;
> > -        }
> > +        ret = get_segctl_physical_address(env, physical, prot, 
> > real_address, rw,
> > +                                          access_type, mmu_idx,
> > +                                          env->CP0_SegCtl0, 0x1FFFFFFF);
> >      }
> >      return ret;
> >  }
> > 
> 
> 
> Regards,
> Yongbok

signature.asc
Description: Digital signature