Re: [PATCH V10 14/22] LoongArch: Add signal handling support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 5/16/22 22:06, Eric W. Biederman wrote:

WANG Xuerui <kernel@xxxxxxxxxx> writes:

Hi,

On 5/15/22 21:48, Huacai Chen wrote:
diff --git a/arch/loongarch/include/uapi/asm/sigcontext.h b/arch/loongarch/include/uapi/asm/sigcontext.h
new file mode 100644
index 000000000000..efeb8b3f8236
--- /dev/null
+++ b/arch/loongarch/include/uapi/asm/sigcontext.h
@@ -0,0 +1,63 @@
+/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
+/*
+ * Author: Hanlu Li <lihanlu@xxxxxxxxxxx>
+ *         Huacai Chen <chenhuacai@xxxxxxxxxxx>
+ *
+ * Copyright (C) 2020-2022 Loongson Technology Corporation Limited
+ */
+#ifndef _UAPI_ASM_SIGCONTEXT_H
+#define _UAPI_ASM_SIGCONTEXT_H
+
+#include <linux/types.h>
+#include <linux/posix_types.h>
+
+/* FP context was used */
+#define USED_FP                      (1 << 0)
+/* Load/Store access flags for address error */
+#define ADRERR_RD            (1 << 30)
+#define ADRERR_WR            (1 << 31)
I've searched GitHub globally, and my local glibc checkout, for usages
of these 3 constants, and there seems to be none; please consider
removing these if doable. We don't want cruft in uapi right from the
beginning.
They will be used in our glibc, I promise.
Okay then. Seems simple enough, and from my quick grepping these appear to be
original creations -- not carried over from somewhere else, so it's already
highly likely that some of the userland tools need these anyway, just not
released yet.
I can understand exporting these values but the names aren't very
well namespaced at all.  Which means they could accidentially
conflict with things.

It would probably be better to do:
SC_USED_FP
SC_ADDRERR_RD
SC_ADDRERR_WR

And with two D's please.  It breaks my fingers to have to
make a typo like that on purpose.

This is very much a bikeshed comment, but I think the
bikeshed should be painted.
IIUC, the ADRERR spelling is because of influence of BUS_ADRERR. But the prefix idea sounds good.
+
+struct sigcontext {
+     __u64   sc_pc;
+     __u64   sc_regs[32];
+     __u32   sc_flags;
+     __u64   sc_extcontext[0] __attribute__((__aligned__(16)));
+};
+
+#define CONTEXT_INFO_ALIGN   16
+struct _ctxinfo {
+     __u32   magic;
+     __u32   size;
+     __u64   padding;        /* padding to 16 bytes */
+};
+
+/* FPU context */
+#define FPU_CTX_MAGIC                0x46505501
+#define FPU_CTX_ALIGN                8
+struct fpu_context {
+     __u64   regs[32];
+     __u64   fcc;
+     __u32   fcsr;
+};
The 3 structs above should already see usage downstream (glibc and other
low-level friends), so they probably shouldn't be touched by now. At
least I can't see problems.
+
+/* LSX context */
+#define LSX_CTX_MAGIC                0x53580001
+#define LSX_CTX_ALIGN                16
+struct lsx_context {
+     __u64   regs[2*32];
+     __u64   fcc;
+     __u32   fcsr;
+     __u32   vcsr;
+};
+
+/* LASX context */
+#define LASX_CTX_MAGIC               0x41535801
+#define LASX_CTX_ALIGN               32
+struct lasx_context {
+     __u64   regs[4*32];
+     __u64   fcc;
+     __u32   fcsr;
+     __u32   vcsr;
+};
Do we want to freeze the LSX/LASX layout this early, before any detail
of said extension are published? We'll need to update kernel later
anyway, so I'd recommend leaving them out for the initial bring-up.
Yes, they are freezed.
Okay too, I remember these are the same values as in the old world, so it should
be easy to support both worlds at least in this regard.
You know.  I really don't like this including code for hardware that may
be frozen but is not publicly documented yet.  Especially when the plan
is to publicly document the hardware.  It has the real problem that no
one else can review the code.

In ever design I have worked with there have been places where the
people putting it together have had blind spots.  The only way I know to
get past blind spots is to get as many people as possible looking,
and to listen to the feedback.

Given that neither lsx_context nor lasx_context are used in the kernel
code yet I would very much prefer that their inclusion wait until there
is actual code that needs them.

If nothing else that will put the definitions in context so people can
more easily see the big picture and understand how the pieces fit.

Hmm, thinking twice, the code actually doesn't get destroyed, nor magically "thawed" and modified, if not upstreamed initially; just that these same lines would go in later. Maybe I overlooked the problem because I've tried to reverse-engineer the LSX/LASX back in the MIPS days of Loongson, and that I've seen early version of the port that contained the same handling, so all of this come as familiar.

So actually removing the code is the sensible thing to do here. We don't really lose anything or waste too much time for that.


Eric



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux