Message-ID: <CAAofZF6Gnzm9isPt3NUuSPBmBWQsj56O43pPZAf64WEP8no2Rg@mail.gmail.com>
Date: Wed, 2 Apr 2025 13:42:42 +0200
From: Marco Crivellari <marco.crivellari@...e.com>
To: "Maciej W. Rozycki" <macro@...am.me.uk>
Cc: linux-mips@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Thomas Bogendoerfer <tsbogend@...ha.franken.de>, Frederic Weisbecker <frederic@...nel.org>, 
	Anna-Maria Behnsen <anna-maria@...utronix.de>, Thomas Gleixner <tglx@...utronix.de>, 
	Peter Zijlstra <peterz@...radead.org>, Huacai Chen <chenhuacai@...nel.org>
Subject: Re: [PATCH v6 1/1] MIPS: Fix idle VS timer enqueue

Hi Maciej,

> Well, you should be able to set a breakpoint at `rollback_handle_int' and
> fiddle with $epc by hand to see if the code sequence correctly skips over
> WAIT.  Though I reckon QEMU used to have an issue with presenting the MIPS
> privileged context over its debug stub.  Has the issue been fixed?  Either
> way you should be able to just operate on the copy in $k0 retrieved with
> (D)MFC0.

Nope, seems not fixed so far. But yes, changing $k0 is working fine.
With the cpu in idle code (executing "wait"), ctrl+c, then placed a bp
in rollback_handle_init+4. Then "c" to hit the bp.
When the bp is hit, I can see $k0 = r4k_wait_exit.

I changed $k0 with an address inside the region, and setting a bp on "bne",
 the value is equal to $k1. I'm assuming the value is also saved
correctly in $epc,
considering it points correctly to r4k_wait_exit.

> See Section 3.5 of the latter manual for further
> discussion of the ISA bit.

Thank you, I found the manuals for rev. 5,
I still didn't have enough time to take a look anyhow.

> See the comment at said macro in <linux/compiler_types.h> and also commit
> 6727ad9e206c ("nmi_backtrace: generate one-line reports for idle cpus").

Aha, thanks!

 > Hmm, "skipover" maybe?

Now that I'm looking at the code, shouldn't it be better to address this in a
separate patch or another time?

I can see the rollback_handler* is exported also in arch/mips/kernel/traps.c
And there are a few parts that makes use of the "rollback" name; I'm
wondering if also
the code should be refactored a bit then,
eg arch/mips /include/asm/idle.h:using_rollback_handler()


Thank you!


On Mon, Mar 31, 2025 at 10:09 PM Maciej W. Rozycki <macro@...am.me.uk> wrote:
>
> On Mon, 31 Mar 2025, Marco Crivellari wrote:
>
> > > There's some complication here coming from the need to factor in the ISA
> > > bit in the microMIPS mode; something that hasn't been discussed so far.
> > > The `.fill 0' approach is a hack and it has struck me that we need to add
> > > a `.noinsn' pseudo-op to GAS for this purpose, complementing `.insn', but
> > > we need to stick with the hack for now anyway as it will take years until
> > > we can rely on a new feature in the assembler.
> >
> > Ah, interesting. So de facto having the "repeat" to 0, will generate nothing.
>
>  Correct, and as a side effect this pseudo-op clears the ISA bit on any
> label attached, as it is not an instruction.
>
> > > I can't imagine how we'd advance past WAIT without these instructions,
> > > what do you have in mind?
> >
> > I've not been precise, sorry.
> > I meant to remove the instructions like they are now because the
> > region would have
> > been different, then.
> > So, those instructions would have needed a change, in practice.
>
>  OK, so this is precisely what happened here with my proposal.  Thanks for
> clarifying.
>
> > > NB how do you actually verify this stuff with QEMU?  Is it by injecting
> > > an interrupt by hand at a chosen code location via GDB attached to QEMU's
> > > built-in debug stub?
> >
> > Short answer: I am not able to fully test this, I can only boot.
> >
> > The reason is that gdb-multiarch is not working as expected.
> > The binary in my distribution has the python support broken. So when I try to
> > inject the interrupt, I'm receiving a python error (actually, I receive the same
> > error after the "target remote" command).
> > I've also tried to compile the binary from source, but again,
> > I understand why in OBS the build I found is broken...
>
>  Well, you should be able to set a breakpoint at `rollback_handle_int' and
> fiddle with $epc by hand to see if the code sequence correctly skips over
> WAIT.  Though I reckon QEMU used to have an issue with presenting the MIPS
> privileged context over its debug stub.  Has the issue been fixed?  Either
> way you should be able to just operate on the copy in $k0 retrieved with
> (D)MFC0.
>
> > > Below I've included a complete change based on the outline above.  It
> > > seems to do the right thing for a couple of my configurations, but I've
> > > only eyeballed the resulting code and haven't tried running it.  Most of
> > > my hardware doesn't implement the WAIT instruction anyway.
> >
> > It means it would be better to have someone else try the resulting
> > patch, I guess.
>
>  Exhaustive run-time verification is not always necessary if you can
> demonstrate that your code is correct via other means, including
> proofreading.
>
>  FAOD I have one MIPS32r2 system wired for testing, but that might not be
> the most interesting configuration to verify as it'll now just use EI/EHB
> to enable interrupts ahead of WAIT.  I could try an R1 kernel instead, but
> I'm not sure if it can be made to work owing to the differences in the FPU
> between R1 and R2 for the MIPS32 ISA.  I used to have a MIPS64 (R1) system
> there, but the CPU daughtercard sadly stopped working 3 years ago and I
> wasn't able to repair it, owing to the lack of available spare parts (it's
> most likely a dead CPU).
>
> > > Let me know if you find anything here unclear or have any questions or
> > > comments.
> >
> > 1)
> >
> > > /* Keep the ISA bit clear for calculations on local labels here. */
> >
> > The ISA bit should be the bit 0, correct?
> > So, also in the macro code, it's done to preserve that bit.
>
>  Correct, the bit will be set according to the ISA mode at the time the
> originating machine instruction is executed, in EPC or any other register
> the PC is copied to, e.g. $ra.  Likewise any instruction setting the PC
> such as JR or ERET will set the ISA mode from the ISA bit of the source
> register (the ISA bit for exception entry is set from CP0 Config3.ISAOnExc
> register bit).
>
>  All code labels in microMIPS code will have the bit set, so that
> relocations correctly calculate immediates used to make register jumps.
>
>  The ISA mode can be switched explicitly with the JALX instruction (you
> can mix regular MIPS and microMIPS code as long as hardware supports the
> other ISA mode; either or both can be implemented in a given piece of
> silicon).
>
>  Other immediate jumps and branches preserve the current ISA mode, but the
> assembler and linker verify you don't attempt to use these instructions to
> pass control to code in the other mode; this is an assembly or link error
> depending at what stage the label reference is resolved.
>
>  FYI the documents for the microMIPS mode of operation are respectively:
>
> - "MIPS Architecture For Programmers Volume I-B: Introduction to the
>    microMIPS32 Architecture"
>
> - "MIPS Architecture for Programmers Volume II-B: The microMIPS32
>    Instruction Set"
>
> and their microMIPS64 counterparts (although the first document is almost
> an exact copy of its regular MIPS variant).  Be sure to download revision
> 5.xx, because revision 6.xx describes an entirely different ISA which we
> currently have no support for (downstream patches were never submitted;
> also microMIPSr6 removed branch delay slots, which caused all sorts of
> portability issues).  See Section 3.5 of the latter manual for further
> discussion of the ISA bit.
>
> > 2)
> > .section .cpuidle.text,"ax"
> >
> > This should be a single patch, right?
> > Based on what I understood, 'a' should be the allocation, and 'x' the
> > executable attribute.
>
>  Both correct (see the GAS manual for the section flags); this just
> matches the __cpuidle macro from <linux/compiler_types.h>.
>
> > This should be in order to mark those symbols like "cpuidle text":
> >
> > $ nm -n vmlinux | grep -A3 "cpuidle_text"
> > ffffffff80a127e0 T __cpuidle_text_start
> > ffffffff80a127e0 T r4k_wait
> > ffffffff80a12800 t r4k_wait_insn
> > ffffffff80a12804 t r4k_wait_exit
> > --
> > ffffffff80a12c00 T __cpuidle_text_end
> > ffffffff80a12c00 T __noinstr_text_end
> > ffffffff80a12c00 t rest_init
> > ffffffff80a12ccc t kernel_init
> >
> > I guess it is used in order to understand if the instruction pointer is inside
> > one of these functions / labels.
>
>  See the comment at said macro in <linux/compiler_types.h> and also commit
> 6727ad9e206c ("nmi_backtrace: generate one-line reports for idle cpus").
>
> > How does a commit description sound like this?
> >
> > "
> > mips: Add dedicated .cpuidle.text section for CPU idle routines
> >
> > Add a dedicated executable section for CPU idle code to properly organize
> > and identify idle-related functions inside the .text section.
> > "
>
>  How about:
>
> "
> MIPS: Move __r4k_wait() to .cpuidle.text section
>
> Fix missing .cpuidle.text section assignment for __r4k_wait() to correct
> backtracing with nmi_backtrace().
>
> Fixes: 97c8580e85cf ("MIPS: Annotate cpu_wait implementations with __cpuidle")
> "
>
> or suchlike (based on the commit referred)?
>
>  This probably does not itself qualify for linux-stable as the issue is
> only annoying rather than fatal, but I think the annotation should be
> there in case someone wants to backport it downstream.
>
> > 3)
> >
> > > I think we also need to replace "rollback" with
> > > another name as with new code we don't roll back anymore.
> >
> > Should be changed with "region", instead of rollback, maybe?
> > Do you have something better to suggest?
>
>  Hmm, "skipover" maybe?
>
>   Maciej



--

Marco Crivellari

L3 Support Engineer, Technology & Product




marco.crivellari@...e.com

Powered by blists - more mailing lists