From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933479AbdEKRyi (ORCPT ); Thu, 11 May 2017 13:54:38 -0400 Received: from foss.arm.com ([217.140.101.70]:51908 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933460AbdEKRyh (ORCPT ); Thu, 11 May 2017 13:54:37 -0400 Date: Thu, 11 May 2017 18:53:58 +0100 From: Mark Rutland To: Marc Zyngier Cc: Suzuki K Poulose , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, bigeasy@linutronix.de, catalin.marinas@arm.com, peterz@infradead.org, tglx@linutronix.de, will.deacon@arm.com Subject: Re: [PATCHv2] arm64/cpufeature: don't use mutex in bringup path Message-ID: <20170511175357.GA29929@leverpostej> References: <1494514878-26878-1-git-send-email-mark.rutland@arm.com> <498b2e16-538a-d5ea-7843-2ebbff2007df@arm.com> <20170511153719.GB19626@leverpostej> <232510f0-115e-b2c6-022f-03efd2606d56@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 11, 2017 at 05:08:19PM +0100, Marc Zyngier wrote: > On 11/05/17 16:54, Suzuki K Poulose wrote: > > On 11/05/17 16:37, Mark Rutland wrote: > >> On Thu, May 11, 2017 at 04:15:38PM +0100, Suzuki K Poulose wrote: > >>> On 11/05/17 16:01, Mark Rutland wrote: > >>>> +static inline bool cpus_have_const_cap(int num) > >>>> +{ > >>>> + if (static_branch_likely(&arm64_const_caps_ready)) > >>>> + return __cpus_have_const_cap(num); > >>>> + else > >>>> + return cpus_have_cap(num); > >>> > >>> We use cpus_have_const_cap() from hyp code, via has_vhe() and we could potentially > >>> try to access unmapped kernel data from hyp if we fallback to cpus_have_cap(). > >>> However, it looks like we have already set arm64_const_caps_ready, so should not > >>> hit it in practise. May be we could add a stricter version of the helper ? > >>> > >>> static inline cpus_have_const_cap_strict(int num) > >>> { > >>> BUG_ON(!static_branch_likely(&arm64_const_caps_ready); > >>> return __cpus_have_const_cap(num); > >>> } > >> > >> Just to check, is that the only user of cpus_have_const_cap() at hyp? > > > > Uh, no we have one more, via system_supports_fpsimd() in __actvate_traps. > > Indeed, and I'd definitely expect to see more of that trickling in (if > only to deal with errata). > > I'm OK with the BUG_ON version, TBH. It's not pretty, but it will be > perfectly visible if it fires. We can't make system_supports_fpsimd() BUG_ON(), because that will fire the first time the boot CPU tries to switch thread, due to fpsimd_thread_switch(). However, thinking about it, there's no risk that this code runs at hyp before we've intialised the caps. We initialise hyp from kvm_arch_init(), which is a module initcall. As it's built-in, that's actually a device initcall, which happens long after we've finalised the cpucaps. So the v2 patch should be safe, though we can make that a little clearer with the below, which I'll fold into v3. Thanks, Mark. ---->8---- diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 5e19165..28bf4ea 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -24,6 +24,7 @@ #include #include +#include #include #include #include @@ -356,8 +357,10 @@ static inline void __cpu_init_hyp_mode(phys_addr_t pgd_ptr, { /* * Call initialization code, and switch to the full blown - * HYP code. + * HYP code. If the cpucaps haven't been finialized yet, + * something has gone very wrong, and hyp will crash and burn. */ + BUG_ON(!static_branch_likely(&arm64_const_caps_ready)); __kvm_call_hyp((void *)pgd_ptr, hyp_stack_ptr, vector_ptr); }