Re: [RFC PATCH 28/42] target/mips/tx79: Move RDHWR usermode kludge to trans_SQ()

From: "Maciej W. Rozycki" <macro@orcam.me.uk>
To: Fredrik Noring <noring@nocrew.org>
Cc: "Thomas Huth" <thuth@redhat.com>,
	qemu-devel@nongnu.org,
	"Aleksandar Rikalo" <aleksandar.rikalo@syrmia.com>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"Laurent Vivier" <laurent@vivier.eu>,
	"Philippe Mathieu-Daudé" <f4bug@amsat.org>,
	"Alex Bennée" <alex.bennee@linaro.org>,
	"Aurelien Jarno" <aurelien@aurel32.net>
Subject: Re: [RFC PATCH 28/42] target/mips/tx79: Move RDHWR usermode kludge to trans_SQ()
Date: Tue, 16 Feb 2021 13:21:34 +0100 (CET)	[thread overview]
Message-ID: <alpine.DEB.2.21.2102161244430.1521@angie.orcam.me.uk> (raw)
In-Reply-To: <YCtu1czHMjeixsj+@sx9>

On Tue, 16 Feb 2021, Fredrik Noring wrote:

> > Not that it's odd (the final address is masked, remember), but that it a store
> > to an address in the zero page.
> 
> The address always resolves to 0xffffe83b (then masked) in 32-bit KSEG2,
> because rt is always $3 and rd is always $29 so -6085(zero), hence the
> last page (which is much better) rather than the first, as Maciej
> discovered:
> 
> https://patchwork.kernel.org/comment/23824173/
> 
> Other possible RDHWR encodings are no longer used, and can therefore be
> ignored and revert to SQ:
> 
> https://patchwork.kernel.org/comment/23842167/

 Or rather were never used in the general case (I can't rule out someone 
using that stuff for something, but I wouldn't call it supported; I used 
some of it internally while evaluating the speed of RDHWR emulation before 
the use of $3 or indeed RDHWR was settled in the TLS psABI, though the 
actual code that ultimately went into Linux was developed independently).

> > I would do this as
> > 
> > {
> >   RDHWR_user  011111 00000 ..... ..... 00000 111011   @rd_rt
> >   SQ          011111 ..... ..... ................     @ldst
> > }
> 
> Both rd and rt have fixed values, as mentioned.

 I would suggest actually supporting variable `rt', see below.  Would it 
be a problem?

> For reference, RDHWR is currently done like this in the Linux kernel:
> 
> 	if (IS_ENABLED(CONFIG_CPU_R5900)) {
> 		/*
> 		 * On the R5900, a valid RDHWR instruction
> 		 *
> 		 *     +--------+-------+----+----+-------+--------+
> 		 *     | 011111 | 00000 | rt | rd | 00000 | 111011 |
> 		 *     +--------+-------+----+----+-------+--------+
> 		 *          6       5      5    5     5        6
> 		 *
> 		 * having rt $3 (v1) and rd $29 (MIPS_HWR_ULR) is
> 		 * interpreted as the R5900 specific SQ instruction
> 		 *
> 		 *     +--------+-------+----+---------------------+
> 		 *     | 011111 |  base | rt |        offset       |
> 		 *     +--------+-------+----+---------------------+
> 		 *          6       5      5            16
> 		 *
> 		 * with
> 		 *
> 		 *     sq v1,-6085(zero)
> 		 *
> 		 * that asserts an address exception since -6085(zero)
> 		 * always resolves to 0xffffe83b in 32-bit KSEG2.
> 		 *
> 		 * Other legacy values of rd, such as MIPS_HWR_CPUNUM,
> 		 * are ignored.
> 		 */
> 		if (insn.r_format.func == rdhwr_op &&
> 		    insn.r_format.rd == MIPS_HWR_ULR &&
> 		    insn.r_format.rt == 3 &&

 I suggest leaving the `rt' check out for consistency, as changing the 
register to read the value of UserLocal into from psABI-mandated $3 does 
not cause any issue with the R5900 (the `rt' field overlaps between both 
machine instructions, so the encoding placed there does not affect the 
KSEG2 access trap caused) and those encodings are also emulated in the 
slow path for other legacy ISA CPUs:

	case MIPS_HWR_ULR:		/* Read UserLocal register */
		regs->regs[rt] = ti->tp_value;
		return 0;

 So e.g. `rdhwr $25, $29' is interpreted as `sq $25,-6085($0)' by the 
R5900 => no issue, it still traps.

 I know I have previously written that we can ignore `rt' encodings other 
than $3, but they are harmless and handling them saves a couple of machine 
instructions needed to make the check, so I think while we can, we do not 
actually have to ignore them.

> 		    insn.r_format.rs == 0 &&
> 		    insn.r_format.re == 0) {
> 			if (compute_return_epc(regs) < 0 ||
> 			    simulate_rdhwr(regs, insn.r_format.rd,
> 					   insn.r_format.rt) < 0)
> 				goto sigill;
> 			return;
> 		}
> 		goto sigbus;
> 	} else ...

 Code continuation quoted left for reference.

  Maciej