[PATCH 0/3] target/mips: Revert TARGET_PAGE_BITS_VARY and bug fixes

[PATCH 0/3] target/mips: Revert TARGET_PAGE_BITS_VARY and bug fixes

[Richard Henderson]

The logic behind changing the system page size because of what the
Loongson kernel "prefers" is flawed.

In the Loongson-2E manual, section 5.5, it is clear that the cpu
supports a 4k page size (along with many others).  Therefore we
must continue to support a 4k page size.

While in the area, I noticed two other bugs related to update_pagemask.


r~


Richard Henderson (3):
  target/mips: Revert TARGET_PAGE_BITS_VARY
  target/mips: Require even maskbits in update_pagemask
  target/mips: Simplify and fix update_pagemask

 target/mips/cpu-param.h             |  5 -----
 target/mips/tcg/tcg-internal.h      |  2 +-
 hw/mips/fuloong2e.c                 |  1 -
 hw/mips/loongson3_virt.c            |  1 -
 target/mips/tcg/system/cp0_helper.c | 32 +++++++++--------------------
 target/mips/tcg/system/tlb_helper.c |  4 ++--
 6 files changed, 13 insertions(+), 32 deletions(-)

-- 
2.43.0

[PATCH 1/3] target/mips: Revert TARGET_PAGE_BITS_VARY

[Richard Henderson]

Revert ee3863b9d41 and a08d60bc6c2b.  The logic behind changing
the system page size because of what the Loongson kernel "prefers"
is flawed.

In the Loongson-2E manual, section 5.5, it is clear that the cpu
supports a 4k page size (along with many others).  Therefore we
must continue to support a 4k page size.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/mips/cpu-param.h             | 5 -----
 hw/mips/fuloong2e.c                 | 1 -
 hw/mips/loongson3_virt.c            | 1 -
 target/mips/tcg/system/cp0_helper.c | 7 +------
 target/mips/tcg/system/tlb_helper.c | 2 +-
 5 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/target/mips/cpu-param.h b/target/mips/cpu-param.h
index 11b3ac0ac6..8fcb1b4f5f 100644
--- a/target/mips/cpu-param.h
+++ b/target/mips/cpu-param.h
@@ -18,12 +18,7 @@
 #  define TARGET_VIRT_ADDR_SPACE_BITS 32
 #endif
 #endif
-#ifdef CONFIG_USER_ONLY
 #define TARGET_PAGE_BITS 12
-#else
-#define TARGET_PAGE_BITS_VARY
-#define TARGET_PAGE_BITS_MIN 12
-#endif
 
 #define TCG_GUEST_DEFAULT_MO (0)
 
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 646044e274..2a8507b8b0 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -334,7 +334,6 @@ static void mips_fuloong2e_machine_init(MachineClass *mc)
     mc->default_cpu_type = MIPS_CPU_TYPE_NAME("Loongson-2E");
     mc->default_ram_size = 256 * MiB;
     mc->default_ram_id = "fuloong2e.ram";
-    mc->minimum_page_bits = 14;
     machine_add_audiodev_property(mc);
 }
 
diff --git a/hw/mips/loongson3_virt.c b/hw/mips/loongson3_virt.c
index db1cc51314..1da20dccec 100644
--- a/hw/mips/loongson3_virt.c
+++ b/hw/mips/loongson3_virt.c
@@ -677,7 +677,6 @@ static void loongson3v_machine_class_init(ObjectClass *oc, void *data)
     mc->max_cpus = LOONGSON_MAX_VCPUS;
     mc->default_ram_id = "loongson3.highram";
     mc->default_ram_size = 1600 * MiB;
-    mc->minimum_page_bits = 14;
     mc->default_nic = "virtio-net-pci";
 }
 
diff --git a/target/mips/tcg/system/cp0_helper.c b/target/mips/tcg/system/cp0_helper.c
index 01a07a169f..8c2114c58a 100644
--- a/target/mips/tcg/system/cp0_helper.c
+++ b/target/mips/tcg/system/cp0_helper.c
@@ -877,18 +877,13 @@ void update_pagemask(CPUMIPSState *env, target_ulong arg1, int32_t *pagemask)
     if ((mask >> maskbits) != 0) {
         goto invalid;
     }
-    /* We don't support VTLB entry smaller than target page */
-    if ((maskbits + TARGET_PAGE_BITS_MIN) < TARGET_PAGE_BITS) {
-        goto invalid;
-    }
     env->CP0_PageMask = mask << CP0PM_MASK;
 
     return;
 
 invalid:
     /* When invalid, set to default target page size. */
-    mask = (~TARGET_PAGE_MASK >> TARGET_PAGE_BITS_MIN);
-    env->CP0_PageMask = mask << CP0PM_MASK;
+    env->CP0_PageMask = 0;
 }
 
 void helper_mtc0_pagemask(CPUMIPSState *env, target_ulong arg1)
diff --git a/target/mips/tcg/system/tlb_helper.c b/target/mips/tcg/system/tlb_helper.c
index ca4d6b27bc..123639fa18 100644
--- a/target/mips/tcg/system/tlb_helper.c
+++ b/target/mips/tcg/system/tlb_helper.c
@@ -875,7 +875,7 @@ refill:
             break;
         }
     }
-    pw_pagemask = m >> TARGET_PAGE_BITS_MIN;
+    pw_pagemask = m >> TARGET_PAGE_BITS;
     update_pagemask(env, pw_pagemask << CP0PM_MASK, &pw_pagemask);
     pw_entryhi = (address & ~0x1fff) | (env->CP0_EntryHi & 0xFF);
     {
-- 
2.43.0

Re: [PATCH 1/3] target/mips: Revert TARGET_PAGE_BITS_VARY

[Philippe Mathieu-Daudé]

On 28/3/25 18:55, Richard Henderson wrote:
> Revert ee3863b9d41 and a08d60bc6c2b.  The logic behind changing
> the system page size because of what the Loongson kernel "prefers"
> is flawed.
> 
> In the Loongson-2E manual, section 5.5, it is clear that the cpu
> supports a 4k page size (along with many others).  Therefore we
> must continue to support a 4k page size.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/mips/cpu-param.h             | 5 -----
>   hw/mips/fuloong2e.c                 | 1 -
>   hw/mips/loongson3_virt.c            | 1 -
>   target/mips/tcg/system/cp0_helper.c | 7 +------
>   target/mips/tcg/system/tlb_helper.c | 2 +-
>   5 files changed, 2 insertions(+), 14 deletions(-)


> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> index 646044e274..2a8507b8b0 100644
> --- a/hw/mips/fuloong2e.c
> +++ b/hw/mips/fuloong2e.c
> @@ -334,7 +334,6 @@ static void mips_fuloong2e_machine_init(MachineClass *mc)
>       mc->default_cpu_type = MIPS_CPU_TYPE_NAME("Loongson-2E");
>       mc->default_ram_size = 256 * MiB;
>       mc->default_ram_id = "fuloong2e.ram";
> -    mc->minimum_page_bits = 14;
>       machine_add_audiodev_property(mc);
>   }

fuloong2e machine uses a Loongson-2E CPU, which as the manual you
pointed out mentions, supports 4K pages.

>   
> diff --git a/hw/mips/loongson3_virt.c b/hw/mips/loongson3_virt.c
> index db1cc51314..1da20dccec 100644
> --- a/hw/mips/loongson3_virt.c
> +++ b/hw/mips/loongson3_virt.c
> @@ -677,7 +677,6 @@ static void loongson3v_machine_class_init(ObjectClass *oc, void *data)
>       mc->max_cpus = LOONGSON_MAX_VCPUS;
>       mc->default_ram_id = "loongson3.highram";
>       mc->default_ram_size = 1600 * MiB;
> -    mc->minimum_page_bits = 14;
>       mc->default_nic = "virtio-net-pci";
>   }

loongson3v machine uses Loongson-3 series CPUs with TCG, or
Loongson-3A4000 with KVM.

The Loongson-3A1000 is based on a GS464 core, which does support
4K (chapter 3.5: PageMask register).

The Loongson-3A2000 and 3A3000 are based on a GS464E core, which
also supports 4K (chapter 7.7 PageMask Register).

   Address page Mask. The address mask is used to control the size
   of the page table stored in the page table entry. GS464E supports
   4KB to 1GB page size increments of 4.

The Loongson-3A4000 is based on a GS464V core, for which I couldn't
find the full manual. Being a GS464 with vector capabilities ("The
memory functional unit of GS464V is similar with GS464") I'll assume
4K is also OK there.

(I'll amend that information to the commit description)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/3] target/mips: Revert TARGET_PAGE_BITS_VARY

[Huacai Chen]

Hi, Richard,

On Sat, Mar 29, 2025 at 1:55 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Revert ee3863b9d41 and a08d60bc6c2b.  The logic behind changing
> the system page size because of what the Loongson kernel "prefers"
> is flawed.
>
> In the Loongson-2E manual, section 5.5, it is clear that the cpu
> supports a 4k page size (along with many others).  Therefore we
> must continue to support a 4k page size.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/mips/cpu-param.h             | 5 -----
>  hw/mips/fuloong2e.c                 | 1 -
>  hw/mips/loongson3_virt.c            | 1 -
>  target/mips/tcg/system/cp0_helper.c | 7 +------
>  target/mips/tcg/system/tlb_helper.c | 2 +-
>  5 files changed, 2 insertions(+), 14 deletions(-)
>
> diff --git a/target/mips/cpu-param.h b/target/mips/cpu-param.h
> index 11b3ac0ac6..8fcb1b4f5f 100644
> --- a/target/mips/cpu-param.h
> +++ b/target/mips/cpu-param.h
> @@ -18,12 +18,7 @@
>  #  define TARGET_VIRT_ADDR_SPACE_BITS 32
>  #endif
>  #endif
> -#ifdef CONFIG_USER_ONLY
>  #define TARGET_PAGE_BITS 12
> -#else
> -#define TARGET_PAGE_BITS_VARY
> -#define TARGET_PAGE_BITS_MIN 12
> -#endif
I'm a bit confused about TARGET_PAGE_BITS and other macros.

In my opinion, if we define TARGET_PAGE_BITS as 12, that means we only
support 4K pages. And if we define TARGET_PAGE_BITS_VARY and
TARGET_PAGE_BITS_MIN as 12, that means we support the minimum page as
4K, but we also support larger pages.

Am I wrong?

>
>  #define TCG_GUEST_DEFAULT_MO (0)
>
> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> index 646044e274..2a8507b8b0 100644
> --- a/hw/mips/fuloong2e.c
> +++ b/hw/mips/fuloong2e.c
> @@ -334,7 +334,6 @@ static void mips_fuloong2e_machine_init(MachineClass *mc)
>      mc->default_cpu_type = MIPS_CPU_TYPE_NAME("Loongson-2E");
>      mc->default_ram_size = 256 * MiB;
>      mc->default_ram_id = "fuloong2e.ram";
> -    mc->minimum_page_bits = 14;
Loongson prefers 16K pages not because it doesn't support 4K, but
because 4K pages cause cache aliases, which make the kernel difficult
to implement.

Huacai

>      machine_add_audiodev_property(mc);
>  }
>
> diff --git a/hw/mips/loongson3_virt.c b/hw/mips/loongson3_virt.c
> index db1cc51314..1da20dccec 100644
> --- a/hw/mips/loongson3_virt.c
> +++ b/hw/mips/loongson3_virt.c
> @@ -677,7 +677,6 @@ static void loongson3v_machine_class_init(ObjectClass *oc, void *data)
>      mc->max_cpus = LOONGSON_MAX_VCPUS;
>      mc->default_ram_id = "loongson3.highram";
>      mc->default_ram_size = 1600 * MiB;
> -    mc->minimum_page_bits = 14;
>      mc->default_nic = "virtio-net-pci";
>  }
>
> diff --git a/target/mips/tcg/system/cp0_helper.c b/target/mips/tcg/system/cp0_helper.c
> index 01a07a169f..8c2114c58a 100644
> --- a/target/mips/tcg/system/cp0_helper.c
> +++ b/target/mips/tcg/system/cp0_helper.c
> @@ -877,18 +877,13 @@ void update_pagemask(CPUMIPSState *env, target_ulong arg1, int32_t *pagemask)
>      if ((mask >> maskbits) != 0) {
>          goto invalid;
>      }
> -    /* We don't support VTLB entry smaller than target page */
> -    if ((maskbits + TARGET_PAGE_BITS_MIN) < TARGET_PAGE_BITS) {
> -        goto invalid;
> -    }
>      env->CP0_PageMask = mask << CP0PM_MASK;
>
>      return;
>
>  invalid:
>      /* When invalid, set to default target page size. */
> -    mask = (~TARGET_PAGE_MASK >> TARGET_PAGE_BITS_MIN);
> -    env->CP0_PageMask = mask << CP0PM_MASK;
> +    env->CP0_PageMask = 0;
>  }
>
>  void helper_mtc0_pagemask(CPUMIPSState *env, target_ulong arg1)
> diff --git a/target/mips/tcg/system/tlb_helper.c b/target/mips/tcg/system/tlb_helper.c
> index ca4d6b27bc..123639fa18 100644
> --- a/target/mips/tcg/system/tlb_helper.c
> +++ b/target/mips/tcg/system/tlb_helper.c
> @@ -875,7 +875,7 @@ refill:
>              break;
>          }
>      }
> -    pw_pagemask = m >> TARGET_PAGE_BITS_MIN;
> +    pw_pagemask = m >> TARGET_PAGE_BITS;
>      update_pagemask(env, pw_pagemask << CP0PM_MASK, &pw_pagemask);
>      pw_entryhi = (address & ~0x1fff) | (env->CP0_EntryHi & 0xFF);
>      {
> --
> 2.43.0
>

Re: [PATCH 1/3] target/mips: Revert TARGET_PAGE_BITS_VARY

[Richard Henderson]

On 3/31/25 20:15, Huacai Chen wrote:
>>   #  define TARGET_VIRT_ADDR_SPACE_BITS 32
>>   #endif
>>   #endif
>> -#ifdef CONFIG_USER_ONLY
>>   #define TARGET_PAGE_BITS 12
>> -#else
>> -#define TARGET_PAGE_BITS_VARY
>> -#define TARGET_PAGE_BITS_MIN 12
>> -#endif
> I'm a bit confused about TARGET_PAGE_BITS and other macros.
> 
> In my opinion, if we define TARGET_PAGE_BITS as 12, that means we only
> support 4K pages. And if we define TARGET_PAGE_BITS_VARY and
> TARGET_PAGE_BITS_MIN as 12, that means we support the minimum page as
> 4K, but we also support larger pages.
> 
> Am I wrong?
Yes.

TARGET_PAGE_BITS is a minimum value that is used by the memory subsystem for managing ram 
and i/o.  If variable, via TARGET_PAGE_BITS_VARY, this is set very early in qemu startup 
and cannot be changed.

The page size for the mips cpu, like many others, may be changed at runtime.  The page 
size used at that point is reported to softmmu during tlb_fill.

The value of TARGET_PAGE_BITS must be the minimum supported by the cpu.

For Arm, the minimum for armv6 was 1k, then armv7 dropped support for tiny pages so the 
minimum is 4k.  At runtime, armv8 supports page sizes of 4k, 16k, and 64k.

For MIPS, ignoring those cpus which support 1k pages, the minimum is 4k.


r~

Re: [PATCH 1/3] target/mips: Revert TARGET_PAGE_BITS_VARY

[Huacai Chen]

Hi, Richard,

On Tue, Apr 1, 2025 at 9:39 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/31/25 20:15, Huacai Chen wrote:
> >>   #  define TARGET_VIRT_ADDR_SPACE_BITS 32
> >>   #endif
> >>   #endif
> >> -#ifdef CONFIG_USER_ONLY
> >>   #define TARGET_PAGE_BITS 12
> >> -#else
> >> -#define TARGET_PAGE_BITS_VARY
> >> -#define TARGET_PAGE_BITS_MIN 12
> >> -#endif
> > I'm a bit confused about TARGET_PAGE_BITS and other macros.
> >
> > In my opinion, if we define TARGET_PAGE_BITS as 12, that means we only
> > support 4K pages. And if we define TARGET_PAGE_BITS_VARY and
> > TARGET_PAGE_BITS_MIN as 12, that means we support the minimum page as
> > 4K, but we also support larger pages.
> >
> > Am I wrong?
> Yes.
>
> TARGET_PAGE_BITS is a minimum value that is used by the memory subsystem for managing ram
> and i/o.  If variable, via TARGET_PAGE_BITS_VARY, this is set very early in qemu startup
> and cannot be changed.
>
> The page size for the mips cpu, like many others, may be changed at runtime.  The page
> size used at that point is reported to softmmu during tlb_fill.
>
> The value of TARGET_PAGE_BITS must be the minimum supported by the cpu.
>
> For Arm, the minimum for armv6 was 1k, then armv7 dropped support for tiny pages so the
> minimum is 4k.  At runtime, armv8 supports page sizes of 4k, 16k, and 64k.
>
> For MIPS, ignoring those cpus which support 1k pages, the minimum is 4k.
If all types of cpus of the target arch has the same minimum supported
page size, we only need to define TARGET_PAGE_BITS; otherwise we need
to define TARGET_PAGE_BITS_VARY, and TARGET_PAGE_BITS_MIN means the
minimum supported page size of the smallest supported page's cpu type.
Here we remove TARGET_PAGE_BITS_VARY because we just ignore the 1K
pages.

Am I right now?

Huacai

>
>
> r~

Re: [PATCH 1/3] target/mips: Revert TARGET_PAGE_BITS_VARY

[Richard Henderson]

On 4/1/25 20:04, Huacai Chen wrote:
> Hi, Richard,
> 
> On Tue, Apr 1, 2025 at 9:39 PM Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 3/31/25 20:15, Huacai Chen wrote:
>>>>    #  define TARGET_VIRT_ADDR_SPACE_BITS 32
>>>>    #endif
>>>>    #endif
>>>> -#ifdef CONFIG_USER_ONLY
>>>>    #define TARGET_PAGE_BITS 12
>>>> -#else
>>>> -#define TARGET_PAGE_BITS_VARY
>>>> -#define TARGET_PAGE_BITS_MIN 12
>>>> -#endif
>>> I'm a bit confused about TARGET_PAGE_BITS and other macros.
>>>
>>> In my opinion, if we define TARGET_PAGE_BITS as 12, that means we only
>>> support 4K pages. And if we define TARGET_PAGE_BITS_VARY and
>>> TARGET_PAGE_BITS_MIN as 12, that means we support the minimum page as
>>> 4K, but we also support larger pages.
>>>
>>> Am I wrong?
>> Yes.
>>
>> TARGET_PAGE_BITS is a minimum value that is used by the memory subsystem for managing ram
>> and i/o.  If variable, via TARGET_PAGE_BITS_VARY, this is set very early in qemu startup
>> and cannot be changed.
>>
>> The page size for the mips cpu, like many others, may be changed at runtime.  The page
>> size used at that point is reported to softmmu during tlb_fill.
>>
>> The value of TARGET_PAGE_BITS must be the minimum supported by the cpu.
>>
>> For Arm, the minimum for armv6 was 1k, then armv7 dropped support for tiny pages so the
>> minimum is 4k.  At runtime, armv8 supports page sizes of 4k, 16k, and 64k.
>>
>> For MIPS, ignoring those cpus which support 1k pages, the minimum is 4k.
> If all types of cpus of the target arch has the same minimum supported
> page size, we only need to define TARGET_PAGE_BITS; otherwise we need
> to define TARGET_PAGE_BITS_VARY, and TARGET_PAGE_BITS_MIN means the
> minimum supported page size of the smallest supported page's cpu type.
> Here we remove TARGET_PAGE_BITS_VARY because we just ignore the 1K
> pages.
> 
> Am I right now?

Yes.


r~

Re: [PATCH 1/3] target/mips: Revert TARGET_PAGE_BITS_VARY

[Huacai Chen]

On Thu, Apr 3, 2025 at 2:11 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 4/1/25 20:04, Huacai Chen wrote:
> > Hi, Richard,
> >
> > On Tue, Apr 1, 2025 at 9:39 PM Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> On 3/31/25 20:15, Huacai Chen wrote:
> >>>>    #  define TARGET_VIRT_ADDR_SPACE_BITS 32
> >>>>    #endif
> >>>>    #endif
> >>>> -#ifdef CONFIG_USER_ONLY
> >>>>    #define TARGET_PAGE_BITS 12
> >>>> -#else
> >>>> -#define TARGET_PAGE_BITS_VARY
> >>>> -#define TARGET_PAGE_BITS_MIN 12
> >>>> -#endif
> >>> I'm a bit confused about TARGET_PAGE_BITS and other macros.
> >>>
> >>> In my opinion, if we define TARGET_PAGE_BITS as 12, that means we only
> >>> support 4K pages. And if we define TARGET_PAGE_BITS_VARY and
> >>> TARGET_PAGE_BITS_MIN as 12, that means we support the minimum page as
> >>> 4K, but we also support larger pages.
> >>>
> >>> Am I wrong?
> >> Yes.
> >>
> >> TARGET_PAGE_BITS is a minimum value that is used by the memory subsystem for managing ram
> >> and i/o.  If variable, via TARGET_PAGE_BITS_VARY, this is set very early in qemu startup
> >> and cannot be changed.
> >>
> >> The page size for the mips cpu, like many others, may be changed at runtime.  The page
> >> size used at that point is reported to softmmu during tlb_fill.
> >>
> >> The value of TARGET_PAGE_BITS must be the minimum supported by the cpu.
> >>
> >> For Arm, the minimum for armv6 was 1k, then armv7 dropped support for tiny pages so the
> >> minimum is 4k.  At runtime, armv8 supports page sizes of 4k, 16k, and 64k.
> >>
> >> For MIPS, ignoring those cpus which support 1k pages, the minimum is 4k.
> > If all types of cpus of the target arch has the same minimum supported
> > page size, we only need to define TARGET_PAGE_BITS; otherwise we need
> > to define TARGET_PAGE_BITS_VARY, and TARGET_PAGE_BITS_MIN means the
> > minimum supported page size of the smallest supported page's cpu type.
> > Here we remove TARGET_PAGE_BITS_VARY because we just ignore the 1K
> > pages.
> >
> > Am I right now?
>
> Yes.
OK, then it is fine to remove the TARGET_PAGE_BITS_VARY and
TARGET_PAGE_BITS_MIN definition. But Loongson still prefers 16K pages
(4K pages cause cache alias on Loongson), so I want to keep
mc->minimum_page_bits = 14.

Huacai

>
>
> r~

Re: [PATCH 1/3] target/mips: Revert TARGET_PAGE_BITS_VARY

[Richard Henderson]

On 4/2/25 20:05, Huacai Chen wrote:
> On Thu, Apr 3, 2025 at 2:11 AM Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 4/1/25 20:04, Huacai Chen wrote:
>>> Hi, Richard,
>>>
>>> On Tue, Apr 1, 2025 at 9:39 PM Richard Henderson
>>> <richard.henderson@linaro.org> wrote:
>>>>
>>>> On 3/31/25 20:15, Huacai Chen wrote:
>>>>>>     #  define TARGET_VIRT_ADDR_SPACE_BITS 32
>>>>>>     #endif
>>>>>>     #endif
>>>>>> -#ifdef CONFIG_USER_ONLY
>>>>>>     #define TARGET_PAGE_BITS 12
>>>>>> -#else
>>>>>> -#define TARGET_PAGE_BITS_VARY
>>>>>> -#define TARGET_PAGE_BITS_MIN 12
>>>>>> -#endif
>>>>> I'm a bit confused about TARGET_PAGE_BITS and other macros.
>>>>>
>>>>> In my opinion, if we define TARGET_PAGE_BITS as 12, that means we only
>>>>> support 4K pages. And if we define TARGET_PAGE_BITS_VARY and
>>>>> TARGET_PAGE_BITS_MIN as 12, that means we support the minimum page as
>>>>> 4K, but we also support larger pages.
>>>>>
>>>>> Am I wrong?
>>>> Yes.
>>>>
>>>> TARGET_PAGE_BITS is a minimum value that is used by the memory subsystem for managing ram
>>>> and i/o.  If variable, via TARGET_PAGE_BITS_VARY, this is set very early in qemu startup
>>>> and cannot be changed.
>>>>
>>>> The page size for the mips cpu, like many others, may be changed at runtime.  The page
>>>> size used at that point is reported to softmmu during tlb_fill.
>>>>
>>>> The value of TARGET_PAGE_BITS must be the minimum supported by the cpu.
>>>>
>>>> For Arm, the minimum for armv6 was 1k, then armv7 dropped support for tiny pages so the
>>>> minimum is 4k.  At runtime, armv8 supports page sizes of 4k, 16k, and 64k.
>>>>
>>>> For MIPS, ignoring those cpus which support 1k pages, the minimum is 4k.
>>> If all types of cpus of the target arch has the same minimum supported
>>> page size, we only need to define TARGET_PAGE_BITS; otherwise we need
>>> to define TARGET_PAGE_BITS_VARY, and TARGET_PAGE_BITS_MIN means the
>>> minimum supported page size of the smallest supported page's cpu type.
>>> Here we remove TARGET_PAGE_BITS_VARY because we just ignore the 1K
>>> pages.
>>>
>>> Am I right now?
>>
>> Yes.
> OK, then it is fine to remove the TARGET_PAGE_BITS_VARY and
> TARGET_PAGE_BITS_MIN definition. But Loongson still prefers 16K pages
> (4K pages cause cache alias on Loongson), so I want to keep
> mc->minimum_page_bits = 14.

You can't set minimum_page_size = 14, because TARGET_PAGE_BITS_VARY is unset.

However, the Loongson kernel will continue to correctly program 16k pages at runtime, and 
we will continue to correctly provide 16k pages to softmmu during tlb_fill.  All is well.


r~

[PATCH 2/3] target/mips: Require even maskbits in update_pagemask

[Richard Henderson]

The number of bits set in PageMask must be even.

Fixes: d40b55bc1b86 ("target/mips: Fix PageMask with variable page size")
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/mips/tcg/system/cp0_helper.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/target/mips/tcg/system/cp0_helper.c b/target/mips/tcg/system/cp0_helper.c
index 8c2114c58a..5db8166d45 100644
--- a/target/mips/tcg/system/cp0_helper.c
+++ b/target/mips/tcg/system/cp0_helper.c
@@ -866,24 +866,17 @@ void helper_mtc0_memorymapid(CPUMIPSState *env, target_ulong arg1)
 
 void update_pagemask(CPUMIPSState *env, target_ulong arg1, int32_t *pagemask)
 {
-    uint32_t mask;
-    int maskbits;
-
     /* Don't care MASKX as we don't support 1KB page */
-    mask = extract32((uint32_t)arg1, CP0PM_MASK, 16);
-    maskbits = cto32(mask);
+    uint32_t mask = extract32((uint32_t)arg1, CP0PM_MASK, 16);
+    int maskbits = cto32(mask);
 
-    /* Ensure no more set bit after first zero */
-    if ((mask >> maskbits) != 0) {
-        goto invalid;
+    /* Ensure no more set bit after first zero, and maskbits even. */
+    if ((mask >> maskbits) == 0 && maskbits % 2 == 0) {
+        env->CP0_PageMask = mask << CP0PM_MASK;
+    } else {
+        /* When invalid, set to default target page size. */
+        env->CP0_PageMask = 0;
     }
-    env->CP0_PageMask = mask << CP0PM_MASK;
-
-    return;
-
-invalid:
-    /* When invalid, set to default target page size. */
-    env->CP0_PageMask = 0;
 }
 
 void helper_mtc0_pagemask(CPUMIPSState *env, target_ulong arg1)
-- 
2.43.0

Re: [PATCH 2/3] target/mips: Require even maskbits in update_pagemask

[Philippe Mathieu-Daudé]

On 28/3/25 18:55, Richard Henderson wrote:
> The number of bits set in PageMask must be even.
> 
> Fixes: d40b55bc1b86 ("target/mips: Fix PageMask with variable page size")
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/mips/tcg/system/cp0_helper.c | 23 ++++++++---------------
>   1 file changed, 8 insertions(+), 15 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 3/3] target/mips: Simplify and fix update_pagemask

[Richard Henderson]

When update_pagemask was split from helper_mtc0_pagemask,
we failed to actually write to the new parameter but continue
to write to env->CP0_PageMask.  Thus the use within
page_table_walk_refill modifies cpu state and not the local
variable as expected.

Simplify by renaming to compute_pagemask and returning the
value directly.  No need for either env or pointer return.

Fixes: 074cfcb4dae ("target/mips: Implement hardware page table walker for MIPS32")
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/mips/tcg/tcg-internal.h      |  2 +-
 target/mips/tcg/system/cp0_helper.c | 10 +++++-----
 target/mips/tcg/system/tlb_helper.c |  2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/target/mips/tcg/tcg-internal.h b/target/mips/tcg/tcg-internal.h
index 74fc1309a7..950e6afc3f 100644
--- a/target/mips/tcg/tcg-internal.h
+++ b/target/mips/tcg/tcg-internal.h
@@ -47,7 +47,7 @@ bool mips_cpu_exec_interrupt(CPUState *cpu, int int_req);
 
 void mmu_init(CPUMIPSState *env, const mips_def_t *def);
 
-void update_pagemask(CPUMIPSState *env, target_ulong arg1, int32_t *pagemask);
+uint32_t compute_pagemask(uint32_t val);
 
 void r4k_invalidate_tlb(CPUMIPSState *env, int idx, int use_extra);
 uint32_t cpu_mips_get_random(CPUMIPSState *env);
diff --git a/target/mips/tcg/system/cp0_helper.c b/target/mips/tcg/system/cp0_helper.c
index 5db8166d45..78e422b0ca 100644
--- a/target/mips/tcg/system/cp0_helper.c
+++ b/target/mips/tcg/system/cp0_helper.c
@@ -864,24 +864,24 @@ void helper_mtc0_memorymapid(CPUMIPSState *env, target_ulong arg1)
     }
 }
 
-void update_pagemask(CPUMIPSState *env, target_ulong arg1, int32_t *pagemask)
+uint32_t compute_pagemask(uint32_t val)
 {
     /* Don't care MASKX as we don't support 1KB page */
-    uint32_t mask = extract32((uint32_t)arg1, CP0PM_MASK, 16);
+    uint32_t mask = extract32(val, CP0PM_MASK, 16);
     int maskbits = cto32(mask);
 
     /* Ensure no more set bit after first zero, and maskbits even. */
     if ((mask >> maskbits) == 0 && maskbits % 2 == 0) {
-        env->CP0_PageMask = mask << CP0PM_MASK;
+        return mask << CP0PM_MASK;
     } else {
         /* When invalid, set to default target page size. */
-        env->CP0_PageMask = 0;
+        return 0;
     }
 }
 
 void helper_mtc0_pagemask(CPUMIPSState *env, target_ulong arg1)
 {
-    update_pagemask(env, arg1, &env->CP0_PageMask);
+    env->CP0_PageMask = compute_pagemask(arg1);
 }
 
 void helper_mtc0_pagegrain(CPUMIPSState *env, target_ulong arg1)
diff --git a/target/mips/tcg/system/tlb_helper.c b/target/mips/tcg/system/tlb_helper.c
index 123639fa18..df80301a41 100644
--- a/target/mips/tcg/system/tlb_helper.c
+++ b/target/mips/tcg/system/tlb_helper.c
@@ -876,7 +876,7 @@ refill:
         }
     }
     pw_pagemask = m >> TARGET_PAGE_BITS;
-    update_pagemask(env, pw_pagemask << CP0PM_MASK, &pw_pagemask);
+    pw_pagemask = compute_pagemask(pw_pagemask << CP0PM_MASK);
     pw_entryhi = (address & ~0x1fff) | (env->CP0_EntryHi & 0xFF);
     {
         target_ulong tmp_entryhi = env->CP0_EntryHi;
-- 
2.43.0

Re: [PATCH 3/3] target/mips: Simplify and fix update_pagemask

[Philippe Mathieu-Daudé]

On 28/3/25 18:55, Richard Henderson wrote:
> When update_pagemask was split from helper_mtc0_pagemask,
> we failed to actually write to the new parameter but continue
> to write to env->CP0_PageMask.  Thus the use within
> page_table_walk_refill modifies cpu state and not the local
> variable as expected.
> 
> Simplify by renaming to compute_pagemask and returning the
> value directly.  No need for either env or pointer return.
> 
> Fixes: 074cfcb4dae ("target/mips: Implement hardware page table walker for MIPS32")
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/mips/tcg/tcg-internal.h      |  2 +-
>   target/mips/tcg/system/cp0_helper.c | 10 +++++-----
>   target/mips/tcg/system/tlb_helper.c |  2 +-
>   3 files changed, 7 insertions(+), 7 deletions(-)


>   void helper_mtc0_pagegrain(CPUMIPSState *env, target_ulong arg1)
> diff --git a/target/mips/tcg/system/tlb_helper.c b/target/mips/tcg/system/tlb_helper.c
> index 123639fa18..df80301a41 100644
> --- a/target/mips/tcg/system/tlb_helper.c
> +++ b/target/mips/tcg/system/tlb_helper.c
> @@ -876,7 +876,7 @@ refill:
>           }
>       }
>       pw_pagemask = m >> TARGET_PAGE_BITS;
> -    update_pagemask(env, pw_pagemask << CP0PM_MASK, &pw_pagemask);
> +    pw_pagemask = compute_pagemask(pw_pagemask << CP0PM_MASK);

Nice catch.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 0/3] target/mips: Revert TARGET_PAGE_BITS_VARY and bug fixes

[Philippe Mathieu-Daudé]

On 28/3/25 18:55, Richard Henderson wrote:

> Richard Henderson (3):
>    target/mips: Revert TARGET_PAGE_BITS_VARY
>    target/mips: Require even maskbits in update_pagemask
>    target/mips: Simplify and fix update_pagemask

Series queued, thanks!