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

[WANG Xuerui]

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