diff mbox series

target/mips: Fix PageMask with variable page size

Message ID 20200614034729.3004-1-jiaxun.yang@flygoat.com
State New
Headers show
Series target/mips: Fix PageMask with variable page size | expand

Commit Message

Jiaxun Yang June 14, 2020, 3:47 a.m. UTC 
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 nolonger true.

So we refined this piece of code to handle any target page size.
Also added Big Page support defined by MIPS64 Release2.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 target/mips/cp0_helper.c | 48 ++++++++++++++++++++++++++++++----------
 target/mips/cpu.h        |  3 ++-
 target/mips/machine.c    |  2 +-
 3 files changed, 39 insertions(+), 14 deletions(-)

Comments

no-reply@patchew.org June 14, 2020, 4:13 a.m. UTC | #1 
Patchew URL: https://patchew.org/QEMU/20200614034729.3004-1-jiaxun.yang@flygoat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

+++ /tmp/qemu-test/build/tests/qemu-iotests/041.out.bad 2020-06-14 04:06:28.925257078 +0000
@@ -1,5 +1,30 @@
-........................................................................................................
+WARNING:qemu.machine:qemu received signal 9: /tmp/qemu-test/build/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64 -display none -vga none -chardev socket,id=mon,path=/tmp/tmp.TpYwtp98hn/qemu-21810-monitor.sock -mon chardev=mon,mode=control -qtest unix:path=/tmp/tmp.TpYwtp98hn/qemu-21810-qtest.sock -accel qtest -nodefaults -display none -accel qtest -drive if=virtio,id=drive0,file=/tmp/qemu-test/test.img,format=qcow2,cache=writeback,aio=threads,node-name=top,backing.node-name=base -drive if=ide,id=drive1,media=cdrom
+...........................................................................E............................
+======================================================================
+ERROR: test_pause (__main__.TestSingleDrive)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "041", line 108, in test_pause
---
 Ran 104 tests
 
-OK
+FAILED (errors=1)
  TEST    iotest-qcow2: 042
  TEST    iotest-qcow2: 043
  TEST    iotest-qcow2: 046
---
Not run: 259
Failures: 041
Failed 1 of 119 iotests
make: *** [check-tests/check-block.sh] Error 1
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 669, in <module>
    sys.exit(main())
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=6f22b3d3f1444c1098a62865239c3b98', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-5di02x2_/src/docker-src.2020-06-13-23.55.49.5246:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=6f22b3d3f1444c1098a62865239c3b98
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-5di02x2_/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    17m14.597s
user    0m8.192s


The full log is available at
http://patchew.org/logs/20200614034729.3004-1-jiaxun.yang@flygoat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Aleksandar Markovic June 14, 2020, 9:51 p.m. UTC | #2 
недеља, 14. јун 2020., 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 nolonger true.
>
> So we refined this piece of code to handle any target page size.
> Also added Big Page support defined by MIPS64 Release2.
>
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>  target/mips/cp0_helper.c | 48 ++++++++++++++++++++++++++++++----------


Hi, Jiaxun.

Thanks for this contribution too.

When you change machine.c the way you did it, you need to bump the version.
Please see git log on machine.c for details.

Also, this patch covers logicaly related, but slightly different and, in
some cases almost independanr things, and should be split to two or more
logical units - in other words, patches.

Thanks,
Akeksandar


>  target/mips/cpu.h        |  3 ++-
>  target/mips/machine.c    |  2 +-
>  3 files changed, 39 insertions(+), 14 deletions(-)
>
> diff --git a/target/mips/cp0_helper.c b/target/mips/cp0_helper.c
> index bbf12e4a97..7a134085f7 100644
> --- a/target/mips/cp0_helper.c
> +++ b/target/mips/cp0_helper.c
> @@ -872,20 +872,44 @@ void helper_mtc0_memorymapid(CPUMIPSState *env,
> target_ulong arg1)
>      }
>  }
>
> -void update_pagemask(CPUMIPSState *env, target_ulong arg1, int32_t
> *pagemask)
> +void helper_mtc0_pagemask(CPUMIPSState *env, target_ulong arg1)
>  {
> -    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));
> +    uint64_t mask;
> +    int maxmaskbits, maskbits;
> +
> +    if (env->insn_flags & ISA_MIPS32R6) {
> +        return;
>      }
> -}
>
> -void helper_mtc0_pagemask(CPUMIPSState *env, target_ulong arg1)
> -{
> -    update_pagemask(env, arg1, &env->CP0_PageMask);
> +    /* Don't care MASKX as we don't support 1KB page */
> +#ifdef TARGET_MIPS64
> +    if (env->CP0_Config3 & CP0C3_BPG) {
> +        maxmaskbits = 47;
> +    } else {
> +        maxmaskbits = 16;
> +    }
> +#else
> +    maxmaskbits = 16;
> +#endif
> +    mask = extract64((uint64_t)arg1, CP0PM_MASK, maxmaskbits);
> +
> +    maskbits = find_first_zero_bit(&mask, 64);
> +
> +    /* 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:
> +    maskbits = MIN(maxmaskbits, MAX(maskbits, TARGET_PAGE_BITS - 12));
> +    env->CP0_PageMask = ((1 << (maskbits + 1)) - 1) << CP0PM_MASK;
>  }
>
>  void helper_mtc0_pagegrain(CPUMIPSState *env, target_ulong arg1)
> @@ -1111,7 +1135,7 @@ void helper_mthc0_saar(CPUMIPSState *env,
> target_ulong arg1)
>  void helper_mtc0_entryhi(CPUMIPSState *env, target_ulong arg1)
>  {
>      target_ulong old, val, mask;
> -    mask = (TARGET_PAGE_MASK << 1) | env->CP0_EntryHi_ASID_mask;
> +    mask = ~((1 << 14) - 1) | env->CP0_EntryHi_ASID_mask;
>      if (((env->CP0_Config4 >> CP0C4_IE) & 0x3) >= 2) {
>          mask |= 1 << CP0EnHi_EHINV;
>      }
> diff --git a/target/mips/cpu.h b/target/mips/cpu.h
> index 0b3c987bb3..b69806792d 100644
> --- a/target/mips/cpu.h
> +++ b/target/mips/cpu.h
> @@ -617,7 +617,8 @@ struct CPUMIPSState {
>  /*
>   * CP0 Register 5
>   */
> -    int32_t CP0_PageMask;
> +    target_ulong CP0_PageMask;
> +#define CP0PM_MASK 13
>      int32_t CP0_PageGrain_rw_bitmask;
>      int32_t CP0_PageGrain;
>  #define CP0PG_RIE 31
> diff --git a/target/mips/machine.c b/target/mips/machine.c
> index 5b23e3e912..b30a75d46b 100644
> --- a/target/mips/machine.c
> +++ b/target/mips/machine.c
> @@ -252,7 +252,7 @@ const VMStateDescription vmstate_mips_cpu = {
>          VMSTATE_UINT64(env.CP0_EntryLo1, MIPSCPU),
>          VMSTATE_UINTTL(env.CP0_Context, MIPSCPU),
>          VMSTATE_INT32(env.CP0_MemoryMapID, MIPSCPU),
> -        VMSTATE_INT32(env.CP0_PageMask, MIPSCPU),
> +        VMSTATE_UINTTL(env.CP0_PageMask, MIPSCPU),
>          VMSTATE_INT32(env.CP0_PageGrain, MIPSCPU),
>          VMSTATE_UINTTL(env.CP0_SegCtl0, MIPSCPU),
>          VMSTATE_UINTTL(env.CP0_SegCtl1, MIPSCPU),
> --
> 2.27.0
>
Peter Maydell June 15, 2020, 9:13 a.m. UTC | #3 
On Sun, 14 Jun 2020 at 22:52, Aleksandar Markovic
<aleksandar.qemu.devel@gmail.com> wrote:
> When you change machine.c the way you did it, you need to bump the version. Please see git log on machine.c for details.

>> --- a/target/mips/cpu.h
>> +++ b/target/mips/cpu.h
>> @@ -617,7 +617,8 @@ struct CPUMIPSState {
>>  /*
>>   * CP0 Register 5
>>   */
>> -    int32_t CP0_PageMask;
>> +    target_ulong CP0_PageMask;
>> +#define CP0PM_MASK 13

Does CP0_PageMask ever actually hold a value that won't fit
in an int32_t? If not, it might be preferable to avoid changing
its type to avoid the migration compat break, even if MIPS
doesn't have any versioned boards where we have a strict
don't-break-compat promise to users.

thanks
-- PMM
Jiaxun Yang June 15, 2020, 9:16 a.m. UTC | #4 
在 2020/6/15 17:13, Peter Maydell 写道:
> On Sun, 14 Jun 2020 at 22:52, Aleksandar Markovic
> <aleksandar.qemu.devel@gmail.com> wrote:
>> When you change machine.c the way you did it, you need to bump the version. Please see git log on machine.c for details.
> 
>>> --- a/target/mips/cpu.h
>>> +++ b/target/mips/cpu.h
>>> @@ -617,7 +617,8 @@ struct CPUMIPSState {
>>>   /*
>>>    * CP0 Register 5
>>>    */
>>> -    int32_t CP0_PageMask;
>>> +    target_ulong CP0_PageMask;
>>> +#define CP0PM_MASK 13
> 
> Does CP0_PageMask ever actually hold a value that won't fit
> in an int32_t? If not, it might be preferable to avoid changing
> its type to avoid the migration compat break, even if MIPS
> doesn't have any versioned boards where we have a strict
> don't-break-compat promise to users.

In Release2, PageMask was extended to 64bit on MIPS64 processors.

Is it necessary to follow that?

Thanks.

> 
> thanks
> -- PMM
>
Peter Maydell June 15, 2020, 9:23 a.m. UTC | #5 
On Mon, 15 Jun 2020 at 10:17, Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>
>
>
> 在 2020/6/15 17:13, Peter Maydell 写道:
> > On Sun, 14 Jun 2020 at 22:52, Aleksandar Markovic
> > <aleksandar.qemu.devel@gmail.com> wrote:
> >> When you change machine.c the way you did it, you need to bump the version. Please see git log on machine.c for details.
> >
> >>> --- a/target/mips/cpu.h
> >>> +++ b/target/mips/cpu.h
> >>> @@ -617,7 +617,8 @@ struct CPUMIPSState {
> >>>   /*
> >>>    * CP0 Register 5
> >>>    */
> >>> -    int32_t CP0_PageMask;
> >>> +    target_ulong CP0_PageMask;
> >>> +#define CP0PM_MASK 13
> >
> > Does CP0_PageMask ever actually hold a value that won't fit
> > in an int32_t? If not, it might be preferable to avoid changing
> > its type to avoid the migration compat break, even if MIPS
> > doesn't have any versioned boards where we have a strict
> > don't-break-compat promise to users.
>
> In Release2, PageMask was extended to 64bit on MIPS64 processors.
>
> Is it necessary to follow that?

Ah, I see. I'd assumed that you were only fixing the
variable-page-size change (which shouldn't require a
change in the type), but as Aleksandar says you've
mixed in a new feature implementation in the same commit
(which from what you're saying does need the type to change).
If the new feature means the register is now 64 bits then
it is possible to implement this in a migration-compatible
way by using a vmstate subsection; I'll leave it up to
Aleksandar whether that complexity is something that makes
sense for MIPS targets or if it's better to just break migration
compat by bumping the version.

thanks
-- PMM
diff mbox series

Patch

diff --git a/target/mips/cp0_helper.c b/target/mips/cp0_helper.c
index bbf12e4a97..7a134085f7 100644
--- a/target/mips/cp0_helper.c
+++ b/target/mips/cp0_helper.c
@@ -872,20 +872,44 @@  void helper_mtc0_memorymapid(CPUMIPSState *env, target_ulong arg1)
     }
 }
 
-void update_pagemask(CPUMIPSState *env, target_ulong arg1, int32_t *pagemask)
+void helper_mtc0_pagemask(CPUMIPSState *env, target_ulong arg1)
 {
-    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));
+    uint64_t mask;
+    int maxmaskbits, maskbits;
+
+    if (env->insn_flags & ISA_MIPS32R6) {
+        return;
     }
-}
 
-void helper_mtc0_pagemask(CPUMIPSState *env, target_ulong arg1)
-{
-    update_pagemask(env, arg1, &env->CP0_PageMask);
+    /* Don't care MASKX as we don't support 1KB page */
+#ifdef TARGET_MIPS64
+    if (env->CP0_Config3 & CP0C3_BPG) {
+        maxmaskbits = 47;
+    } else {
+        maxmaskbits = 16;
+    }
+#else
+    maxmaskbits = 16;
+#endif
+    mask = extract64((uint64_t)arg1, CP0PM_MASK, maxmaskbits);
+
+    maskbits = find_first_zero_bit(&mask, 64);
+
+    /* 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:
+    maskbits = MIN(maxmaskbits, MAX(maskbits, TARGET_PAGE_BITS - 12));
+    env->CP0_PageMask = ((1 << (maskbits + 1)) - 1) << CP0PM_MASK;
 }
 
 void helper_mtc0_pagegrain(CPUMIPSState *env, target_ulong arg1)
@@ -1111,7 +1135,7 @@  void helper_mthc0_saar(CPUMIPSState *env, target_ulong arg1)
 void helper_mtc0_entryhi(CPUMIPSState *env, target_ulong arg1)
 {
     target_ulong old, val, mask;
-    mask = (TARGET_PAGE_MASK << 1) | env->CP0_EntryHi_ASID_mask;
+    mask = ~((1 << 14) - 1) | env->CP0_EntryHi_ASID_mask;
     if (((env->CP0_Config4 >> CP0C4_IE) & 0x3) >= 2) {
         mask |= 1 << CP0EnHi_EHINV;
     }
diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 0b3c987bb3..b69806792d 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -617,7 +617,8 @@  struct CPUMIPSState {
 /*
  * CP0 Register 5
  */
-    int32_t CP0_PageMask;
+    target_ulong CP0_PageMask;
+#define CP0PM_MASK 13
     int32_t CP0_PageGrain_rw_bitmask;
     int32_t CP0_PageGrain;
 #define CP0PG_RIE 31
diff --git a/target/mips/machine.c b/target/mips/machine.c
index 5b23e3e912..b30a75d46b 100644
--- a/target/mips/machine.c
+++ b/target/mips/machine.c
@@ -252,7 +252,7 @@  const VMStateDescription vmstate_mips_cpu = {
         VMSTATE_UINT64(env.CP0_EntryLo1, MIPSCPU),
         VMSTATE_UINTTL(env.CP0_Context, MIPSCPU),
         VMSTATE_INT32(env.CP0_MemoryMapID, MIPSCPU),
-        VMSTATE_INT32(env.CP0_PageMask, MIPSCPU),
+        VMSTATE_UINTTL(env.CP0_PageMask, MIPSCPU),
         VMSTATE_INT32(env.CP0_PageGrain, MIPSCPU),
         VMSTATE_UINTTL(env.CP0_SegCtl0, MIPSCPU),
         VMSTATE_UINTTL(env.CP0_SegCtl1, MIPSCPU),