Re: [PATCH v3 14/15] KVM: arm64: Convert the SVE guest vcpu flag to a vm flag

[Marc Zyngier <maz@kernel.org>]

On Mon, 02 Dec 2024 12:57:44 +0000,
Fuad Tabba <tabba@google.com> wrote:
> 
> Hi Marc,
> 
> On Sun, 1 Dec 2024 at 16:01, Fuad Tabba <tabba@google.com> wrote:
> >
> > Hi Marc,
> >
> >
> > On Sun, 1 Dec 2024 at 12:58, Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Thu, 28 Nov 2024 12:35:14 +0000,
> > > Fuad Tabba <tabba@google.com> wrote:
> > > >
> > > > The vcpu flag GUEST_HAS_SVE is per-vcpu, but it is based on what
> > > > is now a per-vm feature. Make the flag per-vm.
> > > >
> > > > Signed-off-by: Fuad Tabba <tabba@google.com>
> > > > ---
> > > >  arch/arm64/include/asm/kvm_emulate.h    |  6 +++---
> > > >  arch/arm64/include/asm/kvm_host.h       | 10 ++++++----
> > > >  arch/arm64/kvm/hyp/include/hyp/switch.h |  2 +-
> > > >  arch/arm64/kvm/hyp/nvhe/hyp-main.c      |  2 +-
> > > >  arch/arm64/kvm/hyp/nvhe/pkvm.c          | 11 +++++++----
> > > >  arch/arm64/kvm/hyp/nvhe/switch.c        |  8 ++++----
> > > >  arch/arm64/kvm/hyp/vhe/switch.c         |  2 +-
> > > >  arch/arm64/kvm/reset.c                  |  2 +-
> > > >  8 files changed, 24 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > > > index 406e99a452bf..ae6d0dc0e4ff 100644
> > > > --- a/arch/arm64/include/asm/kvm_emulate.h
> > > > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > > > @@ -620,7 +620,7 @@ static __always_inline void kvm_write_cptr_el2(u64 val)
> > > >  }
> > > >
> > > >  /* Resets the value of cptr_el2 when returning to the host. */
> > > > -static __always_inline void kvm_reset_cptr_el2(struct kvm_vcpu *vcpu)
> > > > +static __always_inline void kvm_reset_cptr_el2(struct kvm *kvm)
> > >
> > > I'd rather you avoid the resulting churn:
> > >
> > > static __always_inline void kvm_reset_cptr_el2(struct kvm_vcpu *vcpu)
> > > {
> > >         struct kvm *kvm = kern_hyp_va(vcpu->kvm);
> > >         [...]
> > >
> > > which does the trick at zero cost. It is also more logical to keep the
> > > vcpu as a parameter, as the per-VM-ness is only an implementation
> > > detail.
> >
> > The reason for the churn (here and below for vcpu_has_sve()), which in
> > hindsight I should have mentioned in the commit message or as a note,
> > is that this code is called by the host kernel (e.g., in reset.c), and
> > by the hypervisor). For nvhe, we need kern_hyp_va(), but not for the
> > host. Should I gate the va conversion with is_hyp_nvhe()?
> 
> I spoke too soon. Your suggestion results in less churn, and of
> course, solves the issue I had with kern_hyp_va.
> 
> I did have to convert kvm_reset_cptr_el2() into a macro, because it
> now needs kern_hyp_va(), defined in kvm_mmu.h, which in turn includes
> kvm_emulate.h. But since it's __always_inline, it shouldn't make any
> difference in practice.
> 
> If this conversion to a macro is alright by you, I'll respin this.

If you can convert it to something like:

static __always_inline void __kvm_reset_cptr_el2(struct kvm *kvm)
{
	[...]
}

#define kvm_reset_cptr_el2(v)	__kvm_reset_cptr_el2(kern_hyp_va((v)->kvm))

that'd be great, as it keeps the body of the function as C code, which
helps with debugging.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.