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

[Yongbok Kim]

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.

> +        /* 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.

> +
> +    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.

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