diff mbox series

[V17,1/6] target/mips: Fix PageMask with variable page size

Message ID 1604636510-8347-2-git-send-email-chenhc@lemote.com
State New
Headers show
Series mips: Add Loongson-3 machine support | expand

Commit Message

chen huacai Nov. 6, 2020, 4:21 a.m. UTC 
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(-)

Comments

Philippe Mathieu-Daudé Nov. 7, 2020, 12:11 p.m. UTC | #1 
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
>
Huacai Chen Nov. 8, 2020, 1:34 a.m. UTC | #2 
Hi, Philippe,

On Sat, Nov 7, 2020 at 8:11 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> 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).
Yes, 12 is TARGET_PAGE_BITS_MIN.

Huacai
>
> > +        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
> >
Philippe Mathieu-Daudé Nov. 8, 2020, 11:15 p.m. UTC | #3 
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(-)
> 
> 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);
> +    maskbits = find_first_zero_bit(&mask, 32);

Thanks, applied to mips-fixes, using cto32 instead
of find_first_zero_bit.

> +
> +    /* 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) {
> +        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;
>  }
>  
>  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
>
diff mbox series

Patch

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);
+    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) {
+        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;
 }
 
 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