arch: Add support for mips architecture #80
Conversation
|
I've tested the patch on openwrt/MIPS32 devices, PLY works fine. For MIPS64, the root cause is different types object files: |
There was a problem hiding this comment.
Great stuff!
Thank you for your contribution. The code looks good. I just hope we can simplify the changes to the build system a bit.
Regarding your comment about the 64-bit support: Can we just drop it for now? Rather than building something that has no hope of working anyway?
configure.ac
Outdated
| AM_CONDITIONAL([MIPS_ARCH], [AS_CASE([$host], [mips*], [true], [false])]) | ||
| AM_CONDITIONAL([MIPS64_CPU], [AS_CASE([$host], [mips64*], [true], [false])]) | ||
| AM_COND_IF([MIPS_ARCH], [AC_ARG_ENABLE(mips-n32n64, | ||
| AS_HELP_STRING([--enable-mips-n32n64], [enable n32/n64 (instead of o32) ABI for mips architecture]), | ||
| [if test x$enableval = xyes ; then | ||
| AC_DEFINE(USE_MIPS_N32, 1, [enable n32/n64 ABI for mips architecture]) | ||
| fi])]) | ||
| AM_COND_IF([MIPS64_CPU], [AC_DEFINE(USE_MIPS_64BIT, 1, [enable USE_MIPS64_BIT for mips64 cpus])]) |
There was a problem hiding this comment.
Shouldn't this information be available via $host_cpu? I.e. isn't this set via the --host argument to configure?
It seems unlikely that existing architectures all fit into the existing AC_CANONICAL_HOST, but MIPS needs this extra logic.
There was a problem hiding this comment.
Well, I'm not against dropping support for MIPS64 entirely, since I have many MIPS32 devices but none MIPS64 devices :)
Yes, $host_cpu and $host get set by --host argument, I will try to simplify changes to configure.ac, leaving only mips-n32n64 configure option.
src/libply/Makefile.am
Outdated
| if MIPS_ARCH | ||
| libply_la_SOURCES = arch/mips.c | ||
| else | ||
| libply_la_SOURCES = arch/@arch@.c | ||
| endif | ||
| libply_la_SOURCES += \ |
There was a problem hiding this comment.
This should not be needed, @arch@ should already hold the correct value.
There was a problem hiding this comment.
I remember that a few months ago, configuring with different --host parameters, results in different @arch@:
with --host=mips-linux-gcc, @arch@ will be mips, while configuring with --host=mipsel-buildroot-linux-musl-gcc, @arch@ will be mipsel. I will try again later, to make sure about it.
src/libply/arch/mips.c
Outdated
| { .name = "cp0_status", .type = &t_reg_t }, | ||
| { .name = "hi", .type = &t_reg_t }, | ||
| { .name = "lo", .type = &t_reg_t }, | ||
| #if 0 /* Let's pretend that CPU_HAS_SMARTMIPS could never be defined: */ |
There was a problem hiding this comment.
I'm not a MIPS guru. Why are we allowed to pretend that?
There was a problem hiding this comment.
On my MIPS32 Linux kernel build, CONFIG_CPU_HAS_SMARTMIPS is not defined; On buildroot/qemu_mips64 kernel build, the option is not defined either. I think the option is so rarely enabled, I would like to drop it; otherwise, another --enable-smartmips configuration option will be needed.
src/libply/arch/mips.c
Outdated
| #ifndef USE_MIPS_64BIT | ||
| /* pad for 32-bit system; refer to linux/arch/mips/include/asm/ptrace.h */ | ||
| { .name = "uarg0", .type = &t_reg_t }, | ||
| { .name = "uarg1", .type = &t_reg_t }, | ||
| { .name = "uarg2", .type = &t_reg_t }, | ||
| { .name = "uarg3", .type = &t_reg_t }, | ||
| { .name = "uarg4", .type = &t_reg_t }, | ||
| { .name = "uarg5", .type = &t_reg_t }, | ||
| { .name = "uarg6", .type = &t_reg_t }, | ||
| { .name = "uarg7", .type = &t_reg_t }, | ||
| #endif |
There was a problem hiding this comment.
Since there is no uprobe support yet this could be fine for now, but just for my understanding: Is the layout for struct pt_regs different for a kernel thread and a user thread? I.e. when we add uprobe support do we need to have another type for that?
There was a problem hiding this comment.
There is other struct pt_regs defined in arch/mips/include/uapi/asm/ptrace.h, which does not have 8 padding userspace registers. The kernel documentation says it is specifically used for ptrace:
This struct defines the registers as used by PTRACE_{GET,SET}REGS.
So I recon that the layout for kernel threads and user threads are identical on MIPS32. However, for MIPS64, 8 padding userspace registers are not present in struct pt_regs (accordding to arch/mips/include/asm/ptrace.h). If support for MIPS64 is to be dropped, the code can be simplified a lot.
|
I've updated the PR, thanks @wkz |
|
I've just received an email notifying CI build failures about the PR, and have checked the details:
I think the error is not related to the PR; but if I was wrong, what should I do to pass CI checks? |
|
@jaqchen Hello and thank you for taking on this work! I had been meaning to look at this after fixing the BPF infrastructure on OpenWrt some time ago. Could you share the build or packaging recipe you've used on OpenWrt? I'd like to get this included if possible. I'd also like to take a closer look at the MIPS64 problems you encountered -- have you discovered anything more since the first build failure? |
|
@jaqchen I'm so sorry that this fell off my radar. Thank you @guidosarducci, for triggering a notification 😄 I have a fix for the build issue. I've also started to look at ways of figuring out the selected ABI without having to pass explicit defines. Can't make any promises on the timeline, but I have every intention of merging this ASAP. |
|
Thanks for the great news! Looking forward to it... |
|
Merged with some additions in 1d12ea7. I added support for mips64 while I was at it, and tried to figure out the proper layout of @jaqchen I'm sorry this took me so long. Could you verify that everything still works as expected on your target system? Then I'll whip up a new release. @guidosarducci Maybe you could also try it out? |
|
Sorry for the late reply.
Current patch works well for my MIPS32 target devices, which I've simplified a lot.
I remember that the build-failure for MIPS64 cannot be resolved for now, because the way
The generated object |
|
Oh, so much progress has been made for MIPS target, GREAT AND THANKS! ^_^ |
Does that mean that the master branch works for your system? 😄 |
I just started looking at packaging it for OpenWrt so I should have more to say shortly. @jaqchen Did you have a hacked OpenWrt Makefile or any "gotchas" to be aware of from your prior work? Thanks! |
The latest PLY (updated in Dec 9) works fine for my MIPS32 target @guidosarducci @wkz :
I modified my prior Makefile, with COMMIT-ID and release date in generated package (i.e., ply_2022-12-09-1d12ea77-1_mipsel_24kc.ipk), you might need to modify it to include PLY version instead (placed in #
# Copyright (C) 2022 Ye Jiaqiang
#
# This is free software, licensed under the GNU General Public License v2.
# See /LICENSE for more information.
#
include $(TOPDIR)/rules.mk
PKG_NAME:=ply
PKG_RELEASE:=1
PKG_SOURCE_PROTO:=git
PKG_SOURCE_URL:=https://github.com/iovisor/ply.git
PKG_MIRROR_HASH:=022491103c4c280d3e68a3e65cae780dc24e2fdd40001ac6a4befa801f0273d0
PKG_SOURCE_DATE:=2022-12-09
PKG_SOURCE_VERSION:=1d12ea776d07ecc3a215458b28cbdb4a9e461227
PKG_LICENSE:=GPLv2
PKG_LICENSE_FILES:=COPYING
PKG_MAINTAINER:=Ye Jiaqiang <yejq.jiaqiang@gmail.com>
include $(INCLUDE_DIR)/package.mk
define Package/ply
SECTION:=devel
CATEGORY:=Development
TITLE:=Light-weight dynamic tracer for Linux
URL:=https://github.com/iovisor/ply.git
endef
define Package/ply/description
A light-weight dynamic tracer for Linux that leverages the kernel's
BPF VM in concert with kprobes and tracepoints to attach probes to
arbitrary points in the kernel.
endef
CONFIGURE_ARGS += --enable-shared=yes --enable-static=no
define Build/Prepare
$(call Build/Prepare/Default)
cd $(PKG_BUILD_DIR) && exec ./autogen.sh
endef
define Package/ply/install
$(INSTALL_DIR) $(1)/usr/sbin $(1)/usr/lib
$(INSTALL_BIN) $(PKG_BUILD_DIR)/src/ply/.libs/ply $(1)/usr/sbin/
$(CP) $(PKG_BUILD_DIR)/src/libply/.libs/libply.so* $(1)/usr/lib/
endef
$(eval $(call BuildPackage,ply))After some test, the above Makefile also builds for arm targets.
No, not really, I didn't find any "gotchas" ^_^. However, the latest PLY does not run on older kernels, (while prior version of PLY works) @wkz , which I think is due to the evolution of BPF architecture in the kernel (am I right?): |
@jaqchen Thanks, that's wonderful to have and I'll be using that Makefile shortly. I noticed it's fully fleshed out, with MAINTAINER and all; are you also planning on upstreaming this to OpenWrt? |
@guidosarducci No, I think I will be too busy to do that, please remove me from MAINTAINER if upstreaming has been planned. |
Alright, I'll do some testing myself first and then submit an upstream PR, including you in the commit but removing from MAINTAINER. Thanks again, this saves a lot of time. |
@wkz I was able to package and build I see the string/arg issue on mips32be, mips32le and mips64le, which seems to discount an endian or word-size bug as I first suspected. None of these problems appear on ARM32 however. The relevant kernel code and some logs illustrating the problem are below. Kernel Code: (https://elixir.bootlin.com/linux/v5.15.83/source/fs/open.c#L1195)ARMVIRT32: (good)MIPS32BE: (bad)Could you please have a think about these symptoms? I suspect they may ring a bell given your familiarity with internals. I'd also appreciate if you could suggest other useful test cases e.g. printing chars from arrays, dumping the the pt_regs struct, testing the core @jaqchen Did this functionality work for you previously? Does it continue to work on your MIPS builds? A further point I'll note is that I'm running BTF-enabled builds after contributing this to OpenWrt, and the various MIPS32BE:MIPS64LE: |
This is what I got when running ply on my MIPS32LE openwrt: The
For MIPS32BE (I only have MIPS32LE devices, which is somewhat identical), you can try to get the size of And we know that for MIPS32BE, |
@jaqchen Great, thanks! This at least clarifies the functionality should work/has worked before. Unfortunately, as noted I am seeing the same problem across MIPS32BE, MIPS32LE and MIPS64LE.
To double-check, I used gdb as you suggest and found the expected results: At this stage the options to investigate are narrowing. One thing I noted is that I'm building on LTS kernel 5.15 while you're using kernel 5.10. I'm going to try building MIPS32BE on kernel 5.10 next. Could you try building with kernel 5.15 (OpenWrt snapshot) for comparison? Also, were you using the latest @wkz Anything else you might suggest is appreciated. Thanks all! |
|
Just a quick update that things work fine on LTS kernel 5.10 for MIPS32BE, MIPS32LE and MIPS64LE. For example, MIPS32BE: It's possible something broke in the kernel between 5.10 and 5.15, or something deliberately changed which |
Yes, I built latest ply and it works fine on kernel 5.10.
I think you are right. @guidosarducci I might try kernel 5.15 on AArch64 device, I don't think the problem is specific to MIPS architecture. |
There exists mainly two variants of ABIs in the linux kernel
for MIPS architecture, o32 and n32/n64. The register sets are
largely the same between 32-bit and 64-bit MIPS processors,
so single arch/mips.c is added to support both.
Note that configure option `--enable-n32n64 can be specified
to enable n32/n64 ABI for 32-bit MIPS build.