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

[Fuad Tabba]

On Mon, 2 Dec 2024 at 14:29, Marc Zyngier <maz at kernel.org> wrote:
>
> On Mon, 02 Dec 2024 12:57:44 +0000,
> Fuad Tabba <tabba at google.com> wrote:
> >
> > Hi Marc,
> >
> > On Sun, 1 Dec 2024 at 16:01, Fuad Tabba <tabba at google.com> wrote:
> > >
> > > Hi Marc,
> > >
> > >
> > > On Sun, 1 Dec 2024 at 12:58, Marc Zyngier <maz at kernel.org> wrote:
> > > >
> > > > On Thu, 28 Nov 2024 12:35:14 +0000,
> > > > Fuad Tabba <tabba at 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 at 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.

Will do, thanks again!
/fuad

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