| Message ID | 20250328175526.368121-2-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/mips: Revert TARGET_PAGE_BITS_VARY and bug fixes | expand |
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>
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 >
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~
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~
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~
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~
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~
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); {
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(-)