Re: [PATCH v2 07/23] mips: update_mmu_cache() can replace __update_tlb()
On Wed, Jun 14, 2023 at 04:17:58PM -0700, Nathan Chancellor wrote:
> Hi Hugh,
>
> On Thu, Jun 08, 2023 at 12:17:24PM -0700, Hugh Dickins wrote:
> > Don't make update_mmu_cache() a wrapper around __update_tlb(): call it
> > directly, and use the ptep (or pmdp) provided by the caller, instead of
> > re-calling pte_offset_map() - which would raise a question of whether a
> > pte_unmap() is needed to balance it.
> >
> > Check whether the "ptep" provided by the caller is actually the pmdp,
> > instead of testing pmd_huge(): or test pmd_huge() too and warn if it
> > disagrees? This is "hazardous" territory: needs review and testing.
> >
> > Signed-off-by: Hugh Dickins
> > ---
> > arch/mips/include/asm/pgtable.h | 15 +++
> > arch/mips/mm/tlb-r3k.c | 5 +++--
> > arch/mips/mm/tlb-r4k.c | 9 +++--
> > 3 files changed, 9 insertions(+), 20 deletions(-)
> >
>
> I just bisected a crash while powering down a MIPS machine in QEMU to
> this change as commit 8044511d3893 ("mips: update_mmu_cache() can
> replace __update_tlb()") in linux-next. Unfortunately, I can still
> reproduce it with the existing fix you have for this change on the
> mailing list, which is present in next-20230614.
>
> I can reproduce it with the GCC 13.1.0 on kernel.org [1].
>
> $ make -skj"$(nproc)" ARCH=mips CROSS_COMPILE=mips-linux- mrproper
> malta_defconfig vmlinux
>
> $ qemu-system-mipsel \
> -display none \
> -nodefaults \
> -cpu 24Kf \
> -machine malta \
> -kernel vmlinux \
> -initrd rootfs.cpio \
> -m 512m \
> -serial mon:stdio
> ...
> Linux version 6.4.0-rc6-next-20230614 (nathan@dev-arch.thelio-3990X)
> (mips-linux-gcc (GCC) 13.1.0, GNU ld (GNU Binutils) 2.40) #1 SMP Wed Jun 14
> 16:13:02 MST 2023
> ...
> Run /init as init process
> process '/bin/busybox' started with executable stack
> do_page_fault(): sending SIGSEGV to init for invalid read access from
> 003c
> epc = 77b893dc in ld-uClibc-1.0.39.so[77b84000+8000]
> ra = 77b8930c in ld-uClibc-1.0.39.so[77b84000+8000]
> Kernel panic - not syncing: Attempted to kill init! exitcode=0x000b
> ---[ end Kernel panic - not syncing: Attempted to kill init!
> exitcode=0x000b ]---
>
> The rootfs is available at [2] if it is needed. I am more than happy to
> provide additional information or test patches if necessary.
>
> [1]: https://mirrors.edge.kernel.org/pub/tools/crosstool/
> [2]:
> https://github.com/ClangBuiltLinux/boot-utils/releases/download/20230609-194440/mipsel-rootfs.cpio.zst
Seeing this on real h/w as well (just to confirm).
Linux version 6.4.0-rc4-00437-g4bab5c42a698 (r...@yuzhao.bld.corp.google.com)
(mips64el-linux-gnuabi64-gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils
for Debian) 2.40) #3 SMP PREEMPT Thu Jun 15 01:05:20 MDT 2023
Skipping L2 locking due to reduced L2 cache size
CVMSEG size: 2 cache lines (256 bytes)
printk: bootconsole [early0] enabled
CPU0 revision is: 000d9602 (Cavium Octeon III)
FPU revision is: 00739600
Kernel sections are not in the memory maps
Wasting 243712 bytes for tracking 4352 unused pages
Initrd not found or empty - disabling initrd
Using passed Device Tree.
software IO TLB: SWIOTLB bounce buffer size adjusted to 0MB
software IO TLB: area num 1.
software IO TLB: mapped [mem 0x0370d000-0x0374d000] (0MB)
Primary instruction cache 78kB, virtually tagged, 39 way, 16 sets, linesize 128
bytes.
Primary data cache 32kB, 32-way, 8 sets, linesize 128 bytes.
Zone ranges:
DMA32[mem 0x0110-0xefff]
Normal empty
Movable zone start for each node
Early memory node ranges
node 0: [mem 0x0110-0x03646fff]
node 0: [mem 0x0370-0x0faf]
node 0: [mem 0x2000-0x4ebf]
Initmem setup node 0 [mem 0x0110-0x4ebf]
On node 0, zone DMA32: 4352 pages in unavailable ranges
On node 0, zone DMA32: 185 pages in unavailable ranges
On node 0, zone DMA32: 1280 pages in unavailable ranges
On node 0, zone DMA32: 5120 pages in unavailable ranges
percpu: Embedded 15 pages/cpu s24368 r8192 d28880 u61440
pcpu-alloc: s24368 r8192 d28880 u61440 alloc=15*4096
pcpu-alloc: [0] 0 [0] 1 [0] 2 [0] 3
Kernel command line: loglevel=8 console=ttyS0,115200
printk: log_buf_len individual max cpu contribution: 4096 bytes
printk: log_buf_len total cpu_extra contributions: 12288 bytes
printk: log_buf_len min size: 16384 bytes
printk: log_buf_len: 32768 bytes
printk: early log buf free: 14184(86%)
Dentry cache hash table entries: 131072 (order: 8, 1048576 bytes, linear)
Inode-cache hash table entries: 65536 (order: 7, 524288 bytes, linear)
Built 1 zonelists, mobility grouping on. Total pages: 247772
mem auto-init: stack:all(zero), heap alloc:off, heap free:off
Memory: 950032K/1004828K available (8058K kernel code, 575K rwdata, 1880K
rodata, 27488K init, 158K bss, 54796K reserved, 0K cma-reserved)
rcu: Preemptible
Re: [PATCH v2 07/23] mips: update_mmu_cache() can replace __update_tlb()
On Thu, 15 Jun 2023, Nathan Chancellor wrote: > On Wed, Jun 14, 2023 at 10:43:30PM -0700, Hugh Dickins wrote: > > > > I do hope that you find the first fixes the breakage; but if not, then > > I hate to be the bearer of bad news but the first patch did not fix the > breakage, I see the same issue. Boo! > > > I even more fervently hope that the second will, despite my hating it. > > Touch wood for the first, fingers crossed for the second, thanks, > > Thankfully, the second one does. Thanks for the quick and thoughtful > responses! Hurrah! Thanks a lot, Nathan. I'll set aside my disappointment and curiosity, clearly I'm not going to have much of a future as a MIPS programmer. I must take a break, then rush Andrew the second patch, well, not exactly that second patch, since most of that is revert: I'll just send the few lines of replacement patch (with a new Subject line, as update_mmu_cache() goes back to being separate from __update_tlb()). Unless you object, I'll include a Tested-by: you. I realize that your testing is limited to seeing it running; but that's true of most of the testing at this stage - it gets to be more interesting when the patch that adds the rcu_read_lock() and rcu_read_unlock() is added on top later. Thanks again, Hugh
Re: [PATCH v2 07/23] mips: update_mmu_cache() can replace __update_tlb()
On Wed, Jun 14, 2023 at 10:43:30PM -0700, Hugh Dickins wrote:
> On Wed, 14 Jun 2023, Hugh Dickins wrote:
> > On Wed, 14 Jun 2023, Nathan Chancellor wrote:
> > >
> > > I just bisected a crash while powering down a MIPS machine in QEMU to
> > > this change as commit 8044511d3893 ("mips: update_mmu_cache() can
> > > replace __update_tlb()") in linux-next.
> >
> > Thank you, Nathan, that's very helpful indeed. This patch certainly knew
> > that it wanted testing, and I'm glad to hear that it is now seeing some.
> >
> > While powering down? The messages below look like it was just coming up,
> > but no doubt that's because you were bisecting (or because I'm unfamiliar
> > with what messages to expect there). It's probably irrelevant information,
> > but I wonder whether the (V)machine worked well enough for a while before
> > you first powered down and spotted the problem, or whether it's never got
> > much further than trying to run init (busybox)? I'm trying to get a feel
> > for whether the problem occurs under common or uncommon conditions.
Ugh sorry, I have been looking into too many bugs lately and got my
wires crossed :) this is indeed a problem when running init (which is
busybox, this is a simple Buildroot file system).
> > > Unfortunately, I can still
> > > reproduce it with the existing fix you have for this change on the
> > > mailing list, which is present in next-20230614.
> >
> > Right, that later fix was only for a build warning, nothing functional
> > (or at least I hoped that it wasn't making any functional difference).
> >
> > Thanks a lot for the detailed instructions below: unfortunately, those
> > would draw me into a realm of testing I've never needed to enter before,
> > so a lot of time spent on setup and learning. Usually, I just stare at
> > the source.
> >
> > What this probably says is that I should revert most my cleanup there,
> > and keep as close to the existing code as possible. But some change is
> > needed, and I may need to understand (or have a good guess at) what was
> > going wrong, to decide what kind of retreat will be successful.
> >
> > Back to the source for a while: I hope I'll find examples in nearby MIPS
> > kernel source (and git history), which will hint at the right way forward.
> > Then send you a patch against next-20230614 to try, when I'm reasonably
> > confident that it's enough to satisfy my purpose, but likely not to waste
> > your time.
>
> I'm going to take advantage of your good nature by attaching
> two alternative patches, either to go on top of next-20230614.
>
> mips1.patch,
> arch/mips/mm/tlb-r4k.c | 12 +---
> 1 file changed, 1 insertion(+), 11 deletions(-)
>
> is by far my favourite. I couldn't see anything wrong with what's
> already there for mips, but it seems possible that (though I didn't
> find it) somewhere calls update_mmu_cache_pmd() on a page table. So
> mips1.patch restores the pmd_huge() check, and cleans up further by
> removing the silly pgdp, p4dp, pudp, pmdp stuff: the pointer has now
> been passed in by the caller, why walk the tree again? I should have
> done it this way before.
>
> But if that doesn't work, then I'm afraid it will have to be
> mips2.patch,
> arch/mips/include/asm/pgtable.h | 15 ---
> arch/mips/mm/tlb-r3k.c |5 ++---
> arch/mips/mm/tlb-r4k.c | 27 ++-
> 3 files changed, 32 insertions(+), 15 deletions(-)
>
> which reverts all of the original patch and its build warning fix,
> and does a pte_unmap() to balance the silly pte_offset_map() there;
> with an apologetic comment for this being about the only place in
> the tree where I have no idea what to do if ptep were NULL.
>
> I do hope that you find the first fixes the breakage; but if not, then
I hate to be the bearer of bad news but the first patch did not fix the
breakage, I see the same issue.
> I even more fervently hope that the second will, despite my hating it.
> Touch wood for the first, fingers crossed for the second, thanks,
Thankfully, the second one does. Thanks for the quick and thoughtful
responses!
Cheers,
Nathan
Re: [PATCH v2 07/23] mips: update_mmu_cache() can replace __update_tlb()
On Wed, 14 Jun 2023, Hugh Dickins wrote:
> On Wed, 14 Jun 2023, Nathan Chancellor wrote:
> >
> > I just bisected a crash while powering down a MIPS machine in QEMU to
> > this change as commit 8044511d3893 ("mips: update_mmu_cache() can
> > replace __update_tlb()") in linux-next.
>
> Thank you, Nathan, that's very helpful indeed. This patch certainly knew
> that it wanted testing, and I'm glad to hear that it is now seeing some.
>
> While powering down? The messages below look like it was just coming up,
> but no doubt that's because you were bisecting (or because I'm unfamiliar
> with what messages to expect there). It's probably irrelevant information,
> but I wonder whether the (V)machine worked well enough for a while before
> you first powered down and spotted the problem, or whether it's never got
> much further than trying to run init (busybox)? I'm trying to get a feel
> for whether the problem occurs under common or uncommon conditions.
>
> > Unfortunately, I can still
> > reproduce it with the existing fix you have for this change on the
> > mailing list, which is present in next-20230614.
>
> Right, that later fix was only for a build warning, nothing functional
> (or at least I hoped that it wasn't making any functional difference).
>
> Thanks a lot for the detailed instructions below: unfortunately, those
> would draw me into a realm of testing I've never needed to enter before,
> so a lot of time spent on setup and learning. Usually, I just stare at
> the source.
>
> What this probably says is that I should revert most my cleanup there,
> and keep as close to the existing code as possible. But some change is
> needed, and I may need to understand (or have a good guess at) what was
> going wrong, to decide what kind of retreat will be successful.
>
> Back to the source for a while: I hope I'll find examples in nearby MIPS
> kernel source (and git history), which will hint at the right way forward.
> Then send you a patch against next-20230614 to try, when I'm reasonably
> confident that it's enough to satisfy my purpose, but likely not to waste
> your time.
I'm going to take advantage of your good nature by attaching
two alternative patches, either to go on top of next-20230614.
mips1.patch,
arch/mips/mm/tlb-r4k.c | 12 +---
1 file changed, 1 insertion(+), 11 deletions(-)
is by far my favourite. I couldn't see anything wrong with what's
already there for mips, but it seems possible that (though I didn't
find it) somewhere calls update_mmu_cache_pmd() on a page table. So
mips1.patch restores the pmd_huge() check, and cleans up further by
removing the silly pgdp, p4dp, pudp, pmdp stuff: the pointer has now
been passed in by the caller, why walk the tree again? I should have
done it this way before.
But if that doesn't work, then I'm afraid it will have to be
mips2.patch,
arch/mips/include/asm/pgtable.h | 15 ---
arch/mips/mm/tlb-r3k.c |5 ++---
arch/mips/mm/tlb-r4k.c | 27 ++-
3 files changed, 32 insertions(+), 15 deletions(-)
which reverts all of the original patch and its build warning fix,
and does a pte_unmap() to balance the silly pte_offset_map() there;
with an apologetic comment for this being about the only place in
the tree where I have no idea what to do if ptep were NULL.
I do hope that you find the first fixes the breakage; but if not, then
I even more fervently hope that the second will, despite my hating it.
Touch wood for the first, fingers crossed for the second, thanks,
Hugh--- a/arch/mips/mm/tlb-r4k.c
+++ b/arch/mips/mm/tlb-r4k.c
@@ -293,12 +293,6 @@ void local_flush_tlb_one(unsigned long page)
void update_mmu_cache(struct vm_area_struct *vma,
unsigned long address, pte_t *ptep)
{
-#ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
- pgd_t *pgdp;
- p4d_t *p4dp;
- pud_t *pudp;
- pmd_t *pmdp;
-#endif
unsigned long flags;
int idx, pid;
@@ -323,12 +317,8 @@ void update_mmu_cache(struct vm_area_struct *vma,
tlb_probe_hazard();
idx = read_c0_index();
#ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
- pgdp = pgd_offset(vma->vm_mm, address);
- p4dp = p4d_offset(pgdp, address);
- pudp = pud_offset(p4dp, address);
- pmdp = pmd_offset(pudp, address);
/* this could be a huge page */
- if (ptep == (pte_t *)pmdp) {
+ if (pmd_huge(*(pmd_t *)ptep)) {
unsigned long lo;
write_c0_pagemask(PM_HUGE_MASK);
lo = pte_to_entrylo(pte_val(*ptep));
--- a/arch/mips/include/asm/pgtable.h
+++ b/arch/mips/include/asm/pgtable.h
@@ -565,8 +565,15 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
}
#endif
-extern void update_mmu_cache(struct vm_area_struct *vma,
- unsigned long address, pte_t *ptep);
+extern void __update_tlb(struct vm_area_struct *vma, unsigned long address,
+ pte_t pte);
+
+static inline void update_mmu_cache(struct vm_area_struct *vma,
+ unsigned long address, pte_t *ptep)
+{
+ pte_t pte = *ptep;
+ __update_tlb(vma, address, pte);
+}
#define __HAVE_ARCH_UPDATE_MMU_TLB
Re: [PATCH v2 07/23] mips: update_mmu_cache() can replace __update_tlb()
On Wed, 14 Jun 2023, Nathan Chancellor wrote:
> Hi Hugh,
>
> On Thu, Jun 08, 2023 at 12:17:24PM -0700, Hugh Dickins wrote:
> > Don't make update_mmu_cache() a wrapper around __update_tlb(): call it
> > directly, and use the ptep (or pmdp) provided by the caller, instead of
> > re-calling pte_offset_map() - which would raise a question of whether a
> > pte_unmap() is needed to balance it.
> >
> > Check whether the "ptep" provided by the caller is actually the pmdp,
> > instead of testing pmd_huge(): or test pmd_huge() too and warn if it
> > disagrees? This is "hazardous" territory: needs review and testing.
> >
> > Signed-off-by: Hugh Dickins
> > ---
> > arch/mips/include/asm/pgtable.h | 15 +++
> > arch/mips/mm/tlb-r3k.c | 5 +++--
> > arch/mips/mm/tlb-r4k.c | 9 +++--
> > 3 files changed, 9 insertions(+), 20 deletions(-)
> >
> > diff --git a/arch/mips/include/asm/pgtable.h
> > b/arch/mips/include/asm/pgtable.h
> > index 574fa14ac8b2..9175dfab08d5 100644
> > --- a/arch/mips/include/asm/pgtable.h
> > +++ b/arch/mips/include/asm/pgtable.h
> > @@ -565,15 +565,8 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> > }
> > #endif
> >
> > -extern void __update_tlb(struct vm_area_struct *vma, unsigned long address,
> > - pte_t pte);
> > -
> > -static inline void update_mmu_cache(struct vm_area_struct *vma,
> > - unsigned long address, pte_t *ptep)
> > -{
> > - pte_t pte = *ptep;
> > - __update_tlb(vma, address, pte);
> > -}
> > +extern void update_mmu_cache(struct vm_area_struct *vma,
> > + unsigned long address, pte_t *ptep);
> >
> > #define__HAVE_ARCH_UPDATE_MMU_TLB
> > #define update_mmu_tlb update_mmu_cache
> > @@ -581,9 +574,7 @@ static inline void update_mmu_cache(struct
> > vm_area_struct *vma,
> > static inline void update_mmu_cache_pmd(struct vm_area_struct *vma,
> > unsigned long address, pmd_t *pmdp)
> > {
> > - pte_t pte = *(pte_t *)pmdp;
> > -
> > - __update_tlb(vma, address, pte);
> > + update_mmu_cache(vma, address, (pte_t *)pmdp);
> > }
> >
> > /*
> > diff --git a/arch/mips/mm/tlb-r3k.c b/arch/mips/mm/tlb-r3k.c
> > index 53dfa2b9316b..e5722cd8dd6d 100644
> > --- a/arch/mips/mm/tlb-r3k.c
> > +++ b/arch/mips/mm/tlb-r3k.c
> > @@ -176,7 +176,8 @@ void local_flush_tlb_page(struct vm_area_struct *vma,
> > unsigned long page)
> > }
> > }
> >
> > -void __update_tlb(struct vm_area_struct *vma, unsigned long address, pte_t
> > pte)
> > +void update_mmu_cache(struct vm_area_struct *vma,
> > + unsigned long address, pte_t *ptep)
> > {
> > unsigned long asid_mask = cpu_asid_mask(_cpu_data);
> > unsigned long flags;
> > @@ -203,7 +204,7 @@ void __update_tlb(struct vm_area_struct *vma, unsigned
> > long address, pte_t pte)
> > BARRIER;
> > tlb_probe();
> > idx = read_c0_index();
> > - write_c0_entrylo0(pte_val(pte));
> > + write_c0_entrylo0(pte_val(*ptep));
> > write_c0_entryhi(address | pid);
> > if (idx < 0) { /* BARRIER */
> > tlb_write_random();
> > diff --git a/arch/mips/mm/tlb-r4k.c b/arch/mips/mm/tlb-r4k.c
> > index 1b939abbe4ca..c96725d17cab 100644
> > --- a/arch/mips/mm/tlb-r4k.c
> > +++ b/arch/mips/mm/tlb-r4k.c
> > @@ -290,14 +290,14 @@ void local_flush_tlb_one(unsigned long page)
> > * updates the TLB with the new pte(s), and another which also checks
> > * for the R4k "end of page" hardware bug and does the needy.
> > */
> > -void __update_tlb(struct vm_area_struct * vma, unsigned long address,
> > pte_t pte)
> > +void update_mmu_cache(struct vm_area_struct *vma,
> > + unsigned long address, pte_t *ptep)
> > {
> > unsigned long flags;
> > pgd_t *pgdp;
> > p4d_t *p4dp;
> > pud_t *pudp;
> > pmd_t *pmdp;
> > - pte_t *ptep;
> > int idx, pid;
> >
> > /*
> > @@ -326,10 +326,9 @@ void __update_tlb(struct vm_area_struct * vma,
> > unsigned long address, pte_t pte)
> > idx = read_c0_index();
> > #ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
> > /* this could be a huge page */
> > - if (pmd_huge(*pmdp)) {
> > + if (ptep == (pte_t *)pmdp) {
> > unsigned long lo;
> > write_c0_pagemask(PM_HUGE_MASK);
> > - ptep = (pte_t *)pmdp;
> > lo = pte_to_entrylo(pte_val(*ptep));
> > write_c0_entrylo0(lo);
> > write_c0_entrylo1(lo + (HPAGE_SIZE >> 7));
> > @@ -344,8 +343,6 @@ void __update_tlb(struct vm_area_struct * vma, unsigned
> > long address, pte_t pte)
> > } else
> > #endif
> > {
> > - ptep = pte_offset_map(pmdp, address);
> > -
> > #if defined(CONFIG_PHYS_ADDR_T_64BIT) && defined(CONFIG_CPU_MIPS32)
> > #ifdef CONFIG_XPA
> > write_c0_entrylo0(pte_to_entrylo(ptep->pte_high));
> > --
> > 2.35.3
> >
>
> I just bisected a crash while powering down a MIPS machine in QEMU to
> this change as commit 8044511d3893 ("mips: update_mmu_cache() can
> replace
Re: [PATCH v2 07/23] mips: update_mmu_cache() can replace __update_tlb()
Hi Hugh,
On Thu, Jun 08, 2023 at 12:17:24PM -0700, Hugh Dickins wrote:
> Don't make update_mmu_cache() a wrapper around __update_tlb(): call it
> directly, and use the ptep (or pmdp) provided by the caller, instead of
> re-calling pte_offset_map() - which would raise a question of whether a
> pte_unmap() is needed to balance it.
>
> Check whether the "ptep" provided by the caller is actually the pmdp,
> instead of testing pmd_huge(): or test pmd_huge() too and warn if it
> disagrees? This is "hazardous" territory: needs review and testing.
>
> Signed-off-by: Hugh Dickins
> ---
> arch/mips/include/asm/pgtable.h | 15 +++
> arch/mips/mm/tlb-r3k.c | 5 +++--
> arch/mips/mm/tlb-r4k.c | 9 +++--
> 3 files changed, 9 insertions(+), 20 deletions(-)
>
> diff --git a/arch/mips/include/asm/pgtable.h b/arch/mips/include/asm/pgtable.h
> index 574fa14ac8b2..9175dfab08d5 100644
> --- a/arch/mips/include/asm/pgtable.h
> +++ b/arch/mips/include/asm/pgtable.h
> @@ -565,15 +565,8 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> }
> #endif
>
> -extern void __update_tlb(struct vm_area_struct *vma, unsigned long address,
> - pte_t pte);
> -
> -static inline void update_mmu_cache(struct vm_area_struct *vma,
> - unsigned long address, pte_t *ptep)
> -{
> - pte_t pte = *ptep;
> - __update_tlb(vma, address, pte);
> -}
> +extern void update_mmu_cache(struct vm_area_struct *vma,
> + unsigned long address, pte_t *ptep);
>
> #define __HAVE_ARCH_UPDATE_MMU_TLB
> #define update_mmu_tlb update_mmu_cache
> @@ -581,9 +574,7 @@ static inline void update_mmu_cache(struct vm_area_struct
> *vma,
> static inline void update_mmu_cache_pmd(struct vm_area_struct *vma,
> unsigned long address, pmd_t *pmdp)
> {
> - pte_t pte = *(pte_t *)pmdp;
> -
> - __update_tlb(vma, address, pte);
> + update_mmu_cache(vma, address, (pte_t *)pmdp);
> }
>
> /*
> diff --git a/arch/mips/mm/tlb-r3k.c b/arch/mips/mm/tlb-r3k.c
> index 53dfa2b9316b..e5722cd8dd6d 100644
> --- a/arch/mips/mm/tlb-r3k.c
> +++ b/arch/mips/mm/tlb-r3k.c
> @@ -176,7 +176,8 @@ void local_flush_tlb_page(struct vm_area_struct *vma,
> unsigned long page)
> }
> }
>
> -void __update_tlb(struct vm_area_struct *vma, unsigned long address, pte_t
> pte)
> +void update_mmu_cache(struct vm_area_struct *vma,
> + unsigned long address, pte_t *ptep)
> {
> unsigned long asid_mask = cpu_asid_mask(_cpu_data);
> unsigned long flags;
> @@ -203,7 +204,7 @@ void __update_tlb(struct vm_area_struct *vma, unsigned
> long address, pte_t pte)
> BARRIER;
> tlb_probe();
> idx = read_c0_index();
> - write_c0_entrylo0(pte_val(pte));
> + write_c0_entrylo0(pte_val(*ptep));
> write_c0_entryhi(address | pid);
> if (idx < 0) { /* BARRIER */
> tlb_write_random();
> diff --git a/arch/mips/mm/tlb-r4k.c b/arch/mips/mm/tlb-r4k.c
> index 1b939abbe4ca..c96725d17cab 100644
> --- a/arch/mips/mm/tlb-r4k.c
> +++ b/arch/mips/mm/tlb-r4k.c
> @@ -290,14 +290,14 @@ void local_flush_tlb_one(unsigned long page)
> * updates the TLB with the new pte(s), and another which also checks
> * for the R4k "end of page" hardware bug and does the needy.
> */
> -void __update_tlb(struct vm_area_struct * vma, unsigned long address, pte_t
> pte)
> +void update_mmu_cache(struct vm_area_struct *vma,
> + unsigned long address, pte_t *ptep)
> {
> unsigned long flags;
> pgd_t *pgdp;
> p4d_t *p4dp;
> pud_t *pudp;
> pmd_t *pmdp;
> - pte_t *ptep;
> int idx, pid;
>
> /*
> @@ -326,10 +326,9 @@ void __update_tlb(struct vm_area_struct * vma, unsigned
> long address, pte_t pte)
> idx = read_c0_index();
> #ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
> /* this could be a huge page */
> - if (pmd_huge(*pmdp)) {
> + if (ptep == (pte_t *)pmdp) {
> unsigned long lo;
> write_c0_pagemask(PM_HUGE_MASK);
> - ptep = (pte_t *)pmdp;
> lo = pte_to_entrylo(pte_val(*ptep));
> write_c0_entrylo0(lo);
> write_c0_entrylo1(lo + (HPAGE_SIZE >> 7));
> @@ -344,8 +343,6 @@ void __update_tlb(struct vm_area_struct * vma, unsigned
> long address, pte_t pte)
> } else
> #endif
> {
> - ptep = pte_offset_map(pmdp, address);
> -
> #if defined(CONFIG_PHYS_ADDR_T_64BIT) && defined(CONFIG_CPU_MIPS32)
> #ifdef CONFIG_XPA
> write_c0_entrylo0(pte_to_entrylo(ptep->pte_high));
> --
> 2.35.3
>
I just bisected a crash while powering down a MIPS machine in QEMU to
this change as commit 8044511d3893 ("mips: update_mmu_cache() can
replace __update_tlb()") in linux-next. Unfortunately, I can still
reproduce it with the existing fix you have for this change on the
mailing list, which is present in next-20230614.
I can reproduce it with the GCC 13.1.0 on kernel.org [1].
[PATCH v2 07/23] mips: update_mmu_cache() can replace __update_tlb()
Don't make update_mmu_cache() a wrapper around __update_tlb(): call it
directly, and use the ptep (or pmdp) provided by the caller, instead of
re-calling pte_offset_map() - which would raise a question of whether a
pte_unmap() is needed to balance it.
Check whether the "ptep" provided by the caller is actually the pmdp,
instead of testing pmd_huge(): or test pmd_huge() too and warn if it
disagrees? This is "hazardous" territory: needs review and testing.
Signed-off-by: Hugh Dickins
---
arch/mips/include/asm/pgtable.h | 15 +++
arch/mips/mm/tlb-r3k.c | 5 +++--
arch/mips/mm/tlb-r4k.c | 9 +++--
3 files changed, 9 insertions(+), 20 deletions(-)
diff --git a/arch/mips/include/asm/pgtable.h b/arch/mips/include/asm/pgtable.h
index 574fa14ac8b2..9175dfab08d5 100644
--- a/arch/mips/include/asm/pgtable.h
+++ b/arch/mips/include/asm/pgtable.h
@@ -565,15 +565,8 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
}
#endif
-extern void __update_tlb(struct vm_area_struct *vma, unsigned long address,
- pte_t pte);
-
-static inline void update_mmu_cache(struct vm_area_struct *vma,
- unsigned long address, pte_t *ptep)
-{
- pte_t pte = *ptep;
- __update_tlb(vma, address, pte);
-}
+extern void update_mmu_cache(struct vm_area_struct *vma,
+ unsigned long address, pte_t *ptep);
#define__HAVE_ARCH_UPDATE_MMU_TLB
#define update_mmu_tlb update_mmu_cache
@@ -581,9 +574,7 @@ static inline void update_mmu_cache(struct vm_area_struct
*vma,
static inline void update_mmu_cache_pmd(struct vm_area_struct *vma,
unsigned long address, pmd_t *pmdp)
{
- pte_t pte = *(pte_t *)pmdp;
-
- __update_tlb(vma, address, pte);
+ update_mmu_cache(vma, address, (pte_t *)pmdp);
}
/*
diff --git a/arch/mips/mm/tlb-r3k.c b/arch/mips/mm/tlb-r3k.c
index 53dfa2b9316b..e5722cd8dd6d 100644
--- a/arch/mips/mm/tlb-r3k.c
+++ b/arch/mips/mm/tlb-r3k.c
@@ -176,7 +176,8 @@ void local_flush_tlb_page(struct vm_area_struct *vma,
unsigned long page)
}
}
-void __update_tlb(struct vm_area_struct *vma, unsigned long address, pte_t pte)
+void update_mmu_cache(struct vm_area_struct *vma,
+ unsigned long address, pte_t *ptep)
{
unsigned long asid_mask = cpu_asid_mask(_cpu_data);
unsigned long flags;
@@ -203,7 +204,7 @@ void __update_tlb(struct vm_area_struct *vma, unsigned long
address, pte_t pte)
BARRIER;
tlb_probe();
idx = read_c0_index();
- write_c0_entrylo0(pte_val(pte));
+ write_c0_entrylo0(pte_val(*ptep));
write_c0_entryhi(address | pid);
if (idx < 0) { /* BARRIER */
tlb_write_random();
diff --git a/arch/mips/mm/tlb-r4k.c b/arch/mips/mm/tlb-r4k.c
index 1b939abbe4ca..c96725d17cab 100644
--- a/arch/mips/mm/tlb-r4k.c
+++ b/arch/mips/mm/tlb-r4k.c
@@ -290,14 +290,14 @@ void local_flush_tlb_one(unsigned long page)
* updates the TLB with the new pte(s), and another which also checks
* for the R4k "end of page" hardware bug and does the needy.
*/
-void __update_tlb(struct vm_area_struct * vma, unsigned long address, pte_t
pte)
+void update_mmu_cache(struct vm_area_struct *vma,
+ unsigned long address, pte_t *ptep)
{
unsigned long flags;
pgd_t *pgdp;
p4d_t *p4dp;
pud_t *pudp;
pmd_t *pmdp;
- pte_t *ptep;
int idx, pid;
/*
@@ -326,10 +326,9 @@ void __update_tlb(struct vm_area_struct * vma, unsigned
long address, pte_t pte)
idx = read_c0_index();
#ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
/* this could be a huge page */
- if (pmd_huge(*pmdp)) {
+ if (ptep == (pte_t *)pmdp) {
unsigned long lo;
write_c0_pagemask(PM_HUGE_MASK);
- ptep = (pte_t *)pmdp;
lo = pte_to_entrylo(pte_val(*ptep));
write_c0_entrylo0(lo);
write_c0_entrylo1(lo + (HPAGE_SIZE >> 7));
@@ -344,8 +343,6 @@ void __update_tlb(struct vm_area_struct * vma, unsigned
long address, pte_t pte)
} else
#endif
{
- ptep = pte_offset_map(pmdp, address);
-
#if defined(CONFIG_PHYS_ADDR_T_64BIT) && defined(CONFIG_CPU_MIPS32)
#ifdef CONFIG_XPA
write_c0_entrylo0(pte_to_entrylo(ptep->pte_high));
--
2.35.3
