Re: [PATCH v2 2/7] arm64/fpsimd: Track the saved FPSIMD state type separately to TIF_SVE
On Wed, Jul 20, 2022 at 10:40:03AM +0100, Marc Zyngier wrote: > Mark Brown wrote: > > Yes, someone might forget to update the state type but my experience > > with this code is that it's a lot easier to spot "this is writing new > > state, did it update the state type?" than "this is writing new state, > > did it call the helper that implicitly updates the state type or the > > other one?". > My experience in maintaining the KVM code is that the least state > leaks outside of this sort of helpers, the least problematic they > are. I'd rather have multiple helpers that have different *documented* > behaviours than expecting the random hacker to know (or in this case, > *guess*) when or not to add some extra state-twiddling. It also makes > the code far more readable because it is self-contained. > If this series is supposed to help making things more maintainable, > then this is one way to do it. There's nothing self contained going on, and based on what the users are doing it isn't at all obvious to me that taking a copy of the data in another format should be part of the same operation as deciding which format should be the live data. I'm all for helpers but between that and the direct awareness the users already need to have of what's going on I'm just not seeing that a combined convert and make active operation is jumping out as something that should be a helper. signature.asc Description: PGP signature ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 2/7] arm64/fpsimd: Track the saved FPSIMD state type separately to TIF_SVE
On Mon, 11 Jul 2022 16:53:43 +0100,
Mark Brown wrote:
>
> On Mon, Jul 11, 2022 at 03:33:51PM +0100, Marc Zyngier wrote:
> > Mark Brown wrote:
> > > On Mon, Jul 11, 2022 at 10:40:50AM +0100, Marc Zyngier wrote:
> > > > Mark Brown wrote:
>
> > > > > + enum fp_state *type;
>
> > > > For consistency: s/type/fp_type/ ?
>
> > > Sure if nobody else wants a different bikeshed. It really needs a
> > > longer name like fp_state_t or something but that had it's own problems
> > > with non-idiomaticness.
>
> > I'm not talking about the name of the type, but about the name of the
> > member in the struct fpsimd_last_state_struct. I'd like it to be
> > homogeneous to the name you use in struct kvm_vcpu_arch. 'type' is way
>
> Ah, sure I can do that. I had thought this being in the FP last state
> structure made things clear here.
>
> > > > > - thread_sm_enabled(>thread))
> > > > > + thread_sm_enabled(>thread)) {
> > > > > sve_to_fpsimd(task);
> > > > > + task->thread.fp_type = FP_STATE_FPSIMD;
>
> > > > Can you move this assignment into the sve_to_fpsimd() helper?
>
> > > There are cases where we want a FPSIMD version of the state for
> > > reading but don't want to affect the actual state of the process
> > > (eg, if someone reads the FPSIMD registers via ptrace) so we don't
> > > want to change the active register state just because we converted
> > > it. Adding another API that does the convert and update didn't feel
> > > like it was helping since you then have to remember which API does
> > > what and we already have lots of similarly named functions for
> > > slightly different contexts.
>
> > I still think the state conversion should be self contained.
> > Sprinkling this context tracking is bound to end-up with a bug, while
> > documenting what is to be used when, or with a helper named
> > explicitly enough ("extract_fp_from_sve()" springs to mind) for
> > ptrace.
>
> My experience trying to follow and update this code has been that
> layering on more helpers just shifts the potential for bugs around -
> it's easy to have the calling context using the wrong helper and looking
> correct, or to spend time cross checking if the helper in a particular
> context is the right one. Sometimes this happens because something
> about the calling context changed rather than due to writing a new use.
> Yes, someone might forget to update the state type but my experience
> with this code is that it's a lot easier to spot "this is writing new
> state, did it update the state type?" than "this is writing new state,
> did it call the helper that implicitly updates the state type or the
> other one?".
My experience in maintaining the KVM code is that the least state
leaks outside of this sort of helpers, the least problematic they
are. I'd rather have multiple helpers that have different *documented*
behaviours than expecting the random hacker to know (or in this case,
*guess*) when or not to add some extra state-twiddling. It also makes
the code far more readable because it is self-contained.
If this series is supposed to help making things more maintainable,
then this is one way to do it.
M.
--
Without deviation from the norm, progress is not possible.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 2/7] arm64/fpsimd: Track the saved FPSIMD state type separately to TIF_SVE
On Mon, Jul 11, 2022 at 03:33:51PM +0100, Marc Zyngier wrote:
> Mark Brown wrote:
> > On Mon, Jul 11, 2022 at 10:40:50AM +0100, Marc Zyngier wrote:
> > > Mark Brown wrote:
> > > > + enum fp_state *type;
> > > For consistency: s/type/fp_type/ ?
> > Sure if nobody else wants a different bikeshed. It really needs a
> > longer name like fp_state_t or something but that had it's own problems
> > with non-idiomaticness.
> I'm not talking about the name of the type, but about the name of the
> member in the struct fpsimd_last_state_struct. I'd like it to be
> homogeneous to the name you use in struct kvm_vcpu_arch. 'type' is way
Ah, sure I can do that. I had thought this being in the FP last state
structure made things clear here.
> > > > - thread_sm_enabled(>thread))
> > > > + thread_sm_enabled(>thread)) {
> > > > sve_to_fpsimd(task);
> > > > + task->thread.fp_type = FP_STATE_FPSIMD;
> > > Can you move this assignment into the sve_to_fpsimd() helper?
> > There are cases where we want a FPSIMD version of the state for
> > reading but don't want to affect the actual state of the process
> > (eg, if someone reads the FPSIMD registers via ptrace) so we don't
> > want to change the active register state just because we converted
> > it. Adding another API that does the convert and update didn't feel
> > like it was helping since you then have to remember which API does
> > what and we already have lots of similarly named functions for
> > slightly different contexts.
> I still think the state conversion should be self contained.
> Sprinkling this context tracking is bound to end-up with a bug, while
> documenting what is to be used when, or with a helper named
> explicitly enough ("extract_fp_from_sve()" springs to mind) for
> ptrace.
My experience trying to follow and update this code has been that
layering on more helpers just shifts the potential for bugs around -
it's easy to have the calling context using the wrong helper and looking
correct, or to spend time cross checking if the helper in a particular
context is the right one. Sometimes this happens because something
about the calling context changed rather than due to writing a new use.
Yes, someone might forget to update the state type but my experience
with this code is that it's a lot easier to spot "this is writing new
state, did it update the state type?" than "this is writing new state,
did it call the helper that implicitly updates the state type or the
other one?".
Since these callers are already explicitly peering into the data in one
form or another (like reading or writing the actual register values, and
including for some checking the type information) it seems reasonable
for them to also be doing updates to the type selection explicitly. It
does also make the error handling a little neater, if we are switching
between state types then in the case of error we just leave things using
the old, unmodified state.
signature.asc
Description: PGP signature
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 2/7] arm64/fpsimd: Track the saved FPSIMD state type separately to TIF_SVE
On Mon, 11 Jul 2022 12:39:51 +0100,
Mark Brown wrote:
>
> [1 ]
> On Mon, Jul 11, 2022 at 10:40:50AM +0100, Marc Zyngier wrote:
> > Mark Brown wrote:
>
> > > + enum fp_state *type;
>
> > For consistency: s/type/fp_type/ ?
>
> Sure if nobody else wants a different bikeshed. It really needs a
> longer name like fp_state_t or something but that had it's own problems
> with non-idiomaticness.
I'm not talking about the name of the type, but about the name of the
member in the struct fpsimd_last_state_struct. I'd like it to be
homogeneous to the name you use in struct kvm_vcpu_arch. 'type' is way
too vague a name, and maybe it should be fp_reg_type, as that's what
it actually indicates.
>
> > > if (test_and_clear_tsk_thread_flag(task, TIF_SVE) ||
> > > - thread_sm_enabled(>thread))
> > > + thread_sm_enabled(>thread)) {
> > > sve_to_fpsimd(task);
> > > + task->thread.fp_type = FP_STATE_FPSIMD;
>
> > Can you move this assignment into the sve_to_fpsimd() helper?
>
> There are cases where we want a FPSIMD version of the state for
> reading but don't want to affect the actual state of the process
> (eg, if someone reads the FPSIMD registers via ptrace) so we don't
> want to change the active register state just because we converted
> it. Adding another API that does the convert and update didn't feel
> like it was helping since you then have to remember which API does
> what and we already have lots of similarly named functions for
> slightly different contexts.
I still think the state conversion should be self contained.
Sprinkling this context tracking is bound to end-up with a bug, while
documenting what is to be used when, or with a helper named
explicitly enough ("extract_fp_from_sve()" springs to mind) for
ptrace.
> > > } else {
> > > fpsimd_to_sve(current);
> > > + current->thread.fp_type = FP_STATE_SVE;
>
> > Same thing here.
>
> There's not the same issue with reading FPSIMD state via the SVE APIs
> but for consistency it seems best to always leave these updates in the
> callers.
I disagree again. I really want these things to be self-contained, and
be able to reason about what they do from their name and the arguments
they take (and even some documentation). Relying on some extra updates
adds complexity to a difficult part of the kernel, and an even more
difficult part of the architecture.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 2/7] arm64/fpsimd: Track the saved FPSIMD state type separately to TIF_SVE
On Mon, Jul 11, 2022 at 10:40:50AM +0100, Marc Zyngier wrote:
> Mark Brown wrote:
> > + enum fp_state *type;
> For consistency: s/type/fp_type/ ?
Sure if nobody else wants a different bikeshed. It really needs a
longer name like fp_state_t or something but that had it's own problems
with non-idiomaticness.
> > if (test_and_clear_tsk_thread_flag(task, TIF_SVE) ||
> > - thread_sm_enabled(>thread))
> > + thread_sm_enabled(>thread)) {
> > sve_to_fpsimd(task);
> > + task->thread.fp_type = FP_STATE_FPSIMD;
> Can you move this assignment into the sve_to_fpsimd() helper?
There are cases where we want a FPSIMD version of the state for reading
but don't want to affect the actual state of the process (eg, if someone
reads the FPSIMD registers via ptrace) so we don't want to change the
active register state just because we converted it. Adding another API
that does the convert and update didn't feel like it was helping since
you then have to remember which API does what and we already have lots
of similarly named functions for slightly different contexts.
> > } else {
> > fpsimd_to_sve(current);
> > + current->thread.fp_type = FP_STATE_SVE;
> Same thing here.
There's not the same issue with reading FPSIMD state via the SVE APIs
but for consistency it seems best to always leave these updates in the
callers.
> > --- a/arch/arm64/kernel/ptrace.c
> > +++ b/arch/arm64/kernel/ptrace.c
> > @@ -892,8 +892,7 @@ static int sve_set_common(struct task_struct *target,
> > ret = __fpr_set(target, regset, pos, count, kbuf, ubuf,
> > SVE_PT_FPSIMD_OFFSET);
> > clear_tsk_thread_flag(target, TIF_SVE);
> > - if (type == ARM64_VEC_SME)
> > - fpsimd_force_sync_to_sve(target);
> I don't get this particular change. Can you please clarify?
That should probably be shifted to a later patch in the series, I think
I just rebased it to the wrong place.
signature.asc
Description: PGP signature
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 2/7] arm64/fpsimd: Track the saved FPSIMD state type separately to TIF_SVE
On Mon, 20 Jun 2022 13:41:53 +0100,
Mark Brown wrote:
>
> When we save the state for the floating point registers this can be done
> in the form visible through either the FPSIMD V registers or the SVE Z and
> P registers. At present we track which format is currently used based on
> TIF_SVE and the SME streaming mode state but particularly in the SVE case
> this limits our options for optimising things, especially around syscalls.
> Introduce a new enum in thread_struct which explicitly states which format
> is active and keep it up to date when we change it.
>
> At present we do not use this state except to verify that it has the
> expected value when loading the state, future patches will introduce
> functional changes.
>
> Signed-off-by: Mark Brown
> ---
> arch/arm64/include/asm/fpsimd.h| 2 +-
> arch/arm64/include/asm/kvm_host.h | 1 +
> arch/arm64/include/asm/processor.h | 6
> arch/arm64/kernel/fpsimd.c | 57 ++
> arch/arm64/kernel/process.c| 2 ++
> arch/arm64/kernel/ptrace.c | 6 ++--
> arch/arm64/kernel/signal.c | 3 ++
> arch/arm64/kvm/fpsimd.c| 3 +-
> 8 files changed, 61 insertions(+), 19 deletions(-)
>
> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
> index 847fd119cdb8..5762419fdcc0 100644
> --- a/arch/arm64/include/asm/fpsimd.h
> +++ b/arch/arm64/include/asm/fpsimd.h
> @@ -61,7 +61,7 @@ extern void fpsimd_kvm_prepare(void);
> extern void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *state,
>void *sve_state, unsigned int sve_vl,
>void *za_state, unsigned int sme_vl,
> - u64 *svcr);
> + u64 *svcr, enum fp_state *type);
>
> extern void fpsimd_flush_task_state(struct task_struct *target);
> extern void fpsimd_save_and_flush_cpu_state(void);
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index de32152cea04..250e23f221c4 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -310,6 +310,7 @@ struct kvm_vcpu_arch {
> void *sve_state;
> unsigned int sve_max_vl;
> u64 svcr;
> + enum fp_state fp_type;
>
> /* Stage 2 paging state used by the hardware on next switch */
> struct kvm_s2_mmu *hw_mmu;
> diff --git a/arch/arm64/include/asm/processor.h
> b/arch/arm64/include/asm/processor.h
> index 9e58749db21d..192986509a8e 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -122,6 +122,11 @@ enum vec_type {
> ARM64_VEC_MAX,
> };
>
> +enum fp_state {
> + FP_STATE_FPSIMD,
> + FP_STATE_SVE,
> +};
> +
> struct cpu_context {
> unsigned long x19;
> unsigned long x20;
> @@ -152,6 +157,7 @@ struct thread_struct {
> struct user_fpsimd_state fpsimd_state;
> } uw;
>
> + enum fp_state fp_type;/* registers FPSIMD or SVE? */
> unsigned intfpsimd_cpu;
> void*sve_state; /* SVE registers, if any */
> void*za_state; /* ZA register, if any */
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index d67e658f1e24..fdb2925becdf 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -125,6 +125,7 @@ struct fpsimd_last_state_struct {
> u64 *svcr;
> unsigned int sve_vl;
> unsigned int sme_vl;
> + enum fp_state *type;
For consistency: s/type/fp_type/ ?
> };
>
> static DEFINE_PER_CPU(struct fpsimd_last_state_struct, fpsimd_last_state);
> @@ -330,15 +331,6 @@ void task_set_vl_onexec(struct task_struct *task, enum
> vec_type type,
> *The task can execute SVE instructions while in userspace without
> *trapping to the kernel.
> *
> - *When stored, Z0-Z31 (incorporating Vn in bits[127:0] or the
> - *corresponding Zn), P0-P15 and FFR are encoded in
> - *task->thread.sve_state, formatted appropriately for vector
> - *length task->thread.sve_vl or, if SVCR.SM is set,
> - *task->thread.sme_vl.
> - *
> - *task->thread.sve_state must point to a valid buffer at least
> - *sve_state_size(task) bytes in size.
> - *
> *During any syscall, the kernel may optionally clear TIF_SVE and
> *discard the vector state except for the FPSIMD subset.
> *
> @@ -348,7 +340,15 @@ void task_set_vl_onexec(struct task_struct *task, enum
> vec_type type,
> *do_sve_acc() to be called, which does some preparation and then
> *sets TIF_SVE.
> *
> - *When stored, FPSIMD registers V0-V31 are encoded in
> + * During any syscall, the kernel may optionally clear TIF_SVE and
> + * discard the vector state except for the FPSIMD subset.
> + *
> + * The data will be stored in one of two formats:
> + *
> + * * FPSIMD only - FP_STATE_FPSIMD:
>
[PATCH v2 2/7] arm64/fpsimd: Track the saved FPSIMD state type separately to TIF_SVE
When we save the state for the floating point registers this can be done
in the form visible through either the FPSIMD V registers or the SVE Z and
P registers. At present we track which format is currently used based on
TIF_SVE and the SME streaming mode state but particularly in the SVE case
this limits our options for optimising things, especially around syscalls.
Introduce a new enum in thread_struct which explicitly states which format
is active and keep it up to date when we change it.
At present we do not use this state except to verify that it has the
expected value when loading the state, future patches will introduce
functional changes.
Signed-off-by: Mark Brown
---
arch/arm64/include/asm/fpsimd.h| 2 +-
arch/arm64/include/asm/kvm_host.h | 1 +
arch/arm64/include/asm/processor.h | 6
arch/arm64/kernel/fpsimd.c | 57 ++
arch/arm64/kernel/process.c| 2 ++
arch/arm64/kernel/ptrace.c | 6 ++--
arch/arm64/kernel/signal.c | 3 ++
arch/arm64/kvm/fpsimd.c| 3 +-
8 files changed, 61 insertions(+), 19 deletions(-)
diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index 847fd119cdb8..5762419fdcc0 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -61,7 +61,7 @@ extern void fpsimd_kvm_prepare(void);
extern void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *state,
void *sve_state, unsigned int sve_vl,
void *za_state, unsigned int sme_vl,
-u64 *svcr);
+u64 *svcr, enum fp_state *type);
extern void fpsimd_flush_task_state(struct task_struct *target);
extern void fpsimd_save_and_flush_cpu_state(void);
diff --git a/arch/arm64/include/asm/kvm_host.h
b/arch/arm64/include/asm/kvm_host.h
index de32152cea04..250e23f221c4 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -310,6 +310,7 @@ struct kvm_vcpu_arch {
void *sve_state;
unsigned int sve_max_vl;
u64 svcr;
+ enum fp_state fp_type;
/* Stage 2 paging state used by the hardware on next switch */
struct kvm_s2_mmu *hw_mmu;
diff --git a/arch/arm64/include/asm/processor.h
b/arch/arm64/include/asm/processor.h
index 9e58749db21d..192986509a8e 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -122,6 +122,11 @@ enum vec_type {
ARM64_VEC_MAX,
};
+enum fp_state {
+ FP_STATE_FPSIMD,
+ FP_STATE_SVE,
+};
+
struct cpu_context {
unsigned long x19;
unsigned long x20;
@@ -152,6 +157,7 @@ struct thread_struct {
struct user_fpsimd_state fpsimd_state;
} uw;
+ enum fp_state fp_type;/* registers FPSIMD or SVE? */
unsigned intfpsimd_cpu;
void*sve_state; /* SVE registers, if any */
void*za_state; /* ZA register, if any */
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index d67e658f1e24..fdb2925becdf 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -125,6 +125,7 @@ struct fpsimd_last_state_struct {
u64 *svcr;
unsigned int sve_vl;
unsigned int sme_vl;
+ enum fp_state *type;
};
static DEFINE_PER_CPU(struct fpsimd_last_state_struct, fpsimd_last_state);
@@ -330,15 +331,6 @@ void task_set_vl_onexec(struct task_struct *task, enum
vec_type type,
*The task can execute SVE instructions while in userspace without
*trapping to the kernel.
*
- *When stored, Z0-Z31 (incorporating Vn in bits[127:0] or the
- *corresponding Zn), P0-P15 and FFR are encoded in
- *task->thread.sve_state, formatted appropriately for vector
- *length task->thread.sve_vl or, if SVCR.SM is set,
- *task->thread.sme_vl.
- *
- *task->thread.sve_state must point to a valid buffer at least
- *sve_state_size(task) bytes in size.
- *
*During any syscall, the kernel may optionally clear TIF_SVE and
*discard the vector state except for the FPSIMD subset.
*
@@ -348,7 +340,15 @@ void task_set_vl_onexec(struct task_struct *task, enum
vec_type type,
*do_sve_acc() to be called, which does some preparation and then
*sets TIF_SVE.
*
- *When stored, FPSIMD registers V0-V31 are encoded in
+ * During any syscall, the kernel may optionally clear TIF_SVE and
+ * discard the vector state except for the FPSIMD subset.
+ *
+ * The data will be stored in one of two formats:
+ *
+ * * FPSIMD only - FP_STATE_FPSIMD:
+ *
+ *When the FPSIMD only state stored task->thread.fp_type is set to
+ *FP_STATE_FPSIMD, the FPSIMD registers V0-V31 are encoded in
*task->thread.uw.fpsimd_state; bits [max : 128] for each of Z0-Z31 are
*logically zero but not stored anywhere; P0-P15 and
