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
