Re: [PATCH v6 25/26] arm64: KVM: Allow mapping of vectors outside of the RAM region

[Andrew Jones <drjones@redhat.com>]

On Thu, Mar 15, 2018 at 04:17:53PM +0000, Marc Zyngier wrote:
> On 15/03/18 15:54, Andrew Jones wrote:
> > On Wed, Mar 14, 2018 at 04:50:48PM +0000, Marc Zyngier wrote:
> >>  static inline void *kvm_get_hyp_vector(void)
> >>  {
> >>  	struct bp_hardening_data *data = arm64_get_bp_hardening_data();
> >> -	void *vect = kvm_ksym_ref(__kvm_hyp_vector);
> >> +	void *vect = kern_hyp_va(kvm_ksym_ref(__kvm_hyp_vector));
> >> +	int slot = -1;
> >>  
> >> -	if (data->fn) {
> >> -		vect = __bp_harden_hyp_vecs_start +
> >> -		       data->hyp_vectors_slot * SZ_2K;
> >> +	if (cpus_have_const_cap(ARM64_HARDEN_BRANCH_PREDICTOR) && data->fn) {
> > 
> > Now that I'm trying to consider heterogeneous systems, I'm wondering why
> > harden BP isn't checked with this_cpu_has_cap() like harden EL2 vectors.
> > I'm also wondering if it's even possible for CPUs to come up without all
> > of them having harden EL2 vectors if the boot CPU has it. Won't
> > verify_local_cpu_errata_workarounds() require it? I'm probably just
> > getting lost in all the capability stuff...
> 
> Checking harden BP on this particular CPU will only tell you if this CPU
> is affected, but won't give you any additional information (i.e. you'd
> still need to check obtain the stuff pointed to by data).
> 
> Checking for *all* CPUs in one go has some advantages: it is a static
> key, which means that unaffected platforms will fly (this is a hot path
> on VHE), and you can check data if you're affected.

Is there a problem with using the static key to check
ARM64_HARDEN_EL2_VECTORS below? (Sorry, I'm still confused why we're not
using the same functions for both.) If we can't use the static key
below, then should we swap the check with !has_vhe() to ensure the
compiler will avoid emitting the this_cpu_has_cap() call on VHE's fast
path?

I also noticed that calling this_cpu_has_cap() puts a constraint on
kvm_get_hyp_vector() that it must never be called without preemption
disabled. It's not, but it's one more thing to think about. If there's
no reason not to use the static key with cpus_have_const_cap() then
maybe we should?

> 
> > 
> >> +		vect = __bp_harden_hyp_vecs_start;
> >> +		slot = data->hyp_vectors_slot;
> >> +	}
> >>  
> >> -		if (!has_vhe())
> >> -			vect = lm_alias(vect);
> >> +	if (this_cpu_has_cap(ARM64_HARDEN_EL2_VECTORS) && !has_vhe()) {
> >> +		vect = __kvm_bp_vect_base;
> >> +		if (slot == -1)
> >> +			slot = __kvm_harden_el2_vector_slot;
> >>  	}

Thanks,
drew