Add LoongArch support for kpatch v2 #1427

georgejguo · 2024-12-05T08:28:32Z

hi joe,
This pr is based for PR #1415.

jpoimboe · 2024-12-05T19:05:32Z

@georgejguo FYI, instead of opening a new PR, next time you can just rebase your existing branch and force push it, which will reuse the existing PR.

joe-lawrence

Hi @georgejguo - this is a really quick scan of the v2 patchset, see the github comments inline. A few follow ups:

Are there LoongArch specific objects files that are generated by the kernel build, but aren't really part of the kernel itself? See kpatch-build/kpatch-cc and the long list of fileglobs that the are skipped. For example, I'd expect to see arch/loongarch/vdso/* in there.
Without hardware, this will be difficult for us to test and maintain. Are there any public LoongArch resources available for running integration tests?
Do the integration tests run for your kernel? (Which one(s) btw?)
Would you be willing to generate a set of unit test object files (see https://github.com/dynup/kpatch-unit-test-objs where we keep those for various arches. I can walk you through how to generate them.)
Last but not least, @jpoimboe what are your thoughts on supporting arches (arm64 and now LoongArch) that require out-of-tree kernel patches? I was leaning towards some kind of disclaimer and a very careful check that the kernel matches one of these special downstream versions.

joe-lawrence · 2024-12-05T17:42:21Z

kpatch-build/create-diff-object.c

@@ -694,6 +696,11 @@ static bool insn_is_load_immediate(struct kpatch_elf *kelf, void *addr)

 		break;

+	case LOONGARCH64:
+		/* to be done */


I m not sure here, so just commit "to be done". Actually it can pass my simple test case.

joe-lawrence · 2024-12-05T17:51:06Z

kpatch-build/create-diff-object.c

+			bool found = false;
+			unsigned int *insn = sym->sec->data->d_buf + sym->sym.st_value;
+
+			if (*insn == LOONGARCH_NOP && *(insn + 1) == LOONGARCH_NOP)


I do not believe this is endian-safe, ie, if this were run on a BE system like s390x (hypothetical/future cross compilation case), then the bytes would not be in the expected order. See other arch cases or https://github.com/dynup/kpatch/pull/52863dace0200332dc2ed5d8edab1f017e63b368 for specific bugfix where insn is treated as a sequence of bytes and not an unsigned integer.

the link(https://github.com/dynup/kpatch/pull/52863dace0200332dc2ed5d8edab1f017e63b368) is 404 error

Sorry about that, try this link: 52863da for ("create-diff-object: fix endianness in kpatch_no_sibling_calls_ppc64le()")

kpatch-build/create-diff-object.c

joe-lawrence · 2024-12-05T20:16:56Z

kpatch-build/kpatch-elf.c

+				rela->sym = rela->sym->sec->secsym;
+				log_debug("section symbol: %s\n", rela->sym->name);
+			}
+		}


Just curious, which kernel source files are generating these symbols?

Okey, I will try to find it.

joe-lawrence · 2024-12-05T20:40:18Z

kpatch-build/create-diff-object.c

@@ -4081,6 +4102,23 @@ static void kpatch_find_func_profiling_calls(struct kpatch_elf *kelf)
 				insn[4] == 0x00 && insn[5] == 0x00)
 				sym->has_func_profiling = 1;
 			break;
+		case LOONGARCH64:
+			struct section *sec;


Older gcc complains about this:

gcc -std=gnu11 -MMD -MP -I../kmod/patch -Iinsn -Wall -Wsign-compare -Wconversion -Wno-sign-conversion -g -Werror -c -o create-diff-object.o create-diff-object.c create-diff-object.c: In function ‘kpatch_find_func_profiling_calls’: create-diff-object.c:4106:4: error: a label can only be part of a statement and a declaration is not a statement struct section *sec; ^~~~~~

if this new variable remains in v3, please wrap the case block in {} brackets to appease those compilers.

georgejguo · 2024-12-06T09:07:13Z

Hi @georgejguo - this is a really quick scan of the v2 patchset, see the github comments inline. A few follow ups:

Are there LoongArch specific objects files that are generated by the kernel build, but aren't really part of the kernel itself? See kpatch-build/kpatch-cc and the long list of fileglobs that the are skipped. For example, I'd expect to see arch/loongarch/vdso/* in there.

Okey, I will clear it out and try to find ones if it turns out to be ture.
Could you give me some help, I don't know how to find these files?
BTW, this is related my kpatch-elf.c patch?

Without hardware, this will be difficult for us to test and maintain. Are there any public LoongArch resources available for running integration tests?

Do the integration tests run for your kernel? (Which one(s) btw?)

I have tested the code in Fedora-38 with kernel 6.12 and gcc 14.1 for Loongarch, just a simple cases.
As far as I know, there is not any public LoongArch machines availabe for testing, But I have enough machines, and I am willing to maintain and test.

Would you be willing to generate a set of unit test object files (see https://github.com/dynup/kpatch-unit-test-objs where we keep those for various arches. I can walk you through how to generate them.)

Yeap, I am willing to do that.

Last but not least, @jpoimboe what are your thoughts on supporting arches (arm64 and now LoongArch) that require out-of-tree kernel patches? I was leaning towards some kind of disclaimer and a very careful check that the kernel matches one of these special downstream versions.

The kernel has supported liveptch for Loongarch in upstream.

[edited by Joe to cleanup the markdown for github web rendering]

joe-lawrence · 2024-12-06T14:43:30Z

The kernel has supported liveptch for Loongarch in upstream.

Great, thanks for heads up. I must have missed it as it seems like the patchset to turn on the config for that arch was never copied to the [email protected] mailing list (actually having some problems finding much lmkl discussion / posts, too). Anyway select HAVE_LIVEPATCH is indeed present in arch/arloongarch/Kconfig.

joe-lawrence · 2024-12-10T17:00:25Z

Are there LoongArch specific objects files that are generated by the kernel build, but aren't really part of the kernel itself? See kpatch-build/kpatch-cc and the long list of fileglobs that the are skipped. For example, I'd expect to see arch/loongarch/vdso/* in there.

Okey, I will clear it out and try to find ones if it turns out to be ture.
Could you give me some help, I don't know how to find these files?

Hmm, I don't know of a programmatic way off the top of my head, maybe @jpoimboe has a suggestion. That said, the vdso code should be excluded for sure.

BTW, this is related my kpatch-elf.c patch?

Sort of. Using a gcc-14.2.0 cross compiler and the latest kernel tree, I couldn't find any .LBB symbol instances outside of the vdso object files... that's why I was curious to know if it is something that create-diff-object really needs to account for (if the pattern never occurs in kernel code).

Add section alt_instr check support for LoongArch. Signed-off-by: George Guo <[email protected]>

Add initial support for LoongArch. Signed-off-by: George Guo <[email protected]>

Generate 2 NOPs right at the beginning of each function with -fpatchable-function-entry=2 in LoongArch. Here process this situation. Co-developed-by: zhanghongchen <[email protected]> Signed-off-by: George Guo <[email protected]>

Here fix error like: "tcp.o: symbol changed sections: .LBB7266. create-diff-object: unreconcilable difference". Due to LoongArch GCC generating local labels such as .LBB7266, it is difficult to compare the modified sections in the corresponding object files of the two files before and after the patch, so change them with sections symbols in rela section, and delete them in other sections. Co-developed-by: zhanghongchen <[email protected]> Signed-off-by: George Guo <[email protected]>

georgejguo · 2024-12-11T03:06:15Z

hi， this is Add LoongArch support for kpatch v3

The updated file is only kpatch-build/create-diff-object.c according to your advice, Joe
The commit id is f27619f

joe-lawrence · 2024-12-12T22:07:01Z

Hi @georgejguo , I tried this branch's version of create-diff-object against object files created by a gcc-14.2.0 cross compiler and the examples/cmdline.patch, but encountered this error, "changed section .rela.orc_unwind_ip not selected for inclusion". Then I found this stale PR: #1312 where @ZhangHongchen1 mentioned that section with regard to arch-specific differences on LoongArch.

I also noticed that he has a detailed LoongArch support branch in his fork that accounts for several additional things, like trampolines for short (local?) function calls, load immediate instructions, etc. It would be worth seeing if @ZhangHongchen1 intends to pick up or continue that work and then combine efforts with a single PR.

georgejguo · 2024-12-13T01:53:38Z

hi， this is Add LoongArch support for kpatch v4

The updated file is only kpatch-build/create-diff-object.c
The commit id is 695b74b which fixes error, "changed section .rela.orc_unwind_ip not selected for inclusion".

I also added Co-developed-by: zhanghongchen [email protected].

Sorry, I lost this commit while pushing ”Add LoongArch support for kpatch V3“

georgejguo · 2024-12-13T02:06:37Z

Hi @georgejguo , I tried this branch's version of create-diff-object against object files created by a gcc-14.2.0 cross compiler and the examples/cmdline.patch, but encountered this error, "changed section .rela.orc_unwind_ip not selected for inclusion". Then I found this stale PR: #1312 where @ZhangHongchen1 mentioned that section with regard to arch-specific differences on LoongArch.

I also noticed that he has a detailed LoongArch support branch in his fork that accounts for several additional things, like trampolines for short (local?) function calls, load immediate instructions, etc. It would be worth seeing if @ZhangHongchen1 intends to pick up or continue that work and then combine efforts with a single PR.

Sorry, I lost a commit to fix this while pushing ”Add LoongArch support for kpatch V3“.
Actually , In my test case, everything is OK, except for section .rela.orc_unwind_ip.
If there are other sections like '.rela.orc_unwind_ip', it would be better to open a new PR for them."
@ZhangHongchen1 what's your opinion, Hongchen?

Fix error: "changed section .rela.orc_unwind_ip not selected for inclusion". This section is about arch-specific differences on LoongArch, which is generated by LoongArch gcc. Co-developed-by: zhanghongchen <[email protected]> Signed-off-by: George Guo <[email protected]>

Since kpatch now supports LoongArch basically, enable the build. Signed-off-by: George Guo <[email protected]>

Added conditional compilation to prevent 'R_LARCH_64' and 'EM_LOONGARCH' from being referenced on x86 and other non-LoongArch architectures. This ensures the code works across different architectures without errors. Signed-off-by: George Guo <[email protected]>

georgejguo · 2024-12-13T03:46:50Z

hi， this is Add LoongArch support for kpatch v5

The updated file is only kpatch-build/create-diff-object.c
The commit id is dcf1188 fixing error, "changed section .rela.orc_unwind_ip not selected for inclusion", which is better than v4 695b74b)

joe-lawrence reviewed Dec 5, 2024

View reviewed changes

georgejguo added 4 commits December 11, 2024 10:55

kpatch/LoongArch: Add LoongArch specific features

8368005

Add section alt_instr check support for LoongArch. Signed-off-by: George Guo <[email protected]>

kpatch/LoongArch: Add initial support for kpatch

e2ad05f

Add initial support for LoongArch. Signed-off-by: George Guo <[email protected]>

georgejguo force-pushed the master branch from a873bfa to 0f0c129 Compare December 11, 2024 02:59

georgejguo force-pushed the master branch from 0f0c129 to b49793e Compare December 13, 2024 01:49

georgejguo added 3 commits December 13, 2024 11:37

kpatch/LoongArch: enable kpatch build

3b637df

Since kpatch now supports LoongArch basically, enable the build. Signed-off-by: George Guo <[email protected]>

georgejguo force-pushed the master branch from b49793e to 31b8eb5 Compare December 13, 2024 03:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add LoongArch support for kpatch v2 #1427

Add LoongArch support for kpatch v2 #1427

georgejguo commented Dec 5, 2024

jpoimboe commented Dec 5, 2024 •

edited

Loading

joe-lawrence left a comment

joe-lawrence Dec 5, 2024

georgejguo Dec 6, 2024

joe-lawrence Dec 5, 2024

georgejguo Dec 10, 2024

joe-lawrence Dec 10, 2024

joe-lawrence Dec 5, 2024

georgejguo Dec 6, 2024

joe-lawrence Dec 5, 2024

georgejguo commented Dec 6, 2024 •

edited

Loading

joe-lawrence commented Dec 6, 2024

joe-lawrence commented Dec 10, 2024

georgejguo commented Dec 11, 2024 •

edited

Loading

joe-lawrence commented Dec 12, 2024

georgejguo commented Dec 13, 2024 •

edited

Loading

georgejguo commented Dec 13, 2024 •

edited

Loading

georgejguo commented Dec 13, 2024 •

edited

Loading

Add LoongArch support for kpatch v2 #1427

Are you sure you want to change the base?

Add LoongArch support for kpatch v2 #1427

Conversation

georgejguo commented Dec 5, 2024

jpoimboe commented Dec 5, 2024 • edited Loading

joe-lawrence left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgejguo commented Dec 6, 2024 • edited Loading

joe-lawrence commented Dec 6, 2024

joe-lawrence commented Dec 10, 2024

georgejguo commented Dec 11, 2024 • edited Loading

joe-lawrence commented Dec 12, 2024

georgejguo commented Dec 13, 2024 • edited Loading

georgejguo commented Dec 13, 2024 • edited Loading

georgejguo commented Dec 13, 2024 • edited Loading

jpoimboe commented Dec 5, 2024 •

edited

Loading

georgejguo commented Dec 6, 2024 •

edited

Loading

georgejguo commented Dec 11, 2024 •

edited

Loading

georgejguo commented Dec 13, 2024 •

edited

Loading

georgejguo commented Dec 13, 2024 •

edited

Loading

georgejguo commented Dec 13, 2024 •

edited

Loading