Re: [PATCH V17 1/6] target/mips: Fix PageMask with variable page size - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
To: Huacai Chen <zltjiangshi@gmail.com>
Cc: Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>,
	Huacai Chen <chenhuacai@gmail.com>,
	qemu-devel@nongnu.org,
	Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>,
	Huacai Chen <chenhc@lemote.com>,
	Aurelien Jarno <aurelien@aurel32.net>
Subject: Re: [PATCH V17 1/6] target/mips: Fix PageMask with variable page size
Date: Sat, 7 Nov 2020 13:11:40 +0100	[thread overview]
Message-ID: <9b081173-c4f6-28b8-178c-7137e911d2f6@amsat.org> (raw)
In-Reply-To: <1604636510-8347-2-git-send-email-chenhc@lemote.com>

Hi Huacai,

On 11/6/20 5:21 AM, Huacai Chen wrote:
> From: Jiaxun Yang <jiaxun.yang@flygoat.com>
> 
> Our current code assumed the target page size is always 4k
> when handling PageMask and VPN2, however, variable page size
> was just added to mips target and that's no longer true.
> 
> Fixes: ee3863b9d414 ("target/mips: Support variable page size")
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> Signed-off-by: Huacai Chen <chenhc@lemote.com>
> ---
>  target/mips/cp0_helper.c | 27 +++++++++++++++++++++------
>  target/mips/cpu.h        |  1 +
>  2 files changed, 22 insertions(+), 6 deletions(-)

Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
> diff --git a/target/mips/cp0_helper.c b/target/mips/cp0_helper.c
> index 709cc9a..92bf14f 100644
> --- a/target/mips/cp0_helper.c
> +++ b/target/mips/cp0_helper.c
> @@ -892,13 +892,28 @@ void helper_mtc0_memorymapid(CPUMIPSState *env, target_ulong arg1)
>  
>  void update_pagemask(CPUMIPSState *env, target_ulong arg1, int32_t *pagemask)
>  {
> -    uint64_t mask = arg1 >> (TARGET_PAGE_BITS + 1);
> -    if (!(env->insn_flags & ISA_MIPS32R6) || (arg1 == ~0) ||
> -        (mask == 0x0000 || mask == 0x0003 || mask == 0x000F ||
> -         mask == 0x003F || mask == 0x00FF || mask == 0x03FF ||
> -         mask == 0x0FFF || mask == 0x3FFF || mask == 0xFFFF)) {
> -        env->CP0_PageMask = arg1 & (0x1FFFFFFF & (TARGET_PAGE_MASK << 1));
> +    unsigned long mask;
> +    int maskbits;
> +
> +    /* Don't care MASKX as we don't support 1KB page */
> +    mask = extract32((uint32_t)arg1, CP0PM_MASK, 16);

I'd simply use extract64().

> +    maskbits = find_first_zero_bit(&mask, 32);
> +
> +    /* Ensure no more set bit after first zero */
> +    if (mask >> maskbits) {
> +        goto invalid;
> +    }
> +    /* We don't support VTLB entry smaller than target page */
> +    if ((maskbits + 12) < TARGET_PAGE_BITS) {

I suppose the magic 12 is TARGET_PAGE_BITS_MIN, right?

If you confirm I can do the change when applying (no need to
repost the whole series).

> +        goto invalid;
>      }
> +    env->CP0_PageMask = mask << CP0PM_MASK;
> +
> +    return;
> +
> +invalid:
> +    /* When invalid, set to default target page size. */
> +    env->CP0_PageMask = (~TARGET_PAGE_MASK >> 12) << CP0PM_MASK;

Ditto.

>  }
>  
>  void helper_mtc0_pagemask(CPUMIPSState *env, target_ulong arg1)
> diff --git a/target/mips/cpu.h b/target/mips/cpu.h
> index d41579d..23f8c6f 100644
> --- a/target/mips/cpu.h
> +++ b/target/mips/cpu.h
> @@ -619,6 +619,7 @@ struct CPUMIPSState {
>   * CP0 Register 5
>   */
>      int32_t CP0_PageMask;
> +#define CP0PM_MASK 13
>      int32_t CP0_PageGrain_rw_bitmask;
>      int32_t CP0_PageGrain;
>  #define CP0PG_RIE 31
> 