On Thu, Feb 04, 2021 at 11:08:33AM +0000, Quentin Perret wrote:
> On Wednesday 03 Feb 2021 at 15:31:39 (+0000), Will Deacon wrote:
> > On Fri, Jan 08, 2021 at 12:15:15PM +0000, Quentin Perret wrote:
> > > @@ -1481,7 +1486,10 @@ static void cpu_set_hyp_vector(void)
> > > struct bp_hardening_data *data = this_cpu_ptr(&bp_hardening_data);
> > > void *vector = hyp_spectre_vector_selector[data->slot];
> > >
> > > - *this_cpu_ptr_hyp_sym(kvm_hyp_vector) = (unsigned long)vector;
> > > + if (!is_protected_kvm_enabled())
> > > + *this_cpu_ptr_hyp_sym(kvm_hyp_vector) = (unsigned long)vector;
> > > + else
> > > + kvm_call_hyp_nvhe(__pkvm_cpu_set_vector, data->slot);
> >
> > *Very* minor nit, but it might be cleaner to have static inline functions
> > with the same prototypes as the hypercalls, just to make the code even
> > easier to read. e.g
> >
> > if (!is_protected_kvm_enabled())
> > _cpu_set_vector(data->slot);
> > else
> > kvm_call_hyp_nvhe(__pkvm_cpu_set_vector, data->slot);
> >
> > you could then conceivably wrap that in a macro and avoid having the
> > "is_protected_kvm_enabled()" checks explicit every time.
>
> Happy to do this here, but are you suggesting to generalize this pattern
> to other places as well?
I think it's probably a good pattern to follow, but no need to generalise it
prematurely.
> > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > > index 3cf9397dabdb..9d4c9251208e 100644
> > > --- a/arch/arm64/kvm/mmu.c
> > > +++ b/arch/arm64/kvm/mmu.c
> > > @@ -225,15 +225,39 @@ void free_hyp_pgds(void)
> > > if (hyp_pgtable) {
> > > kvm_pgtable_hyp_destroy(hyp_pgtable);
> > > kfree(hyp_pgtable);
> > > + hyp_pgtable = NULL;
> > > }
> > > mutex_unlock(&kvm_hyp_pgd_mutex);
> > > }
> > >
> > > +static bool kvm_host_owns_hyp_mappings(void)
> > > +{
> > > + if (static_branch_likely(&kvm_protected_mode_initialized))
> > > + return false;
> > > +
> > > + /*
> > > + * This can happen at boot time when __create_hyp_mappings() is called
> > > + * after the hyp protection has been enabled, but the static key has
> > > + * not been flipped yet.
> > > + */
> > > + if (!hyp_pgtable && is_protected_kvm_enabled())
> > > + return false;
> > > +
> > > + BUG_ON(!hyp_pgtable);
> >
> > Can we fail more gracefully, e.g. by continuing without KVM?
>
> Got any suggestion as to how that can be done? We could also just remove
> that line -- that really should not happen.
Or downgrade to WARN_ON.
Will
