Re: [Qemu-devel] [PATCH 4/14] target/mips: Add CP0_Ebase.WG (write gate) support

[Yongbok Kim <yongbok.kim@imgtec.com>]

On 18/07/2017 12:55, James Hogan wrote:
> Add support for the CP0_EBase.WG bit, which allows upper bits to be
> written (bits 31:30 on MIPS32, or bits 63:30 on MIPS64), along with the
> CP0_Config5.CV bit to control whether the exception vector for Cache
> Error exceptions is forced into KSeg1.
> 
> This is necessary on MIPS32 to support Segmentation Control and Enhanced
> Virtual Addressing (EVA) extensions (where KSeg1 addresses may not
> represent an unmapped uncached segment).
> 
> It is also useful on MIPS64 to allow the exception base to reside in
> XKPhys, and possibly out of range of KSEG0 and KSEG1.
> 
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Yongbok Kim <yongbok.kim@imgtec.com>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> ---
> Changes in v2:
> - Fix CP0_EBase.WG to be read only when WG is not set in
>   CP0_EBase_rw_bitmask, otherwise it will be wrongly probed as present.
> - Make cache error exception vector conditional on Config3.SC as well as
>   Config5.CV, as per the PRA, and take the CP0C3_SC definition from
>   patch 7 (Yongbok).
> - Rename CP0_EBase_rw_bitmask to CP0_EBaseWG_rw_bitmask (Yongbok).
> ---
>  target/mips/cpu.h            |  5 ++++-
>  target/mips/helper.c         | 14 ++++++++------
>  target/mips/machine.c        |  6 +++---
>  target/mips/op_helper.c      | 12 ++++++++++--
>  target/mips/translate.c      |  8 +++++---
>  target/mips/translate_init.c |  1 +
>  6 files changed, 31 insertions(+), 15 deletions(-)
> 


> --- a/target/mips/op_helper.c
> +++ b/target/mips/op_helper.c
> @@ -1515,14 +1515,22 @@ target_ulong helper_mftc0_ebase(CPUMIPSState *env)
>  
>  void helper_mtc0_ebase(CPUMIPSState *env, target_ulong arg1)
>  {
> -    env->CP0_EBase = (env->CP0_EBase & ~0x3FFFF000) | (arg1 & 0x3FFFF000);
> +    target_ulong mask = 0x3FFFF000 | env->CP0_EBaseWG_rw_bitmask;
> +    if (arg1 & (1 << CP0EBase_WG) & mask) {

isn't it just ...?
if (arg1 & env->CP0_EBaseWG_rw_bitmask) {

> +        mask |= ~0x3FFFFFFF;
> +    }
> +    env->CP0_EBase = (env->CP0_EBase & ~mask) | (arg1 & mask);
>  }
>  
>  void helper_mttc0_ebase(CPUMIPSState *env, target_ulong arg1)
>  {
>      int other_tc = env->CP0_VPEControl & (0xff << CP0VPECo_TargTC);
>      CPUMIPSState *other = mips_cpu_map_tc(env, &other_tc);
> -    other->CP0_EBase = (other->CP0_EBase & ~0x3FFFF000) | (arg1 & 0x3FFFF000);
> +    target_ulong mask = 0x3FFFF000 | env->CP0_EBaseWG_rw_bitmask;
> +    if (arg1 & (1 << CP0EBase_WG) & mask) {

here as well.

> +        mask |= ~0x3FFFFFFF;
> +    }
> +    other->CP0_EBase = (other->CP0_EBase & ~mask) | (arg1 & mask);
>  }
>  


Otherwise,
Reviewed-by: Yongbok Kim <yongbok.kim@imgtec.com>

Regards,
Yongbok