On Thu, Sep 14, 2017 at 01:57:08PM +0100, Alex Bennée wrote:
>
> Dave Martin <Dave.Martin@xxxxxxx> writes:
>
> > This patch defines and implements a new regset NT_ARM_SVE, which
> > describes a thread's SVE register state. This allows a debugger to
> > manipulate the SVE state, as well as being included in ELF
> > coredumps for post-mortem debugging.
> >
> > Because the regset size and layout are dependent on the thread's
> > current vector length, it is not possible to define a C struct to
> > describe the regset contents as is done for existing regsets.
> > Instead, and for the same reasons, NT_ARM_SVE is based on the
> > freeform variable-layout approach used for the SVE signal frame.
> >
> > Additionally, to reduce debug overhead when debugging threads that
> > might or might not have live SVE register state, NT_ARM_SVE may be
> > presented in one of two different formats: the old struct
> > user_fpsimd_state format is embedded for describing the state of a
> > thread with no live SVE state, whereas a new variable-layout
> > structure is embedded for describing live SVE state. This avoids a
> > debugger needing to poll NT_PRFPREG in addition to NT_ARM_SVE, and
> > allows existing userspace code to handle the non-SVE case without
> > too much modification.
> >
> > For this to work, NT_ARM_SVE is defined with a fixed-format header
> > of type struct user_sve_header, which the recipient can use to
> > figure out the content, size and layout of the reset of the regset.
> > Accessor macros are defined to allow the vector-length-dependent
> > parts of the regset to be manipulated.
> >
> > Signed-off-by: Alan Hayward <alan.hayward@xxxxxxx>
> > Signed-off-by: Dave Martin <Dave.Martin@xxxxxxx>
> > Cc: Alex Bennée <alex.bennee@xxxxxxxxxx>
> >
> > ---
> >
> > Changes since v1
> > ----------------
> >
> > Other changes related to Alex Bennée's comments:
> >
> > * Migrate to SVE_VQ_BYTES instead of magic numbers.
> >
> > Requested by Alex Bennée:
> >
> > * Thin out BUG_ON()s:
> > Redundant BUG_ON()s and ones that just check invariants are removed.
> > Important sanity-checks are migrated to WARN_ON()s, with some
> > minimal best-effort patch-up code.
> >
> > Other:
> >
> > * [ABI fix] Bail out with -EIO if attempting to set the
> > SVE regs for an unsupported VL, instead of misparsing the regset data.
> >
> > * Replace some in-kernel open-coded arithmetic with ALIGN()/
> > DIV_ROUND_UP().
> > ---
[...]
> > diff --git a/arch/arm64/include/uapi/asm/ptrace.h b/arch/arm64/include/uapi/asm/ptrace.h
[...]
> > +/* Definitions for user_sve_header.flags: */
> > +#define SVE_PT_REGS_MASK (1 << 0)
> > +
> > +/* Flags: must be kept in sync with prctl interface in
> > <linux/ptrace.h> */
>
> Which flags? We base some on PR_foo flags but we seem to shift them
> anyway so where is the requirement for them to match from?
I've rearranged this as:
-8<-
/* Definitions for user_sve_header.flags: */
#define SVE_PT_REGS_MASK (1 << 0)
#define SVE_PT_REGS_FPSIMD 0
#define SVE_PT_REGS_SVE SVE_PT_REGS_MASK
/*
* Common SVE_PT_* flags:
* These must be kept in sync with prctl interface in <linux/ptrace.h>
*/
#define SVE_PT_VL_INHERIT (PR_SVE_VL_INHERIT >> 16)
#define SVE_PT_VL_ONEXEC (PR_SVE_SET_VL_ONEXEC >> 16)
->8-
This avoids the suggestion that SVE_PT_REGS_{MASK,FPSIMD,SVE} are
supposed to have prctl counterparts.
I don't really want to write more, in case it is misinterpreted as
specification of behaviour.
This comment is really only meant as a reminder to maintainers that
they should go look at prctl.h too, before blindly making changes
here.
Any good? If you have a different suggestion, I'm all ears...
[...]
Cheers
---Dave
