Re: [PATCH v10 08/18] arm64/sve: Refactor user SVE trap maintenance for external use

[Alex Bennée]

Dave Martin  writes:

> In preparation for optimising the way KVM manages switching the
> guest and host FPSIMD state, it is necessary to provide a means for
> code outside arch/arm64/kernel/fpsimd.c to restore the user trap
> configuration for SVE correctly for the current task.
>
> Rather than requiring external code to duplicate the maintenance
> explicitly, this patch wraps moves the trap maintenenace to
> fpsimd_bind_to_cpu(), since it is logically part of the work of
> associating the current task with the cpu.
>
> Because fpsimd_bind_to_cpu() is rather a cryptic name to publish
> alongside fpsimd_bind_state_to_cpu(), the former function is
> renamed to fpsimd_bind_task_to_cpu() to make its purpose more
> explicit.
>
> This patch makes appropriate changes to ensure that
> fpsimd_bind_task_to_cpu() is always called alongside
> task_fpsimd_load(), so that the trap maintenance continues to be
> done in every situation where it was done prior to this patch.
>
> As a side-effect, the metadata updates done by
> fpsimd_bind_task_to_cpu() now change from conditional to
> unconditional in the "already bound" case of sigreturn.  This is
> harmless, and a couple of extra stores on this slow path will not
> impact performance.  I consider this a reasonable price to pay for
> a slightly cleaner interface.
>
> Signed-off-by: Dave Martin 
> Acked-by: Marc Zyngier 
> Acked-by: Catalin Marinas 

In fact the comment I alluded to in 6/18 could be applied in this.

Reviewed-by: Alex Bennée 


> ---
>  arch/arm64/kernel/fpsimd.c | 28 ++--
>  1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 1222491..ba9e7df 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -257,16 +257,6 @@ static void task_fpsimd_load(void)
>  sve_vq_from_vl(current->thread.sve_vl) - 1);
>   else
>   fpsimd_load_state(>thread.uw.fpsimd_state);
> -
> - if (system_supports_sve()) {
> - /* Toggle SVE trapping for userspace if needed */
> - if (test_thread_flag(TIF_SVE))
> - sve_user_enable();
> - else
> - sve_user_disable();
> -
> - /* Serialised by exception return to user */
> - }
>  }
>
>  /*
> @@ -991,7 +981,7 @@ void fpsimd_signal_preserve_current_state(void)
>   * Associate current's FPSIMD context with this cpu
>   * Preemption must be disabled when calling this function.
>   */
> -static void fpsimd_bind_to_cpu(void)
> +static void fpsimd_bind_task_to_cpu(void)
>  {
>   struct fpsimd_last_state_struct *last =
>   this_cpu_ptr(_last_state);
> @@ -999,6 +989,16 @@ static void fpsimd_bind_to_cpu(void)
>   last->st = >thread.uw.fpsimd_state;
>   last->sve_in_use = test_thread_flag(TIF_SVE);
>   current->thread.fpsimd_cpu = smp_processor_id();
> +
> + if (system_supports_sve()) {
> + /* Toggle SVE trapping for userspace if needed */
> + if (test_thread_flag(TIF_SVE))
> + sve_user_enable();
> + else
> + sve_user_disable();
> +
> + /* Serialised by exception return to user */
> + }
>  }
>
>  /*
> @@ -1015,7 +1015,7 @@ void fpsimd_restore_current_state(void)
>
>   if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) {
>   task_fpsimd_load();
> - fpsimd_bind_to_cpu();
> + fpsimd_bind_task_to_cpu();
>   }
>
>   local_bh_enable();
> @@ -1038,9 +1038,9 @@ void fpsimd_update_current_state(struct 
> user_fpsimd_state const *state)
>   fpsimd_to_sve(current);
>
>   task_fpsimd_load();
> + fpsimd_bind_task_to_cpu();
>
> - if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE))
> - fpsimd_bind_to_cpu();
> + clear_thread_flag(TIF_FOREIGN_FPSTATE);
>
>   local_bh_enable();
>  }


--
Alex Bennée
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v10 06/18] arm64: fpsimd: Generalise context saving for non-task contexts

[Alex Bennée]

Dave Martin  writes:

> In preparation for allowing non-task (i.e., KVM vcpu) FPSIMD
> contexts to be handled by the fpsimd common code, this patch adapts
> task_fpsimd_save() to save back the currently loaded context,
> removing the explicit dependency on current.
>
> The relevant storage to write back to in memory is now found by
> examining the fpsimd_last_state percpu struct.
>
> fpsimd_save() does nothing unless TIF_FOREIGN_FPSTATE is clear, and
> fpsimd_last_state is updated under local_bh_disable() or
> local_irq_disable() everywhere that TIF_FOREIGN_FPSTATE is cleared:
> thus, fpsimd_save() will write back to the correct storage for the
> loaded context.
>
> No functional change.
>
> Signed-off-by: Dave Martin 
> Acked-by: Marc Zyngier 
> Acked-by: Catalin Marinas 
> ---
>  arch/arm64/kernel/fpsimd.c | 25 +
>  1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 9d85373..3aa100a 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -270,13 +270,15 @@ static void task_fpsimd_load(void)
>  }
>
>  /*
> - * Ensure current's FPSIMD/SVE storage in thread_struct is up to date
> - * with respect to the CPU registers.
> + * Ensure FPSIMD/SVE storage in memory for the loaded context is up to
> + * date with respect to the CPU registers.
>   *
>   * Softirqs (and preemption) must be disabled.
>   */
> -static void task_fpsimd_save(void)
> +static void fpsimd_save(void)
>  {
> + struct user_fpsimd_state *st = __this_cpu_read(fpsimd_last_state.st);
> +

I thought I was missing something but the only write I saw of this was:

  __this_cpu_write(fpsimd_last_state.st, NULL);

which implied to me it is possible to have an invalid de-reference. I
did figure it out eventually as fpsimd_bind_state_to_cpu uses a more
indirect this_cpu_ptr idiom for tweaking this. I guess a reference to
fpsimd_bind_[task|state]_to_cpu in the comment would have helped my
confusion.

Anyway:

Reviewed-by: Alex Bennée 


>   WARN_ON(!in_softirq() && !irqs_disabled());
>
>   if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) {
> @@ -291,10 +293,9 @@ static void task_fpsimd_save(void)
>   return;
>   }
>
> - sve_save_state(sve_pffr(current),
> ->thread.uw.fpsimd_state.fpsr);
> + sve_save_state(sve_pffr(current), >fpsr);
>   } else
> - fpsimd_save_state(>thread.uw.fpsimd_state);
> + fpsimd_save_state(st);
>   }
>  }
>
> @@ -598,7 +599,7 @@ int sve_set_vector_length(struct task_struct *task,
>   if (task == current) {
>   local_bh_disable();
>
> - task_fpsimd_save();
> + fpsimd_save();
>   set_thread_flag(TIF_FOREIGN_FPSTATE);
>   }
>
> @@ -837,7 +838,7 @@ asmlinkage void do_sve_acc(unsigned int esr, struct 
> pt_regs *regs)
>
>   local_bh_disable();
>
> - task_fpsimd_save();
> + fpsimd_save();
>   fpsimd_to_sve(current);
>
>   /* Force ret_to_user to reload the registers: */
> @@ -898,7 +899,7 @@ void fpsimd_thread_switch(struct task_struct *next)
>* 'current'.
>*/
>   if (current->mm)
> - task_fpsimd_save();
> + fpsimd_save();
>
>   if (next->mm) {
>   /*
> @@ -980,7 +981,7 @@ void fpsimd_preserve_current_state(void)
>   return;
>
>   local_bh_disable();
> - task_fpsimd_save();
> + fpsimd_save();
>   local_bh_enable();
>  }
>
> @@ -1121,7 +1122,7 @@ void kernel_neon_begin(void)
>
>   /* Save unsaved task fpsimd state, if any: */
>   if (current->mm)
> - task_fpsimd_save();
> + fpsimd_save();
>
>   /* Invalidate any task state remaining in the fpsimd regs: */
>   fpsimd_flush_cpu_state();
> @@ -1244,7 +1245,7 @@ static int fpsimd_cpu_pm_notifier(struct notifier_block 
> *self,
>   switch (cmd) {
>   case CPU_PM_ENTER:
>   if (current->mm)
> - task_fpsimd_save();
> + fpsimd_save();
>   fpsimd_flush_cpu_state();
>   break;
>   case CPU_PM_EXIT:


--
Alex Bennée
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 09/14] ARM: spectre-v2: add PSCI based hardening

[Russell King - ARM Linux]

On Tue, May 22, 2018 at 06:24:13PM +0100, Marc Zyngier wrote:
> On 21/05/18 12:45, Russell King wrote:
> > +#ifdef CONFIG_ARM_PSCI
> > +   if (psci_ops.smccc_version != SMCCC_VERSION_1_0) {
> > +   struct arm_smccc_res res;
> > +
> > +   switch (psci_ops.conduit) {
> > +   case PSCI_CONDUIT_HVC:
> > +   arm_smccc_1_1_hvc(ARM_SMCCC_ARCH_FEATURES_FUNC_ID,
> > + ARM_SMCCC_ARCH_WORKAROUND_1, );
> > +   if ((int)res.a0 < 0)
> > +   break;
> 
> I just realised that there is a small, but significant difference
> between this and the arm64 version: On arm64, we have a table of
> vulnerable implementations, and we try the mitigation on a per-cpu
> basis. Here, you entirely rely on the firmware to discover whether the
> CPU needs mitigation or not. You then need to check for a return value
> of 1, which indicates that although the mitigation is implemented, it is
> not required on this particular CPU.

Okay, so digging further into the documentation seems to suggest that we
only need to check the firmware for A72 and A57 CPUs, and given this
statement:

"Arm recommends that the caller only call this on PEs for which a
 firmware based mitigation of CVE-2017-5715 is required, or where
 a local workaround is infeasible."

it seems that the right answer is to ignore the PSCI based methods when
we have anything but these CPUs.  Do you agree?

> But that's probably moot if you don't support BL systems.

Any bL systems with A72 or A57?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v10 05/18] KVM: arm64: Convert lazy FPSIMD context switch trap to C

[Alex Bennée]

Dave Martin  writes:

> To make the lazy FPSIMD context switch trap code easier to hack on,
> this patch converts it to C.
>
> This is not amazingly efficient, but the trap should typically only
> be taken once per host context switch.
>
> Signed-off-by: Dave Martin 
> Reviewed-by: Marc Zyngier 
> ---
>  arch/arm64/kvm/hyp/entry.S  | 57 
> +
>  arch/arm64/kvm/hyp/switch.c | 24 +++
>  2 files changed, 46 insertions(+), 35 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
> index e41a161..40f349b 100644
> --- a/arch/arm64/kvm/hyp/entry.S
> +++ b/arch/arm64/kvm/hyp/entry.S
> @@ -172,40 +172,27 @@ ENTRY(__fpsimd_guest_restore)
>   // x1: vcpu
>   // x2-x29,lr: vcpu regs
>   // vcpu x0-x1 on the stack
> - stp x2, x3, [sp, #-16]!
> - stp x4, lr, [sp, #-16]!
> -
> -alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
> - mrs x2, cptr_el2
> - bic x2, x2, #CPTR_EL2_TFP
> - msr cptr_el2, x2
> -alternative_else
> - mrs x2, cpacr_el1
> - orr x2, x2, #CPACR_EL1_FPEN
> - msr cpacr_el1, x2
> -alternative_endif
> - isb
> -
> - mov x3, x1
> -
> - ldr x0, [x3, #VCPU_HOST_CONTEXT]
> - kern_hyp_va x0
> - add x0, x0, #CPU_GP_REG_OFFSET(CPU_FP_REGS)
> - bl  __fpsimd_save_state
> -
> - add x2, x3, #VCPU_CONTEXT
> - add x0, x2, #CPU_GP_REG_OFFSET(CPU_FP_REGS)
> - bl  __fpsimd_restore_state
> -
> - // Skip restoring fpexc32 for AArch64 guests
> - mrs x1, hcr_el2
> - tbnzx1, #HCR_RW_SHIFT, 1f
> - ldr x4, [x3, #VCPU_FPEXC32_EL2]
> - msr fpexc32_el2, x4
> -1:
> - ldp x4, lr, [sp], #16
> - ldp x2, x3, [sp], #16
> - ldp x0, x1, [sp], #16
> -
> + stp x2, x3, [sp, #-144]!
> + stp x4, x5, [sp, #16]
> + stp x6, x7, [sp, #32]
> + stp x8, x9, [sp, #48]
> + stp x10, x11, [sp, #64]
> + stp x12, x13, [sp, #80]
> + stp x14, x15, [sp, #96]
> + stp x16, x17, [sp, #112]
> + stp x18, lr, [sp, #128]
> +
> + bl  __hyp_switch_fpsimd
> +
> + ldp x4, x5, [sp, #16]
> + ldp x6, x7, [sp, #32]
> + ldp x8, x9, [sp, #48]
> + ldp x10, x11, [sp, #64]
> + ldp x12, x13, [sp, #80]
> + ldp x14, x15, [sp, #96]
> + ldp x16, x17, [sp, #112]
> + ldp x18, lr, [sp, #128]
> + ldp x0, x1, [sp, #144]
> + ldp x2, x3, [sp], #160
>   eret
>  ENDPROC(__fpsimd_guest_restore)
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index d964523..c0796c4 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -318,6 +318,30 @@ static bool __hyp_text __skip_instr(struct kvm_vcpu 
> *vcpu)
>   }
>  }
>
> +void __hyp_text __hyp_switch_fpsimd(u64 esr __always_unused,
> + struct kvm_vcpu *vcpu)
> +{
> + kvm_cpu_context_t *host_ctxt;
> +
> + if (has_vhe())
> + write_sysreg(read_sysreg(cpacr_el1) | CPACR_EL1_FPEN,
> +  cpacr_el1);
> + else
> + write_sysreg(read_sysreg(cptr_el2) & ~(u64)CPTR_EL2_TFP,
> +  cptr_el2);

Is there no way to do alternative() in C or does it always come down to
different inline asms?

Anyway:

Reviewed-by: Alex Bennée 


> +
> + isb();
> +
> + host_ctxt = kern_hyp_va(vcpu->arch.host_cpu_context);
> + __fpsimd_save_state(_ctxt->gp_regs.fp_regs);
> + __fpsimd_restore_state(>arch.ctxt.gp_regs.fp_regs);
> +
> + /* Skip restoring fpexc32 for AArch64 guests */
> + if (!(read_sysreg(hcr_el2) & HCR_RW))
> + write_sysreg(vcpu->arch.ctxt.sys_regs[FPEXC32_EL2],
> +  fpexc32_el2);
> +}
> +
>  /*
>   * Return true when we were able to fixup the guest exit and should return to
>   * the guest, false when we should restore the host state and return to the


--
Alex Bennée
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] arm64: kvm: use -fno-jump-tables with clang

[Andrey Konovalov]

On Wed, May 23, 2018 at 7:47 PM, Nick Desaulniers
 wrote:
> On Wed, May 23, 2018 at 4:54 AM Andrey Konovalov 
> wrote:
>> On Tue, May 22, 2018 at 8:28 PM, Nick Desaulniers
>>  wrote:
>> > On Fri, May 18, 2018 at 11:13 AM Marc Zyngier 
> wrote:
>> >> > - you have checked that with a released version of the compiler, you
>> >
>> > On Tue, May 22, 2018 at 10:58 AM Andrey Konovalov 
>> > wrote:
>> >> Tested-by: Andrey Konovalov 
>> >
>> > Hi Andrey,
>> > Thank you very much for this report.  Can you confirm as well the
> version
>> > of Clang that you were using?
>
>> I'm on 86852a40 ("[InstCombine] Calloc-ed strings optimizations").
>
>> > If it's not a binary release (built from
>> > source), would you be able to re-confirm with a released version?
>
>> Sure. Which release should I try and how do I get it?
>
> Maybe clang-6.0 as the latest release (though I suspect you may run into
> the recently-fixed-in-clang-7.0 "S" constraint bug that you reported).

Yes, and also into the "support for "r" prefixed variables in ARM
inline assembly" issue.

Tested on upstream commit ded4c39e (before both issues were
introduced) with -fno-jump-tables patch applied using clang 6.0.

Same result, the patch helps.

>
> I've had luck on debian based distributions installing from:
> http://apt.llvm.org/
>
> (These can be added to your /etc/apt/sources.list, then a `sudo apt update`
> and `sudo apt install clang-6.0`)
>
> If you're not able to add remote repositories (some employers block this ;)
> ), then you can find releases for download for a few different platforms:
> https://releases.llvm.org/
>
> For example, a quick:
> $ mkdir llvm-6.0
> $ cd !$
> $ wget
> https://releases.llvm.org/6.0.0/clang+llvm-6.0.0-x86_64-linux-gnu-debian8.tar.xz
> $ tar xvf clang+llvm-6.0.0-x86_64-linux-gnu-debian8.tar.xz
> $ ./clang+llvm-6.0.0-x86_64-linux-gnu-debian8/bin/clang-6.0 -v
> clang version 6.0.0 (tags/RELEASE_600/final)
> Target: x86_64-unknown-linux-gnu
> Thread model: posix
> InstalledDir: .../llvm-6.0/./clang+llvm-6.0.0-x86_64-linux-gnu-debian8/bin
> Found candidate GCC installation: ...
> Candidate multilib: .;@m64
> Selected multilib: .;@m64
>
> Seems to work.
> --
> Thanks,
> ~Nick Desaulniers
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] arm64: kvm: use -fno-jump-tables with clang

[Nick Desaulniers]

On Wed, May 23, 2018 at 4:54 AM Andrey Konovalov 
wrote:
> On Tue, May 22, 2018 at 8:28 PM, Nick Desaulniers
>  wrote:
> > On Fri, May 18, 2018 at 11:13 AM Marc Zyngier 
wrote:
> >> > - you have checked that with a released version of the compiler, you
> >
> > On Tue, May 22, 2018 at 10:58 AM Andrey Konovalov  > wrote:
> >> Tested-by: Andrey Konovalov 
> >
> > Hi Andrey,
> > Thank you very much for this report.  Can you confirm as well the
version
> > of Clang that you were using?

> I'm on 86852a40 ("[InstCombine] Calloc-ed strings optimizations").

> > If it's not a binary release (built from
> > source), would you be able to re-confirm with a released version?

> Sure. Which release should I try and how do I get it?

Maybe clang-6.0 as the latest release (though I suspect you may run into
the recently-fixed-in-clang-7.0 "S" constraint bug that you reported).

I've had luck on debian based distributions installing from:
http://apt.llvm.org/

(These can be added to your /etc/apt/sources.list, then a `sudo apt update`
and `sudo apt install clang-6.0`)

If you're not able to add remote repositories (some employers block this ;)
), then you can find releases for download for a few different platforms:
https://releases.llvm.org/

For example, a quick:
$ mkdir llvm-6.0
$ cd !$
$ wget
https://releases.llvm.org/6.0.0/clang+llvm-6.0.0-x86_64-linux-gnu-debian8.tar.xz
$ tar xvf clang+llvm-6.0.0-x86_64-linux-gnu-debian8.tar.xz
$ ./clang+llvm-6.0.0-x86_64-linux-gnu-debian8/bin/clang-6.0 -v
clang version 6.0.0 (tags/RELEASE_600/final)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: .../llvm-6.0/./clang+llvm-6.0.0-x86_64-linux-gnu-debian8/bin
Found candidate GCC installation: ...
Candidate multilib: .;@m64
Selected multilib: .;@m64

Seems to work.
-- 
Thanks,
~Nick Desaulniers
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v10 07/18] arm64: fpsimd: Eliminate task->mm checks

[Catalin Marinas]

On Wed, May 23, 2018 at 04:03:37PM +0100, Dave P Martin wrote:
> On Wed, May 23, 2018 at 03:56:57PM +0100, Catalin Marinas wrote:
> > On Wed, May 23, 2018 at 02:31:59PM +0100, Dave P Martin wrote:
> > > On Wed, May 23, 2018 at 01:48:12PM +0200, Christoffer Dall wrote:
> > > > On Tue, May 22, 2018 at 05:05:08PM +0100, Dave Martin wrote:
> > > > > This is true by construction however: TIF_FOREIGN_FPSTATE is never
> > > > > cleared except when returning to userspace or returning from a
> > > > > signal: thus, for a true kernel thread no FPSIMD context is ever
> > > > > loaded, TIF_FOREIGN_FPSTATE will remain set and no context will
> > > > > ever be saved.
> > > > 
> > > > I don't understand this construction proof; from looking at the patch
> > > > below it is not obvious to me why fpsimd_thread_switch() can never have
> > > > !wrong_task && !wrong_cpu and therefore clear TIF_FOREIGN_FPSTATE for a
> > > > kernel thread?
> > > 
> > > Looking at this again, I think it is poorly worded.  This patch aims to
> > > make it true by construction, but it isn't prior to the patch.
> > > 
> > > I'm tempted to delete the paragraph: the assertion of both untrue and
> > > not the best way to justify that this patch works.
> > > 
> > > 
> > > How about:
> > > 
> > > -8<-
> > > 
> > > The context switch logic already isolates user threads from each other.
> > > This, it is sufficient for isolating user threads from the kernel,
> > > since the goal either way is to ensure that code executing in userspace
> > > cannot see any FPSIMD state except its own.  Thus, there is no special
> > > property of kernel threads that we care about except that it is
> > > pointless to save or load FPSIMD register state for them.
> > > 
> > > At worst, the removal of all the kernel thread special cases by this
> > > patch would thus spuriously load and save state for kernel threads when
> > > unnecessary.
> > > 
> > > But the context switch logic is already deliberately optimised to defer
> > > reloads of the regs until ret_to_user (or sigreturn as a special case),
> > > which kernel threads by definition never reach.
> > > 
> > > ->8-
> > 
> > The "at worst" paragraph makes it look like it could happen (at least
> > until you reach the last paragraph). Maybe you can just say that
> > wrong_task and wrong_cpu (with the fpsimd_cpu = NR_CPUS addition) are
> > always true for kernel threads. You should probably mention this in a
> > comment in the code as well.
> 
> What if I just delete the second paragraph, and remove the "But" from
> the start of the third, and append:
> 
> "As a result, the wrong_task and wrong_cpu tests in
> fpsimd_thread_switch() will always yield false for kernel threads."
> 
> ...with a similar comment in the code?

Sounds fine. With that, feel free to add:

Reviewed-by: Catalin Marinas 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v10 07/18] arm64: fpsimd: Eliminate task->mm checks

[Dave Martin]

On Wed, May 23, 2018 at 03:56:57PM +0100, Catalin Marinas wrote:
> On Wed, May 23, 2018 at 02:31:59PM +0100, Dave P Martin wrote:
> > On Wed, May 23, 2018 at 01:48:12PM +0200, Christoffer Dall wrote:
> > > On Tue, May 22, 2018 at 05:05:08PM +0100, Dave Martin wrote:
> > > > This is true by construction however: TIF_FOREIGN_FPSTATE is never
> > > > cleared except when returning to userspace or returning from a
> > > > signal: thus, for a true kernel thread no FPSIMD context is ever
> > > > loaded, TIF_FOREIGN_FPSTATE will remain set and no context will
> > > > ever be saved.
> > > 
> > > I don't understand this construction proof; from looking at the patch
> > > below it is not obvious to me why fpsimd_thread_switch() can never have
> > > !wrong_task && !wrong_cpu and therefore clear TIF_FOREIGN_FPSTATE for a
> > > kernel thread?
> > 
> > Looking at this again, I think it is poorly worded.  This patch aims to
> > make it true by construction, but it isn't prior to the patch.
> > 
> > I'm tempted to delete the paragraph: the assertion of both untrue and
> > not the best way to justify that this patch works.
> > 
> > 
> > How about:
> > 
> > -8<-
> > 
> > The context switch logic already isolates user threads from each other.
> > This, it is sufficient for isolating user threads from the kernel,
> > since the goal either way is to ensure that code executing in userspace
> > cannot see any FPSIMD state except its own.  Thus, there is no special
> > property of kernel threads that we care about except that it is
> > pointless to save or load FPSIMD register state for them.
> > 
> > At worst, the removal of all the kernel thread special cases by this
> > patch would thus spuriously load and save state for kernel threads when
> > unnecessary.
> > 
> > But the context switch logic is already deliberately optimised to defer
> > reloads of the regs until ret_to_user (or sigreturn as a special case),
> > which kernel threads by definition never reach.
> > 
> > ->8-
> 
> The "at worst" paragraph makes it look like it could happen (at least
> until you reach the last paragraph). Maybe you can just say that
> wrong_task and wrong_cpu (with the fpsimd_cpu = NR_CPUS addition) are
> always true for kernel threads. You should probably mention this in a
> comment in the code as well.

What if I just delete the second paragraph, and remove the "But" from
the start of the third, and append:

"As a result, the wrong_task and wrong_cpu tests in
fpsimd_thread_switch() will always yield false for kernel threads."

...with a similar comment in the code?

Cheers
---Dave
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v10 14/18] KVM: arm64: Save host SVE context as appropriate

[Catalin Marinas]

On Tue, May 22, 2018 at 05:05:15PM +0100, Dave P Martin wrote:
> This patch adds SVE context saving to the hyp FPSIMD context switch
> path.  This means that it is no longer necessary to save the host
> SVE state in advance of entering the guest, when in use.
> 
> In order to avoid adding pointless complexity to the code, VHE is
> assumed if SVE is in use.  VHE is an architectural prerequisite for
> SVE, so there is no good reason to turn CONFIG_ARM64_VHE off in
> kernels that support both SVE and KVM.
> 
> Historically, software models exist that can expose the
> architecturally invalid configuration of SVE without VHE, so if
> this situation is detected at kvm_init() time then KVM will be
> disabled.
> 
> Signed-off-by: Dave Martin 

Acked-by: Catalin Marinas 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v10 07/18] arm64: fpsimd: Eliminate task->mm checks

[Catalin Marinas]

On Wed, May 23, 2018 at 02:31:59PM +0100, Dave P Martin wrote:
> On Wed, May 23, 2018 at 01:48:12PM +0200, Christoffer Dall wrote:
> > On Tue, May 22, 2018 at 05:05:08PM +0100, Dave Martin wrote:
> > > This is true by construction however: TIF_FOREIGN_FPSTATE is never
> > > cleared except when returning to userspace or returning from a
> > > signal: thus, for a true kernel thread no FPSIMD context is ever
> > > loaded, TIF_FOREIGN_FPSTATE will remain set and no context will
> > > ever be saved.
> > 
> > I don't understand this construction proof; from looking at the patch
> > below it is not obvious to me why fpsimd_thread_switch() can never have
> > !wrong_task && !wrong_cpu and therefore clear TIF_FOREIGN_FPSTATE for a
> > kernel thread?
> 
> Looking at this again, I think it is poorly worded.  This patch aims to
> make it true by construction, but it isn't prior to the patch.
> 
> I'm tempted to delete the paragraph: the assertion of both untrue and
> not the best way to justify that this patch works.
> 
> 
> How about:
> 
> -8<-
> 
> The context switch logic already isolates user threads from each other.
> This, it is sufficient for isolating user threads from the kernel,
> since the goal either way is to ensure that code executing in userspace
> cannot see any FPSIMD state except its own.  Thus, there is no special
> property of kernel threads that we care about except that it is
> pointless to save or load FPSIMD register state for them.
> 
> At worst, the removal of all the kernel thread special cases by this
> patch would thus spuriously load and save state for kernel threads when
> unnecessary.
> 
> But the context switch logic is already deliberately optimised to defer
> reloads of the regs until ret_to_user (or sigreturn as a special case),
> which kernel threads by definition never reach.
> 
> ->8-

The "at worst" paragraph makes it look like it could happen (at least
until you reach the last paragraph). Maybe you can just say that
wrong_task and wrong_cpu (with the fpsimd_cpu = NR_CPUS addition) are
always true for kernel threads. You should probably mention this in a
comment in the code as well.

-- 
Catalin
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v10 04/18] KVM: arm/arm64: Introduce kvm_arch_vcpu_run_pid_change

[Dave Martin]

On Wed, May 23, 2018 at 03:34:20PM +0100, Alex Bennée wrote:
> 
> Dave Martin  writes:
> 
> > From: Christoffer Dall 
> >
> > KVM/ARM differs from other architectures in having to maintain an
> > additional virtual address space from that of the host and the
> > guest, because we split the execution of KVM across both EL1 and
> > EL2.
> >
> > This results in a need to explicitly map data structures into EL2
> > (hyp) which are accessed from the hyp code.  As we are about to be
> > more clever with our FPSIMD handling on arm64, which stores data in
> > the task struct and uses thread_info flags, we will have to map
> > parts of the currently executing task struct into the EL2 virtual
> > address space.
> >
> > However, we don't want to do this on every KVM_RUN, because it is a
> > fairly expensive operation to walk the page tables, and the common
> > execution mode is to map a single thread to a VCPU.  By introducing
> > a hook that architectures can select with
> > HAVE_KVM_VCPU_RUN_PID_CHANGE, we do not introduce overhead for
> > other architectures, but have a simple way to only map the data we
> > need when required for arm64.
> >
> > This patch introduces the framework only, and wires it up in the
> > arm/arm64 KVM common code.
> >
> > No functional change.
> >
> > Signed-off-by: Christoffer Dall 
> > Signed-off-by: Dave Martin 
> > Reviewed-by: Marc Zyngier 
> > ---
> >  include/linux/kvm_host.h | 9 +
> >  virt/kvm/Kconfig | 3 +++
> >  virt/kvm/kvm_main.c  | 7 ++-
> >  3 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index 6930c63..4268ace 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -1276,4 +1276,13 @@ static inline long kvm_arch_vcpu_async_ioctl(struct 
> > file *filp,
> >  void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
> > unsigned long start, unsigned long end);
> >
> > +#ifdef CONFIG_HAVE_KVM_VCPU_RUN_PID_CHANGE
> > +int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu);
> > +#else
> > +static inline int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
> > +{
> > +   return 0;
> > +}
> > +#endif /* CONFIG_HAVE_KVM_VCPU_RUN_PID_CHANGE */
> > +
> >  #endif
> > diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
> > index cca7e06..72143cf 100644
> > --- a/virt/kvm/Kconfig
> > +++ b/virt/kvm/Kconfig
> > @@ -54,3 +54,6 @@ config HAVE_KVM_IRQ_BYPASS
> >
> >  config HAVE_KVM_VCPU_ASYNC_IOCTL
> > bool
> > +
> > +config HAVE_KVM_VCPU_RUN_PID_CHANGE
> > +   bool
> 
> This almost threw me as I thought you might be able to enable this and
> break the build, but apparently not:
> 
> Reviewed-by: Alex Bennée 

Without a "help", the option seems non-interactive and cannot be true
unless something selects it.  It seems a bit weird to me too, but the
idiom appears widely used...

Christoffer?

[...]

Cheers
---Dave

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v10 02/18] thread_info: Add update_thread_flag() helpers

[Alex Bennée]

Dave Martin  writes:

> On Wed, May 23, 2018 at 02:46:52PM +0100, Alex Bennée wrote:
>>
>> Dave Martin  writes:
>>
>> > There are a number of bits of code sprinkled around the kernel to
>> > set a thread flag if a certain condition is true, and clear it
>> > otherwise.
>> >
>> > To help make those call sites terser and less cumbersome, this
>> > patch adds a new family of thread flag manipulators
>> >
>> >update*_thread_flag([...,] flag, cond)
>> >
>> > which do the equivalent of:
>> >
>> >if (cond)
>> >set*_thread_flag([...,] flag);
>> >else
>> >clear*_thread_flag([...,] flag);
>> >
>> > Signed-off-by: Dave Martin 
>> > Acked-by: Steven Rostedt (VMware) 
>> > Acked-by: Marc Zyngier 
>> > Acked-by: Catalin Marinas 
>> > Cc: Ingo Molnar 
>> > Cc: Peter Zijlstra 
>> > Cc: Oleg Nesterov 
>> > ---
>> >  include/linux/sched.h   |  6 ++
>> >  include/linux/thread_info.h | 11 +++
>> >  2 files changed, 17 insertions(+)
>> >
>
> [...]
>
>> > diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
>> > index cf2862b..8d8821b 100644
>> > --- a/include/linux/thread_info.h
>> > +++ b/include/linux/thread_info.h
>> > @@ -60,6 +60,15 @@ static inline void clear_ti_thread_flag(struct 
>> > thread_info *ti, int flag)
>> >clear_bit(flag, (unsigned long *)>flags);
>> >  }
>> >
>> > +static inline void update_ti_thread_flag(struct thread_info *ti, int flag,
>> > +   bool value)
>> > +{
>> > +  if (value)
>> > +  set_ti_thread_flag(ti, flag);
>> > +  else
>> > +  clear_ti_thread_flag(ti, flag);
>> > +}
>> > +
>>
>> value does seem a bit of vanilla non-informative name for a condition
>> flag but whatever:
>>
>> Reviewed-by: Alex Bennée 
>
> I guess, though I couldn't some up with an obviously better name.
>
> I support "condition" would have worked, but it's more verbose.

Well as you use cond in the text I think cond would also work as an
abbreviated variable name. But its a minor nit ;-)

>
> Thanks for the review
> ---Dave


--
Alex Bennée
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v10 04/18] KVM: arm/arm64: Introduce kvm_arch_vcpu_run_pid_change

[Alex Bennée]

Dave Martin  writes:

> From: Christoffer Dall 
>
> KVM/ARM differs from other architectures in having to maintain an
> additional virtual address space from that of the host and the
> guest, because we split the execution of KVM across both EL1 and
> EL2.
>
> This results in a need to explicitly map data structures into EL2
> (hyp) which are accessed from the hyp code.  As we are about to be
> more clever with our FPSIMD handling on arm64, which stores data in
> the task struct and uses thread_info flags, we will have to map
> parts of the currently executing task struct into the EL2 virtual
> address space.
>
> However, we don't want to do this on every KVM_RUN, because it is a
> fairly expensive operation to walk the page tables, and the common
> execution mode is to map a single thread to a VCPU.  By introducing
> a hook that architectures can select with
> HAVE_KVM_VCPU_RUN_PID_CHANGE, we do not introduce overhead for
> other architectures, but have a simple way to only map the data we
> need when required for arm64.
>
> This patch introduces the framework only, and wires it up in the
> arm/arm64 KVM common code.
>
> No functional change.
>
> Signed-off-by: Christoffer Dall 
> Signed-off-by: Dave Martin 
> Reviewed-by: Marc Zyngier 
> ---
>  include/linux/kvm_host.h | 9 +
>  virt/kvm/Kconfig | 3 +++
>  virt/kvm/kvm_main.c  | 7 ++-
>  3 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 6930c63..4268ace 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1276,4 +1276,13 @@ static inline long kvm_arch_vcpu_async_ioctl(struct 
> file *filp,
>  void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
>   unsigned long start, unsigned long end);
>
> +#ifdef CONFIG_HAVE_KVM_VCPU_RUN_PID_CHANGE
> +int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu);
> +#else
> +static inline int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
> +{
> + return 0;
> +}
> +#endif /* CONFIG_HAVE_KVM_VCPU_RUN_PID_CHANGE */
> +
>  #endif
> diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
> index cca7e06..72143cf 100644
> --- a/virt/kvm/Kconfig
> +++ b/virt/kvm/Kconfig
> @@ -54,3 +54,6 @@ config HAVE_KVM_IRQ_BYPASS
>
>  config HAVE_KVM_VCPU_ASYNC_IOCTL
> bool
> +
> +config HAVE_KVM_VCPU_RUN_PID_CHANGE
> +   bool

This almost threw me as I thought you might be able to enable this and
break the build, but apparently not:

Reviewed-by: Alex Bennée 


> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index c7b2e92..c32e240 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2550,8 +2550,13 @@ static long kvm_vcpu_ioctl(struct file *filp,
>   oldpid = rcu_access_pointer(vcpu->pid);
>   if (unlikely(oldpid != current->pids[PIDTYPE_PID].pid)) {
>   /* The thread running this VCPU changed. */
> - struct pid *newpid = get_task_pid(current, PIDTYPE_PID);
> + struct pid *newpid;
>
> + r = kvm_arch_vcpu_run_pid_change(vcpu);
> + if (r)
> + break;
> +
> + newpid = get_task_pid(current, PIDTYPE_PID);
>   rcu_assign_pointer(vcpu->pid, newpid);
>   if (oldpid)
>   synchronize_rcu();


--
Alex Bennée
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v10 02/18] thread_info: Add update_thread_flag() helpers

[Dave Martin]

On Wed, May 23, 2018 at 02:46:52PM +0100, Alex Bennée wrote:
> 
> Dave Martin  writes:
> 
> > There are a number of bits of code sprinkled around the kernel to
> > set a thread flag if a certain condition is true, and clear it
> > otherwise.
> >
> > To help make those call sites terser and less cumbersome, this
> > patch adds a new family of thread flag manipulators
> >
> > update*_thread_flag([...,] flag, cond)
> >
> > which do the equivalent of:
> >
> > if (cond)
> > set*_thread_flag([...,] flag);
> > else
> > clear*_thread_flag([...,] flag);
> >
> > Signed-off-by: Dave Martin 
> > Acked-by: Steven Rostedt (VMware) 
> > Acked-by: Marc Zyngier 
> > Acked-by: Catalin Marinas 
> > Cc: Ingo Molnar 
> > Cc: Peter Zijlstra 
> > Cc: Oleg Nesterov 
> > ---
> >  include/linux/sched.h   |  6 ++
> >  include/linux/thread_info.h | 11 +++
> >  2 files changed, 17 insertions(+)
> >

[...]

> > diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
> > index cf2862b..8d8821b 100644
> > --- a/include/linux/thread_info.h
> > +++ b/include/linux/thread_info.h
> > @@ -60,6 +60,15 @@ static inline void clear_ti_thread_flag(struct 
> > thread_info *ti, int flag)
> > clear_bit(flag, (unsigned long *)>flags);
> >  }
> >
> > +static inline void update_ti_thread_flag(struct thread_info *ti, int flag,
> > +bool value)
> > +{
> > +   if (value)
> > +   set_ti_thread_flag(ti, flag);
> > +   else
> > +   clear_ti_thread_flag(ti, flag);
> > +}
> > +
> 
> value does seem a bit of vanilla non-informative name for a condition
> flag but whatever:
> 
> Reviewed-by: Alex Bennée 

I guess, though I couldn't some up with an obviously better name.

I support "condition" would have worked, but it's more verbose.

Thanks for the review
---Dave
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v10 03/18] arm64: Use update{,_tsk}_thread_flag()

[Alex Bennée]

Dave Martin  writes:

> This patch uses the new update_thread_flag() helpers to simplify a
> couple of if () set; else clear; constructs.
>
> No functional change.
>
> Signed-off-by: Dave Martin 
> Acked-by: Marc Zyngier 
> Acked-by: Catalin Marinas 
> Cc: Will Deacon 

Reviewed-by: Alex Bennée 

> ---
>  arch/arm64/kernel/fpsimd.c | 18 --
>  1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 12e1c96..9d85373 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -618,10 +618,8 @@ int sve_set_vector_length(struct task_struct *task,
>   task->thread.sve_vl = vl;
>
>  out:
> - if (flags & PR_SVE_VL_INHERIT)
> - set_tsk_thread_flag(task, TIF_SVE_VL_INHERIT);
> - else
> - clear_tsk_thread_flag(task, TIF_SVE_VL_INHERIT);
> + update_tsk_thread_flag(task, TIF_SVE_VL_INHERIT,
> +flags & PR_SVE_VL_INHERIT);
>
>   return 0;
>  }
> @@ -910,12 +908,12 @@ void fpsimd_thread_switch(struct task_struct *next)
>* the TIF_FOREIGN_FPSTATE flag so the state will be loaded
>* upon the next return to userland.
>*/
> - if (__this_cpu_read(fpsimd_last_state.st) ==
> - >thread.uw.fpsimd_state
> - && next->thread.fpsimd_cpu == smp_processor_id())
> - clear_tsk_thread_flag(next, TIF_FOREIGN_FPSTATE);
> - else
> - set_tsk_thread_flag(next, TIF_FOREIGN_FPSTATE);
> + bool wrong_task = __this_cpu_read(fpsimd_last_state.st) !=
> + >thread.uw.fpsimd_state;
> + bool wrong_cpu = next->thread.fpsimd_cpu != smp_processor_id();
> +
> + update_tsk_thread_flag(next, TIF_FOREIGN_FPSTATE,
> +wrong_task || wrong_cpu);
>   }
>  }


--
Alex Bennée
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v10 02/18] thread_info: Add update_thread_flag() helpers

[Alex Bennée]

Dave Martin  writes:

> There are a number of bits of code sprinkled around the kernel to
> set a thread flag if a certain condition is true, and clear it
> otherwise.
>
> To help make those call sites terser and less cumbersome, this
> patch adds a new family of thread flag manipulators
>
>   update*_thread_flag([...,] flag, cond)
>
> which do the equivalent of:
>
>   if (cond)
>   set*_thread_flag([...,] flag);
>   else
>   clear*_thread_flag([...,] flag);
>
> Signed-off-by: Dave Martin 
> Acked-by: Steven Rostedt (VMware) 
> Acked-by: Marc Zyngier 
> Acked-by: Catalin Marinas 
> Cc: Ingo Molnar 
> Cc: Peter Zijlstra 
> Cc: Oleg Nesterov 
> ---
>  include/linux/sched.h   |  6 ++
>  include/linux/thread_info.h | 11 +++
>  2 files changed, 17 insertions(+)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index b3d697f..c2c3051 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1578,6 +1578,12 @@ static inline void clear_tsk_thread_flag(struct 
> task_struct *tsk, int flag)
>   clear_ti_thread_flag(task_thread_info(tsk), flag);
>  }
>
> +static inline void update_tsk_thread_flag(struct task_struct *tsk, int flag,
> +   bool value)
> +{
> + update_ti_thread_flag(task_thread_info(tsk), flag, value);
> +}
> +
>  static inline int test_and_set_tsk_thread_flag(struct task_struct *tsk, int 
> flag)
>  {
>   return test_and_set_ti_thread_flag(task_thread_info(tsk), flag);
> diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
> index cf2862b..8d8821b 100644
> --- a/include/linux/thread_info.h
> +++ b/include/linux/thread_info.h
> @@ -60,6 +60,15 @@ static inline void clear_ti_thread_flag(struct thread_info 
> *ti, int flag)
>   clear_bit(flag, (unsigned long *)>flags);
>  }
>
> +static inline void update_ti_thread_flag(struct thread_info *ti, int flag,
> +  bool value)
> +{
> + if (value)
> + set_ti_thread_flag(ti, flag);
> + else
> + clear_ti_thread_flag(ti, flag);
> +}
> +

value does seem a bit of vanilla non-informative name for a condition
flag but whatever:

Reviewed-by: Alex Bennée 


>  static inline int test_and_set_ti_thread_flag(struct thread_info *ti, int 
> flag)
>  {
>   return test_and_set_bit(flag, (unsigned long *)>flags);
> @@ -79,6 +88,8 @@ static inline int test_ti_thread_flag(struct thread_info 
> *ti, int flag)
>   set_ti_thread_flag(current_thread_info(), flag)
>  #define clear_thread_flag(flag) \
>   clear_ti_thread_flag(current_thread_info(), flag)
> +#define update_thread_flag(flag, value) \
> + update_ti_thread_flag(current_thread_info(), flag, value)
>  #define test_and_set_thread_flag(flag) \
>   test_and_set_ti_thread_flag(current_thread_info(), flag)
>  #define test_and_clear_thread_flag(flag) \


--
Alex Bennée
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v10 01/18] arm64: fpsimd: Fix TIF_FOREIGN_FPSTATE after invalidating cpu regs

[Catalin Marinas]

On Tue, May 22, 2018 at 05:05:02PM +0100, Dave P Martin wrote:
> fpsimd_last_state.st is set to NULL as a way of indicating that
> current's FPSIMD registers are no longer loaded in the cpu.  In
> particular, this is done when the kernel temporarily uses or
> clobbers the FPSIMD registers for its own purposes, as in CPU PM or
> kernel-mode NEON, resulting in them being populated with garbage
> data not belonging to a task.
> 
> Commit 17eed27b02da ("arm64/sve: KVM: Prevent guests from using
> SVE") factors this operation out as a new helper
> fpsimd_flush_cpu_state() to make it clearer what is being done
> here, and on SVE systems this helper is now used, via
> kvm_fpsimd_flush_cpu_state(), to invalidate the registers after KVM
> has run a vcpu.  The reason for this is that KVM does not yet
> understand how to restore the full host SVE registers itself after
> loading the guest FPSIMD context into them.
> 
> This exposes a particular problem: if fpsimd_last_state.st is set
> to NULL without also setting TIF_FOREIGN_FPSTATE, the kernel may
> continue to think that current's FPSIMD registers are live even
> though they have actually been clobbered.
> 
> Prior to the aforementioned commit, the only path where
> fpsimd_last_state.st is set to NULL without setting
> TIF_FOREIGN_FPSTATE is when kernel_neon_begin() is called by a
> kernel thread (where current->mm can be NULL).  This does not
> matter, because the only harm is that at context-switch time
> fpsimd_thread_switch() may unnecessarily save the FPSIMD registers
> back to current's thread_struct (even though kernel threads are not
> considered to have any FPSIMD context of their own and the
> registers will never be reloaded).
> 
> Note that although CPU_PM_ENTER lacks the TIF_FOREIGN_FPSTATE
> setting, every CPU passing through that path must subsequently pass
> through CPU_PM_EXIT before it can re-enter the kernel proper.
> CPU_PM_EXIT sets the flag.
> 
> The sve_flush_cpu_state() function added by commit 17eed27b02da
> also lacks the proper maintenance of TIF_FOREIGN_FPSTATE.  This may
> cause the bits of a host task's SVE registers that do not alias the
> FPSIMD register file to spontaneously appear zeroed if a KVM vcpu
> runs in the same task in the meantime.  Although this effect is
> hidden by the fact that the non-FPSIMD bits of the SVE registers
> are zeroed by a syscall anyway, it is doubtless a bad idea to rely
> on these different code paths interacting correctly under future
> maintenance.
> 
> This patch makes TIF_FOREIGN_FPSTATE an unconditional side-effect
> of fpsimd_flush_cpu_state(), and removes the set_thread_flag()
> calls that become redunant as a result.  This ensures that
> TIF_FOREIGN_FPSTATE cannot remain clear if the FPSIMD state in the
> FPSIMD registers is invalid.
> 
> Signed-off-by: Dave Martin 
> Cc: Catalin Marinas 
> Cc: Will Deacon 
> Cc: Ard Biesheuvel 

Reviewed-by: Catalin Marinas 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v10 01/18] arm64: fpsimd: Fix TIF_FOREIGN_FPSTATE after invalidating cpu regs

[Alex Bennée]

Dave Martin  writes:

> fpsimd_last_state.st is set to NULL as a way of indicating that
> current's FPSIMD registers are no longer loaded in the cpu.  In
> particular, this is done when the kernel temporarily uses or
> clobbers the FPSIMD registers for its own purposes, as in CPU PM or
> kernel-mode NEON, resulting in them being populated with garbage
> data not belonging to a task.

>
> Signed-off-by: Dave Martin 
> Cc: Catalin Marinas 
> Cc: Will Deacon 
> Cc: Ard Biesheuvel 

Reviewed-by: Alex Bennée 

--
Alex Bennée
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v10 07/18] arm64: fpsimd: Eliminate task->mm checks

[Dave Martin]

On Wed, May 23, 2018 at 01:48:12PM +0200, Christoffer Dall wrote:
> On Tue, May 22, 2018 at 05:05:08PM +0100, Dave Martin wrote:
> > Currently the FPSIMD handling code uses the condition task->mm ==
> > NULL as a hint that task has no FPSIMD register context.
> > 
> > The ->mm check is only there to filter out tasks that cannot
> > possibly have FPSIMD context loaded, for optimisation purposes.
> > Also, TIF_FOREIGN_FPSTATE must always be checked anyway before
> > saving FPSIMD context back to memory.  For these reasons, the ->mm
> > checks are not useful, providing that that TIF_FOREIGN_FPSTATE is

Hmmm, "that that".  I'll fix that.

> > maintained in a consistent way for kernel threads.
> > 
> > This is true by construction however: TIF_FOREIGN_FPSTATE is never
> > cleared except when returning to userspace or returning from a
> > signal: thus, for a true kernel thread no FPSIMD context is ever
> > loaded, TIF_FOREIGN_FPSTATE will remain set and no context will
> > ever be saved.
> 
> I don't understand this construction proof; from looking at the patch
> below it is not obvious to me why fpsimd_thread_switch() can never have
> !wrong_task && !wrong_cpu and therefore clear TIF_FOREIGN_FPSTATE for a
> kernel thread?

Looking at this again, I think it is poorly worded.  This patch aims to
make it true by construction, but it isn't prior to the patch.

I'm tempted to delete the paragraph: the assertion of both untrue and
not the best way to justify that this patch works.


How about:

-8<-

The context switch logic already isolates user threads from each other.
This, it is sufficient for isolating user threads from the kernel,
since the goal either way is to ensure that code executing in userspace
cannot see any FPSIMD state except its own.  Thus, there is no special
property of kernel threads that we care about except that it is
pointless to save or load FPSIMD register state for them.

At worst, the removal of all the kernel thread special cases by this
patch would thus spuriously load and save state for kernel threads when
unnecessary.

But the context switch logic is already deliberately optimised to defer
reloads of the regs until ret_to_user (or sigreturn as a special case),
which kernel threads by definition never reach.

->8-


As an aside, I notice that we allow thread.fpsimd_cpu to be initialised
to 0 for the init task.  This means that the wrong_cpu check may yield
false for the init task when it shouldn't, because the init task's
FPSIMD state (which doesn't logically exist) is never loaded anywhere.
But the wrong_task check will always yield true for the init task for
the same reason, so this is just an inconsistency in the code rather
than a bug AFAICT.

copy_thread() calls fpsimd_flush_task_state() to make sure that
wrong_cpu is initially true for new tasks.  We should do the same for
the init task by adding

.fpsimd_cpu = NR_CPUS,

to INIT_THREAD.


Cheers
---Dave

> 
> 
> Thanks,
> -Christoffer
> 
> > 
> > This patch removes the redundant checks and special-case code.
> > 
> > Signed-off-by: Dave Martin 
> > Cc: Catalin Marinas 
> > Cc: Will Deacon 
> > Cc: Ard Biesheuvel 
> > 
> > ---
> > 
> > Changes since v9:
> > 
> >  * New patch.  Introduced during debugging, since the ->mm checks
> >appear bogus and/or redundant, so are likely to be hiding or
> >causing bugs.
> > ---
> >  arch/arm64/include/asm/thread_info.h |  1 +
> >  arch/arm64/kernel/fpsimd.c   | 38 
> > 
> >  2 files changed, 14 insertions(+), 25 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/thread_info.h 
> > b/arch/arm64/include/asm/thread_info.h
> > index 740aa03c..a2ac914 100644
> > --- a/arch/arm64/include/asm/thread_info.h
> > +++ b/arch/arm64/include/asm/thread_info.h
> > @@ -47,6 +47,7 @@ struct thread_info {
> >  
> >  #define INIT_THREAD_INFO(tsk)  
> > \
> >  {  \
> > +   .flags  = _TIF_FOREIGN_FPSTATE, \
> > .preempt_count  = INIT_PREEMPT_COUNT,   \
> > .addr_limit = KERNEL_DS,\
> >  }
> > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> > index 3aa100a..1222491 100644
> > --- a/arch/arm64/kernel/fpsimd.c
> > +++ b/arch/arm64/kernel/fpsimd.c
> > @@ -891,31 +891,21 @@ asmlinkage void do_fpsimd_exc(unsigned int esr, 
> > struct pt_regs *regs)
> >  
> >  void fpsimd_thread_switch(struct task_struct *next)
> >  {
> > +   bool wrong_task, wrong_cpu;
> > +
> > if (!system_supports_fpsimd())
> > return;
> > -   /*
> > -* Save the current FPSIMD state to memory, but only if whatever is in
> > -* the registers is in fact the most recent userland FPSIMD state of
> > -* 'current'.
> > -*/
> > -   if 

Re: [PATCH] arm64: kvm: use -fno-jump-tables with clang

[Andrey Konovalov]

On Tue, May 22, 2018 at 8:28 PM, Nick Desaulniers
 wrote:
> On Fri, May 18, 2018 at 11:13 AM Marc Zyngier  wrote:
>> > - you have checked that with a released version of the compiler, you
>
> On Tue, May 22, 2018 at 10:58 AM Andrey Konovalov 
> wrote:
>> Tested-by: Andrey Konovalov 
>
> Hi Andrey,
> Thank you very much for this report.  Can you confirm as well the version
> of Clang that you were using?

I'm on 86852a40 ("[InstCombine] Calloc-ed strings optimizations").

> If it's not a binary release (built from
> source), would you be able to re-confirm with a released version?

Sure. Which release should I try and how do I get it?
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v10 07/18] arm64: fpsimd: Eliminate task->mm checks

[Christoffer Dall]

On Tue, May 22, 2018 at 05:05:08PM +0100, Dave Martin wrote:
> Currently the FPSIMD handling code uses the condition task->mm ==
> NULL as a hint that task has no FPSIMD register context.
> 
> The ->mm check is only there to filter out tasks that cannot
> possibly have FPSIMD context loaded, for optimisation purposes.
> Also, TIF_FOREIGN_FPSTATE must always be checked anyway before
> saving FPSIMD context back to memory.  For these reasons, the ->mm
> checks are not useful, providing that that TIF_FOREIGN_FPSTATE is
> maintained in a consistent way for kernel threads.
> 
> This is true by construction however: TIF_FOREIGN_FPSTATE is never
> cleared except when returning to userspace or returning from a
> signal: thus, for a true kernel thread no FPSIMD context is ever
> loaded, TIF_FOREIGN_FPSTATE will remain set and no context will
> ever be saved.

I don't understand this construction proof; from looking at the patch
below it is not obvious to me why fpsimd_thread_switch() can never have
!wrong_task && !wrong_cpu and therefore clear TIF_FOREIGN_FPSTATE for a
kernel thread?


Thanks,
-Christoffer

> 
> This patch removes the redundant checks and special-case code.
> 
> Signed-off-by: Dave Martin 
> Cc: Catalin Marinas 
> Cc: Will Deacon 
> Cc: Ard Biesheuvel 
> 
> ---
> 
> Changes since v9:
> 
>  * New patch.  Introduced during debugging, since the ->mm checks
>appear bogus and/or redundant, so are likely to be hiding or
>causing bugs.
> ---
>  arch/arm64/include/asm/thread_info.h |  1 +
>  arch/arm64/kernel/fpsimd.c   | 38 
> 
>  2 files changed, 14 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/thread_info.h 
> b/arch/arm64/include/asm/thread_info.h
> index 740aa03c..a2ac914 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -47,6 +47,7 @@ struct thread_info {
>  
>  #define INIT_THREAD_INFO(tsk)
> \
>  {\
> + .flags  = _TIF_FOREIGN_FPSTATE, \
>   .preempt_count  = INIT_PREEMPT_COUNT,   \
>   .addr_limit = KERNEL_DS,\
>  }
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 3aa100a..1222491 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -891,31 +891,21 @@ asmlinkage void do_fpsimd_exc(unsigned int esr, struct 
> pt_regs *regs)
>  
>  void fpsimd_thread_switch(struct task_struct *next)
>  {
> + bool wrong_task, wrong_cpu;
> +
>   if (!system_supports_fpsimd())
>   return;
> - /*
> -  * Save the current FPSIMD state to memory, but only if whatever is in
> -  * the registers is in fact the most recent userland FPSIMD state of
> -  * 'current'.
> -  */
> - if (current->mm)
> - fpsimd_save();
>  
> - if (next->mm) {
> - /*
> -  * If we are switching to a task whose most recent userland
> -  * FPSIMD state is already in the registers of *this* cpu,
> -  * we can skip loading the state from memory. Otherwise, set
> -  * the TIF_FOREIGN_FPSTATE flag so the state will be loaded
> -  * upon the next return to userland.
> -  */
> - bool wrong_task = __this_cpu_read(fpsimd_last_state.st) !=
> + /* Save unsaved fpsimd state, if any: */
> + fpsimd_save();
> +
> + /* Fix up TIF_FOREIGN_FPSTATE to correctly describe next's state: */
> + wrong_task = __this_cpu_read(fpsimd_last_state.st) !=
>   >thread.uw.fpsimd_state;
> - bool wrong_cpu = next->thread.fpsimd_cpu != smp_processor_id();
> + wrong_cpu = next->thread.fpsimd_cpu != smp_processor_id();
>  
> - update_tsk_thread_flag(next, TIF_FOREIGN_FPSTATE,
> -wrong_task || wrong_cpu);
> - }
> + update_tsk_thread_flag(next, TIF_FOREIGN_FPSTATE,
> +wrong_task || wrong_cpu);
>  }
>  
>  void fpsimd_flush_thread(void)
> @@ -1120,9 +1110,8 @@ void kernel_neon_begin(void)
>  
>   __this_cpu_write(kernel_neon_busy, true);
>  
> - /* Save unsaved task fpsimd state, if any: */
> - if (current->mm)
> - fpsimd_save();
> + /* Save unsaved fpsimd state, if any: */
> + fpsimd_save();
>  
>   /* Invalidate any task state remaining in the fpsimd regs: */
>   fpsimd_flush_cpu_state();
> @@ -1244,8 +1233,7 @@ static int fpsimd_cpu_pm_notifier(struct notifier_block 
> *self,
>  {
>   switch (cmd) {
>   case CPU_PM_ENTER:
> - if (current->mm)
> - fpsimd_save();
> + fpsimd_save();
>   

Re: [PATCH 08/14] arm64: ssbd: Disable mitigation on CPU resume if required by user

[Julien Grall]

Hi,

On 05/22/2018 04:06 PM, Marc Zyngier wrote:

On a system where firmware can dynamically change the state of the
mitigation, the CPU will always come up with the mitigation enabled,
including when coming back from suspend.

If the user has requested "no mitigation" via a command line option,
let's enforce it by calling into the firmware again to disable it.

Signed-off-by: Marc Zyngier 


Reviewed-by: Julien Grall 

Cheers,


---
  arch/arm64/include/asm/cpufeature.h | 6 ++
  arch/arm64/kernel/cpu_errata.c  | 8 
  arch/arm64/kernel/suspend.c | 8 
  3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/cpufeature.h 
b/arch/arm64/include/asm/cpufeature.h
index 1bacdf57f0af..d9dcb683259e 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -553,6 +553,12 @@ static inline int arm64_get_ssbd_state(void)
  #endif
  }
  
+#ifdef CONFIG_ARM64_SSBD

+void arm64_set_ssbd_mitigation(bool state);
+#else
+static inline void arm64_set_ssbd_mitigation(bool state) {}
+#endif
+
  #endif /* __ASSEMBLY__ */
  
  #endif

diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 8f686f39b9c1..b4c12e9140f0 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -297,7 +297,7 @@ void __init arm64_enable_wa2_handling(struct alt_instr *alt,
*updptr = cpu_to_le32(aarch64_insn_gen_nop());
  }
  
-static void do_ssbd(bool state)

+void arm64_set_ssbd_mitigation(bool state)
  {
switch (psci_ops.conduit) {
case PSCI_CONDUIT_HVC:
@@ -371,20 +371,20 @@ static bool has_ssbd_mitigation(const struct 
arm64_cpu_capabilities *entry,
switch (ssbd_state) {
case ARM64_SSBD_FORCE_DISABLE:
pr_info_once("%s disabled from command-line\n", entry->desc);
-   do_ssbd(false);
+   arm64_set_ssbd_mitigation(false);
required = false;
break;
  
  	case ARM64_SSBD_EL1_ENTRY:

if (required) {
__this_cpu_write(arm64_ssbd_callback_required, 1);
-   do_ssbd(true);
+   arm64_set_ssbd_mitigation(true);
}
break;
  
  	case ARM64_SSBD_FORCE_ENABLE:

pr_info_once("%s forced from command-line\n", entry->desc);
-   do_ssbd(true);
+   arm64_set_ssbd_mitigation(true);
required = true;
break;
  
diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c

index a307b9e13392..70c283368b64 100644
--- a/arch/arm64/kernel/suspend.c
+++ b/arch/arm64/kernel/suspend.c
@@ -62,6 +62,14 @@ void notrace __cpu_suspend_exit(void)
 */
if (hw_breakpoint_restore)
hw_breakpoint_restore(cpu);
+
+   /*
+* On resume, firmware implementing dynamic mitigation will
+* have turned the mitigation on. If the user has forcefully
+* disabled it, make sure their wishes are obeyed.
+*/
+   if (arm64_get_ssbd_state() == ARM64_SSBD_FORCE_DISABLE)
+   arm64_set_ssbd_mitigation(false);
  }
  
  /*




--
Julien Grall
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 13/14] ARM: KVM: Add SMCCC_ARCH_WORKAROUND_1 fast handling

[Marc Zyngier]

On 21/05/18 12:45, Russell King wrote:
> We want SMCCC_ARCH_WORKAROUND_1 to be fast. As fast as possible.
> So let's intercept it as early as we can by testing for the
> function call number as soon as we've identified a HVC call
> coming from the guest.
> 
> Signed-off-by: Russell King 
> ---
>  arch/arm/kvm/hyp/hyp-entry.S | 13 -
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kvm/hyp/hyp-entry.S b/arch/arm/kvm/hyp/hyp-entry.S
> index 918a05dd2d63..67de45685e29 100644
> --- a/arch/arm/kvm/hyp/hyp-entry.S
> +++ b/arch/arm/kvm/hyp/hyp-entry.S
> @@ -16,6 +16,7 @@
>   * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
>   */
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -202,7 +203,7 @@ ENDPROC(__hyp_do_panic)
>   lsr r2, r2, #16
>   and r2, r2, #0xff
>   cmp r2, #0
> - bne guest_trap  @ Guest called HVC
> + bne guest_hvc_trap  @ Guest called HVC
>  
>   /*
>* Getting here means host called HVC, we shift parameters and branch
> @@ -253,6 +254,16 @@ THUMB(   orr lr, #1)
>   pop {r2, lr}
>   eret
>  
> +guest_hvc_trap:
> + movwip, #:lower16:ARM_SMCCC_ARCH_WORKAROUND_1
> + movtip, #:upper16:ARM_SMCCC_ARCH_WORKAROUND_1

r12 is a live guest register, and only r0-r2 are saved at that stage.
You could additionally corrupt r3 though, once you've identified that
you're in the context of an SMCCC 1.1 call.

You should be able to replace r12 with r2.

> + ldr r0, [sp]@ Guest's r0
> + teq r0, ip
> + bne guest_trap
> + pop {r0, r1, r2}

You could replace this slightly more efficient

add sp, sp, #12

since we don't need to restore those registers to the guest. r2 would be
left containing ARM_SMCCC_ARCH_WORKAROUND_1 (harmless), and r1 has the
HSR value (perfectly predictable).

> + mov r0, #0
> + eret
> +
>  guest_trap:
>   load_vcpu r0@ Load VCPU pointer to r0
>  
> 

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 07/14] arm64: ssbd: Skip apply_ssbd if not using dynamic mitigation

[Julien Grall]

Hi Marc,

On 05/22/2018 04:06 PM, Marc Zyngier wrote:

In order to avoid checking arm64_ssbd_callback_required on each
kernel entry/exit even if no mitigation is required, let's
add yet another alternative that by default jumps over the mitigation,
and that gets nop'ed out if we're doing dynamic mitigation.

Think of it as a poor man's static key...

Signed-off-by: Marc Zyngier 


Reviewed-by: Julien Grall 

Cheers,



---
  arch/arm64/kernel/cpu_errata.c | 14 ++
  arch/arm64/kernel/entry.S  |  3 +++
  2 files changed, 17 insertions(+)

diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index f1d4e75b0ddd..8f686f39b9c1 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -283,6 +283,20 @@ void __init arm64_update_smccc_conduit(struct alt_instr 
*alt,
*updptr = cpu_to_le32(insn);
  }
  
+void __init arm64_enable_wa2_handling(struct alt_instr *alt,

+ __le32 *origptr, __le32 *updptr,
+ int nr_inst)
+{
+   BUG_ON(nr_inst != 1);
+   /*
+* Only allow mitigation on EL1 entry/exit and guest
+* ARCH_WORKAROUND_2 handling if the SSBD state allows it to
+* be flipped.
+*/
+   if (arm64_get_ssbd_state() == ARM64_SSBD_EL1_ENTRY)
+   *updptr = cpu_to_le32(aarch64_insn_gen_nop());
+}
+
  static void do_ssbd(bool state)
  {
switch (psci_ops.conduit) {
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 29ad672a6abd..e6f6e2339b22 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -142,6 +142,9 @@ alternative_else_nop_endif
// to save/restore them if required.
.macro  apply_ssbd, state, targ, tmp1, tmp2
  #ifdef CONFIG_ARM64_SSBD
+alternative_cb arm64_enable_wa2_handling
+   b   \targ
+alternative_cb_end
ldr_this_cpu\tmp2, arm64_ssbd_callback_required, \tmp1
cbz \tmp2, \targ
mov w0, #ARM_SMCCC_ARCH_WORKAROUND_2



--
Julien Grall
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 06/14] arm64: ssbd: Add global mitigation state accessor

[Julien Grall]

Hi Marc,

On 05/22/2018 04:06 PM, Marc Zyngier wrote:

We're about to need the mitigation state in various parts of the
kernel in order to do the right thing for userspace and guests.

Let's expose an accessor that will let other subsystems know
about the state.

Signed-off-by: Marc Zyngier 


Reviewed-by: Julien Grall 

Cheers,


---
  arch/arm64/include/asm/cpufeature.h | 10 ++
  1 file changed, 10 insertions(+)

diff --git a/arch/arm64/include/asm/cpufeature.h 
b/arch/arm64/include/asm/cpufeature.h
index 9bc548e22784..1bacdf57f0af 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -543,6 +543,16 @@ static inline u64 read_zcr_features(void)
  #define ARM64_SSBD_FORCE_ENABLE   2
  #define ARM64_SSBD_MITIGATED  3
  
+static inline int arm64_get_ssbd_state(void)

+{
+#ifdef CONFIG_ARM64_SSBD
+   extern int ssbd_state;
+   return ssbd_state;
+#else
+   return ARM64_SSBD_UNKNOWN;
+#endif
+}
+
  #endif /* __ASSEMBLY__ */
  
  #endif




--
Julien Grall
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 05/14] arm64: Add 'ssbd' command-line option

[Julien Grall]

Hi Marc,

On 05/22/2018 04:06 PM, Marc Zyngier wrote:

On a system where the firmware implements ARCH_WORKAROUND_2,
it may be useful to either permanently enable or disable the
workaround for cases where the user decides that they'd rather
not get a trap overhead, and keep the mitigation permanently
on or off instead of switching it on exception entry/exit.

In any case, default to the mitigation being enabled.

Signed-off-by: Marc Zyngier 


Reviewed-by: Julien Grall 

Cheers,


---
  Documentation/admin-guide/kernel-parameters.txt |  17 
  arch/arm64/include/asm/cpufeature.h |   6 ++
  arch/arm64/kernel/cpu_errata.c  | 102 
  3 files changed, 109 insertions(+), 16 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index f2040d46f095..646e112c6f63 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4092,6 +4092,23 @@
expediting.  Set to zero to disable automatic
expediting.
  
+	ssbd=		[ARM64,HW]

+   Speculative Store Bypass Disable control
+
+   On CPUs that are vulnerable to the Speculative
+   Store Bypass vulnerability and offer a
+   firmware based mitigation, this parameter
+   indicates how the mitigation should be used:
+
+   force-on:  Unconditionnaly enable mitigation for
+  for both kernel and userspace
+   force-off: Unconditionnaly disable mitigation for
+  for both kernel and userspace
+   kernel:Always enable mitigation in the
+  kernel, and offer a prctl interface
+  to allow userspace to register its
+  interest in being mitigated too.
+
stack_guard_gap=[MM]
override the default stack gap protection. The value
is in page units and it defines how many pages prior
diff --git a/arch/arm64/include/asm/cpufeature.h 
b/arch/arm64/include/asm/cpufeature.h
index 09b0f2a80c8f..9bc548e22784 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -537,6 +537,12 @@ static inline u64 read_zcr_features(void)
return zcr;
  }
  
+#define ARM64_SSBD_UNKNOWN		-1

+#define ARM64_SSBD_FORCE_DISABLE   0
+#define ARM64_SSBD_EL1_ENTRY   1
+#define ARM64_SSBD_FORCE_ENABLE2
+#define ARM64_SSBD_MITIGATED   3
+
  #endif /* __ASSEMBLY__ */
  
  #endif

diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 7fd6d5b001f5..f1d4e75b0ddd 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -235,6 +235,38 @@ enable_smccc_arch_workaround_1(const struct 
arm64_cpu_capabilities *entry)
  #ifdef CONFIG_ARM64_SSBD
  DEFINE_PER_CPU_READ_MOSTLY(u64, arm64_ssbd_callback_required);
  
+int ssbd_state __read_mostly = ARM64_SSBD_EL1_ENTRY;

+
+static const struct ssbd_options {
+   const char  *str;
+   int state;
+} ssbd_options[] = {
+   { "force-on", ARM64_SSBD_FORCE_ENABLE, },
+   { "force-off",ARM64_SSBD_FORCE_DISABLE, },
+   { "kernel",   ARM64_SSBD_EL1_ENTRY, },
+};
+
+static int __init ssbd_cfg(char *buf)
+{
+   int i;
+
+   if (!buf || !buf[0])
+   return -EINVAL;
+
+   for (i = 0; i < ARRAY_SIZE(ssbd_options); i++) {
+   int len = strlen(ssbd_options[i].str);
+
+   if (strncmp(buf, ssbd_options[i].str, len))
+   continue;
+
+   ssbd_state = ssbd_options[i].state;
+   return 0;
+   }
+
+   return -EINVAL;
+}
+early_param("ssbd", ssbd_cfg);
+
  void __init arm64_update_smccc_conduit(struct alt_instr *alt,
   __le32 *origptr, __le32 *updptr,
   int nr_inst)
@@ -272,44 +304,82 @@ static bool has_ssbd_mitigation(const struct 
arm64_cpu_capabilities *entry,
int scope)
  {
struct arm_smccc_res res;
-   bool supported = true;
+   bool required = true;
+   s32 val;
  
  	WARN_ON(scope != SCOPE_LOCAL_CPU || preemptible());
  
-	if (psci_ops.smccc_version == SMCCC_VERSION_1_0)

+   if (psci_ops.smccc_version == SMCCC_VERSION_1_0) {
+   ssbd_state = ARM64_SSBD_UNKNOWN;
return false;
+   }
  
-	/*

-* The probe function return value is either negative
-* (unsupported or mitigated), positive (unaffected), or zero
-* (requires mitigation). We only need to do anything in the
-* last case.
-

Re: [PATCH 04/14] arm64: Add ARCH_WORKAROUND_2 probing

[Julien Grall]

Hi Marc,

On 05/22/2018 04:06 PM, Marc Zyngier wrote:

As for Spectre variant-2, we rely on SMCCC 1.1 to provide the
discovery mechanism for detecting the SSBD mitigation.

A new capability is also allocated for that purpose, and a
config option.

Signed-off-by: Marc Zyngier 
---
  arch/arm64/Kconfig   |  9 ++
  arch/arm64/include/asm/cpucaps.h |  3 +-
  arch/arm64/kernel/cpu_errata.c   | 69 
  3 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index eb2cf4938f6d..b2103b4df467 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -938,6 +938,15 @@ config HARDEN_EL2_VECTORS
  
  	  If unsure, say Y.
  
+config ARM64_SSBD

+   bool "Speculative Store Bypass Disable" if EXPERT
+   default y
+   help
+ This enables mitigation of the bypassing of previous stores
+ by speculative loads.
+
+ If unsure, say Y.
+
  menuconfig ARMV8_DEPRECATED
bool "Emulate deprecated/obsolete ARMv8 instructions"
depends on COMPAT
diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
index bc51b72fafd4..5b2facf786ba 100644
--- a/arch/arm64/include/asm/cpucaps.h
+++ b/arch/arm64/include/asm/cpucaps.h
@@ -48,7 +48,8 @@
  #define ARM64_HAS_CACHE_IDC   27
  #define ARM64_HAS_CACHE_DIC   28
  #define ARM64_HW_DBM  29
+#define ARM64_SSBD 30


NIT: Could you indent 30 the same way as the other number?

Reviewed-by: Julien Grall 

Cheers,

--
Julien Grall
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 03/14] arm64: Add per-cpu infrastructure to call ARCH_WORKAROUND_2

[Julien Grall]

Hi Marc,

On 05/22/2018 04:06 PM, Marc Zyngier wrote:

In a heterogeneous system, we can end up with both affected and
unaffected CPUs. Let's check their status before calling into the
firmware.

Signed-off-by: Marc Zyngier 


Reviewed-by: Julien Grall 

Cheers,


---
  arch/arm64/kernel/cpu_errata.c |  2 ++
  arch/arm64/kernel/entry.S  | 11 +++
  2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 46b3aafb631a..0288d6cf560e 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -233,6 +233,8 @@ enable_smccc_arch_workaround_1(const struct 
arm64_cpu_capabilities *entry)
  #endif/* CONFIG_HARDEN_BRANCH_PREDICTOR */
  
  #ifdef CONFIG_ARM64_SSBD

+DEFINE_PER_CPU_READ_MOSTLY(u64, arm64_ssbd_callback_required);
+
  void __init arm64_update_smccc_conduit(struct alt_instr *alt,
   __le32 *origptr, __le32 *updptr,
   int nr_inst)
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index f33e6aed3037..29ad672a6abd 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -140,8 +140,10 @@ alternative_else_nop_endif
  
  	// This macro corrupts x0-x3. It is the caller's duty

// to save/restore them if required.
-   .macro  apply_ssbd, state
+   .macro  apply_ssbd, state, targ, tmp1, tmp2
  #ifdef CONFIG_ARM64_SSBD
+   ldr_this_cpu\tmp2, arm64_ssbd_callback_required, \tmp1
+   cbz \tmp2, \targ
mov w0, #ARM_SMCCC_ARCH_WORKAROUND_2
mov w1, #\state
  alternative_cbarm64_update_smccc_conduit
@@ -176,12 +178,13 @@ alternative_cb_end
ldr x19, [tsk, #TSK_TI_FLAGS]   // since we can unmask debug
disable_step_tsk x19, x20   // exceptions when scheduling.
  
-	apply_ssbd 1

+   apply_ssbd 1, 1f, x22, x23
  
  #ifdef CONFIG_ARM64_SSBD

ldp x0, x1, [sp, #16 * 0]
ldp x2, x3, [sp, #16 * 1]
  #endif
+1:
  
  	mov	x29, xzr			// fp pointed to user-space

.else
@@ -323,8 +326,8 @@ alternative_if ARM64_WORKAROUND_845719
  alternative_else_nop_endif
  #endif
  3:
-   apply_ssbd 0
-
+   apply_ssbd 0, 5f, x0, x1
+5:
.endif
  
  	msr	elr_el1, x21			// set up the return data




--
Julien Grall
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 02/14] arm64: Call ARCH_WORKAROUND_2 on transitions between EL0 and EL1

[Julien Grall]

Hi Marc,

On 05/22/2018 04:06 PM, Marc Zyngier wrote:

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index ec2ee720e33e..f33e6aed3037 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -18,6 +18,7 @@
   * along with this program.  If not, see .
   */
  
+#include 

  #include 
  #include 
  
@@ -137,6 +138,18 @@ alternative_else_nop_endif

add \dst, \dst, #(\sym - .entry.tramp.text)
.endm
  
+	// This macro corrupts x0-x3. It is the caller's duty

+   // to save/restore them if required.


NIT: Shouldn't you use /* ... */ for multi-line comments?

Regardless that:

Reviewed-by: Julien Grall 

Cheers,

--
Julien Grall
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCHv4 05/10] arm64/cpufeature: detect pointer authentication

[Suzuki K Poulose]

Mark,

On 03/05/18 14:20, Mark Rutland wrote:

So that we can dynamically handle the presence of pointer authentication
functionality, wire up probing code in cpufeature.c.

 From ARMv8.3 onwards, ID_AA64ISAR1 is no longer entirely RES0, and now
has four fields describing the presence of pointer authentication
functionality:

* APA - address authentication present, using an architected algorithm
* API - address authentication present, using an IMP DEF algorithm
* GPA - generic authentication present, using an architected algorithm
* GPI - generic authentication present, using an IMP DEF algorithm

For the moment we only care about address authentication, so we only
need to check APA and API. It is assumed that if all CPUs support an IMP
DEF algorithm, the same algorithm is used across all CPUs.

Note that when we implement KVM support, we will also need to ensure
that CPUs have uniform support for GPA and GPI.

Signed-off-by: Mark Rutland 
Cc: Catalin Marinas 
Cc: Suzuki K Poulose 
Cc: Will Deacon 
---
  arch/arm64/include/asm/cpucaps.h |  5 -
  arch/arm64/kernel/cpufeature.c   | 47 
  2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
index bc51b72fafd4..9dcb4d1b14f5 100644
--- a/arch/arm64/include/asm/cpucaps.h
+++ b/arch/arm64/include/asm/cpucaps.h
@@ -48,7 +48,10 @@
  #define ARM64_HAS_CACHE_IDC   27
  #define ARM64_HAS_CACHE_DIC   28
  #define ARM64_HW_DBM  29
+#define ARM64_HAS_ADDRESS_AUTH_ARCH30
+#define ARM64_HAS_ADDRESS_AUTH_IMP_DEF 31


Where are these caps used ? I couldn't find anything in the series
that uses them. Otherwise looks good to me.

Cheers

Suzuki

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCHv4 06/10] arm64: add basic pointer authentication support

[Suzuki K Poulose]

Hi Mark,


On 03/05/18 14:20, Mark Rutland wrote:

This patch adds basic support for pointer authentication, allowing
userspace to make use of APIAKey. The kernel maintains an APIAKey value
for each process (shared by all threads within), which is initialised to
a random value at exec() time.

To describe that address authentication instructions are available, the
ID_AA64ISAR0.{APA,API} fields are exposed to userspace. A new hwcap,
APIA, is added to describe that the kernel manages APIAKey.

Instructions using other keys (APIBKey, APDAKey, APDBKey) are disabled,
and will behave as NOPs. These may be made use of in future patches.

No support is added for the generic key (APGAKey), though this cannot be
trapped or made to behave as a NOP. Its presence is not advertised with
a hwcap.

Signed-off-by: Mark Rutland 
Cc: Catalin Marinas 
Cc: Ramana Radhakrishnan 
Cc: Suzuki K Poulose 
Cc: Will Deacon 




diff --git a/arch/arm64/include/asm/pointer_auth.h 
b/arch/arm64/include/asm/pointer_auth.h
new file mode 100644
index ..034877ee28bc
--- /dev/null
+++ b/arch/arm64/include/asm/pointer_auth.h


...


+
+#define __ptrauth_key_install(k, v)\
+do {   \
+   write_sysreg_s(v.lo, SYS_ ## k ## KEYLO_EL1);   \
+   write_sysreg_s(v.hi, SYS_ ## k ## KEYHI_EL1);   \
+} while (0)


I think it might be safer to have parentheses around v, to prevent
something like __ptrauth_key_install(APIA, *key_val) work fine.


diff --git a/arch/arm64/include/uapi/asm/hwcap.h 
b/arch/arm64/include/uapi/asm/hwcap.h
index 17c65c8f33cb..01f02ac500ae 100644
--- a/arch/arm64/include/uapi/asm/hwcap.h
+++ b/arch/arm64/include/uapi/asm/hwcap.h
@@ -48,5 +48,6 @@
  #define HWCAP_USCAT   (1 << 25)
  #define HWCAP_ILRCPC  (1 << 26)
  #define HWCAP_FLAGM   (1 << 27)
+#define HWCAP_APIA (1 << 28)
  
  #endif /* _UAPI__ASM_HWCAP_H */

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 01b1a7e7d70f..f418d4cb6691 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1030,6 +1030,11 @@ static void cpu_copy_el2regs(const struct 
arm64_cpu_capabilities *__unused)
  #endif

...


+#ifdef CONFIG_ARM64_PNTR_AUTH
+   HWCAP_CAP(SYS_ID_AA64ISAR1_EL1, ID_AA64ISAR1_APA_SHIFT, FTR_UNSIGNED, 
1, CAP_HWCAP, HWCAP_APIA),
+#endif


Did you mean CONFIG_ARM64_PTR_AUTH here ?

Cheers

Suzuki
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 09/14] ARM: spectre-v2: add PSCI based hardening

[Marc Zyngier]

On Tue, 22 May 2018 18:57:18 +0100,
Russell King wrote:
> 
> On Tue, May 22, 2018 at 06:24:13PM +0100, Marc Zyngier wrote:
> > On 21/05/18 12:45, Russell King wrote:
> > > Add PSCI based hardening for cores that require more complex handling in
> > > firmware.
> > > 
> > > Signed-off-by: Russell King 
> > > Acked-by: Marc Zyngier 
> > > ---
> > >  arch/arm/mm/proc-v7-bugs.c | 50 
> > > ++
> > >  arch/arm/mm/proc-v7.S  | 21 +++
> > >  2 files changed, 71 insertions(+)
> > > 
> > > diff --git a/arch/arm/mm/proc-v7-bugs.c b/arch/arm/mm/proc-v7-bugs.c
> > > index 65a9b8141f86..0c37e6a2830d 100644
> > > --- a/arch/arm/mm/proc-v7-bugs.c
> > > +++ b/arch/arm/mm/proc-v7-bugs.c
> > > @@ -1,9 +1,12 @@
> > >  // SPDX-License-Identifier: GPL-2.0
> > > +#include 
> > >  #include 
> > > +#include 
> > >  #include 
> > >  
> > >  #include 
> > >  #include 
> > > +#include 
> > >  #include 
> > >  
> > >  void cpu_v7_bugs_init(void);
> > > @@ -39,6 +42,9 @@ void cpu_v7_ca15_ibe(void)
> > >  #ifdef CONFIG_HARDEN_BRANCH_PREDICTOR
> > >  void (*harden_branch_predictor)(void);
> > >  
> > > +extern void cpu_v7_smc_switch_mm(phys_addr_t pgd_phys, struct mm_struct 
> > > *mm);
> > > +extern void cpu_v7_hvc_switch_mm(phys_addr_t pgd_phys, struct mm_struct 
> > > *mm);
> > > +
> > >  static void harden_branch_predictor_bpiall(void)
> > >  {
> > >   write_sysreg(0, BPIALL);
> > > @@ -49,6 +55,18 @@ static void harden_branch_predictor_iciallu(void)
> > >   write_sysreg(0, ICIALLU);
> > >  }
> > >  
> > > +#ifdef CONFIG_ARM_PSCI
> > > +static void call_smc_arch_workaround_1(void)
> > > +{
> > > + arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_1, NULL);
> > > +}
> > > +
> > > +static void call_hvc_arch_workaround_1(void)
> > > +{
> > > + arm_smccc_1_1_hvc(ARM_SMCCC_ARCH_WORKAROUND_1, NULL);
> > > +}
> > > +#endif
> > > +
> > >  void cpu_v7_bugs_init(void)
> > >  {
> > >   const char *spectre_v2_method = NULL;
> > > @@ -73,6 +91,38 @@ void cpu_v7_bugs_init(void)
> > >   spectre_v2_method = "ICIALLU";
> > >   break;
> > >   }
> > > +
> > > +#ifdef CONFIG_ARM_PSCI
> > > + if (psci_ops.smccc_version != SMCCC_VERSION_1_0) {
> > > + struct arm_smccc_res res;
> > > +
> > > + switch (psci_ops.conduit) {
> > > + case PSCI_CONDUIT_HVC:
> > > + arm_smccc_1_1_hvc(ARM_SMCCC_ARCH_FEATURES_FUNC_ID,
> > > +   ARM_SMCCC_ARCH_WORKAROUND_1, );
> > > + if ((int)res.a0 < 0)
> > > + break;
> > 
> > I just realised that there is a small, but significant difference
> > between this and the arm64 version: On arm64, we have a table of
> > vulnerable implementations, and we try the mitigation on a per-cpu
> > basis. Here, you entirely rely on the firmware to discover whether the
> > CPU needs mitigation or not. You then need to check for a return value
> > of 1, which indicates that although the mitigation is implemented, it is
> > not required on this particular CPU.
> > 
> > But that's probably moot if you don't support BL systems.
> > 
> > > + harden_branch_predictor = call_hvc_arch_workaround_1;
> > > + processor.switch_mm = cpu_v7_hvc_switch_mm;
> > > + spectre_v2_method = "hypervisor";
> > > + break;
> > > +
> > > + case PSCI_CONDUIT_SMC:
> > > + arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FUNC_ID,
> > > +   ARM_SMCCC_ARCH_WORKAROUND_1, );
> > > + if ((int)res.a0 < 0)
> > > + break;
> > > + harden_branch_predictor = call_smc_arch_workaround_1;
> > > + processor.switch_mm = cpu_v7_smc_switch_mm;
> > > + spectre_v2_method = "firmware PSCI";
> > 
> > My previous remark still stands: this is not really PSCI.
> 
> Sorry, no.  Your comment was for the HVC call, not the SMC.  You said
> nothing about this one.

My bad then. For all intents and purposes, they are the same thing,
just serviced by a different exception level.

M.

-- 
Jazz is not dead, it just smell funny.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm