Re: [PATCH v2 5/7] KVM: arm64: Allocate memory at hyp for host sve state in pKVM

[Marc Zyngier <maz@kernel.org>]

On Wed, 22 May 2024 15:37:53 +0100,
Fuad Tabba <tabba@google.com> wrote:
> 
> Hi Marc,
> 
> On Tue, May 21, 2024 at 10:44 PM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Tue, 21 May 2024 17:37:18 +0100,
> > Fuad Tabba <tabba@google.com> wrote:
> > >
> > > Protected mode needs to maintain (save/restore) the host's sve
> > > state, rather than relying on the host kernel to do that. This is
> > > to avoid leaking information to the host about guests and the
> > > type of operations they are performing.
> > >
> > > As a first step towards that, allocate memory at hyp, per cpu, to
> > > hold the host sve data. The following patch will use this memory
> > > to save/restore the host state.
> >
> > What I read in the code contradicts this statement. The memory is
> > definitely allocated on the *host*.
> 
> You're right. I should say memory mapped at hyp.
> 
> > >
> > > Signed-off-by: Fuad Tabba <tabba@google.com>
> > > ---
> > > Note that the last patch in this series will consolidate the
> > > setup of the host's fpsimd and sve states, which currently take
> > > place in two different locations. Moreover, that last patch will
> > > also place the host fpsimd and sve_state pointers in a union.
> > > ---
> > >  arch/arm64/include/asm/kvm_host.h    |  2 +
> > >  arch/arm64/include/asm/kvm_pkvm.h    |  9 ++++
> > >  arch/arm64/include/uapi/asm/ptrace.h | 14 ++++++
> > >  arch/arm64/kvm/arm.c                 | 68 ++++++++++++++++++++++++++++
> > >  arch/arm64/kvm/hyp/nvhe/setup.c      | 24 ++++++++++
> > >  5 files changed, 117 insertions(+)
> > >
> > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > index 0a5fceb20f3a..7b3745ef1d73 100644
> > > --- a/arch/arm64/include/asm/kvm_host.h
> > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > @@ -535,7 +535,9 @@ struct kvm_cpu_context {
> > >   */
> > >  struct kvm_host_data {
> > >       struct kvm_cpu_context host_ctxt;
> > > +
> > >       struct user_fpsimd_state *fpsimd_state; /* hyp VA */
> > > +     struct user_sve_state *sve_state;       /* hyp VA */
> > >
> > >       /* Ownership of the FP regs */
> > >       enum {
> > > diff --git a/arch/arm64/include/asm/kvm_pkvm.h b/arch/arm64/include/asm/kvm_pkvm.h
> > > index ad9cfb5c1ff4..b9d12e123efb 100644
> > > --- a/arch/arm64/include/asm/kvm_pkvm.h
> > > +++ b/arch/arm64/include/asm/kvm_pkvm.h
> > > @@ -128,4 +128,13 @@ static inline unsigned long hyp_ffa_proxy_pages(void)
> > >       return (2 * KVM_FFA_MBOX_NR_PAGES) + DIV_ROUND_UP(desc_max, PAGE_SIZE);
> > >  }
> > >
> > > +static inline size_t pkvm_host_sve_state_size(void)
> > > +{
> > > +     if (!system_supports_sve())
> > > +             return 0;
> > > +
> > > +     return size_add(sizeof(struct user_sve_state),
> > > +                     SVE_SIG_REGS_SIZE(sve_vq_from_vl(kvm_host_sve_max_vl)));
> > > +}
> > > +
> > >  #endif       /* __ARM64_KVM_PKVM_H__ */
> > > diff --git a/arch/arm64/include/uapi/asm/ptrace.h b/arch/arm64/include/uapi/asm/ptrace.h
> > > index 7fa2f7036aa7..77aabf964071 100644
> > > --- a/arch/arm64/include/uapi/asm/ptrace.h
> > > +++ b/arch/arm64/include/uapi/asm/ptrace.h
> > > @@ -120,6 +120,20 @@ struct user_sve_header {
> > >       __u16 __reserved;
> > >  };
> > >
> > > +struct user_sve_state {
> > > +     __u64 zcr_el1;
> > > +
> > > +     /*
> > > +      * Ordering is important since __sve_save_state/__sve_restore_state
> > > +      * relies on it.
> > > +      */
> > > +     __u32 fpsr;
> > > +     __u32 fpcr;
> > > +
> > > +     /* Must be SVE_VQ_BYTES (128 bit) aligned. */
> > > +     __u8 sve_regs[];
> > > +};
> > > +
> >
> > Huh, why is this in uapi? Why should userspace even care about this at
> > all? From what I can tell, this is purely internal to the kernel, and
> > in any case, KVM isn't tied to that format if it doesn't dump stuff in
> > the userspace thread context. Given the number of bugs this format has
> > generated, I really wouldn't mind moving away from it.
> 
> I put it here since the sve_header is here, as well as other similar structures.
> 
> Should I move it to asm/kvm_pkvm.h?

kvm_host.h or kvm_types.h seem like the best option (replacing the
"user" prefix with something else), as this is directly consumed by
kvm_host.h.

Thanks,

	M.

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