[PATCH v2 07/12] KVM: arm64: Rework specifying restricted features for protected VMs

[Fuad Tabba]

Hi Marc,

On Sun, 24 Nov 2024 at 12:38, Marc Zyngier <maz at kernel.org> wrote:
>
> On Fri, 22 Nov 2024 11:06:17 +0000,
> Fuad Tabba <tabba at google.com> wrote:
> >
> > The existing code didn't properly distinguish between signed and
> > unsigned features, and was difficult to read and to maintain.
> > Rework it using the same method used in other parts of KVM when
> > handling vcpu features.
> >
> > Signed-off-by: Fuad Tabba <tabba at google.com>
> > ---
> >  .../arm64/kvm/hyp/include/nvhe/fixed_config.h |   1 -
> >  arch/arm64/kvm/hyp/nvhe/sys_regs.c            | 356 +++++++++---------
> >  2 files changed, 186 insertions(+), 171 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/hyp/include/nvhe/fixed_config.h b/arch/arm64/kvm/hyp/include/nvhe/fixed_config.h
> > index 69e26d1a0ebe..37a6d2434e47 100644
> > --- a/arch/arm64/kvm/hyp/include/nvhe/fixed_config.h
> > +++ b/arch/arm64/kvm/hyp/include/nvhe/fixed_config.h
> > @@ -198,7 +198,6 @@
> >       FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64ISAR2_EL1_APA3), ID_AA64ISAR2_EL1_APA3_PAuth) \
> >       )
> >
> > -u64 pvm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id);
> >  bool kvm_handle_pvm_sysreg(struct kvm_vcpu *vcpu, u64 *exit_code);
> >  bool kvm_handle_pvm_restricted(struct kvm_vcpu *vcpu, u64 *exit_code);
> >  void kvm_init_pvm_id_regs(struct kvm_vcpu *vcpu);
> > diff --git a/arch/arm64/kvm/hyp/nvhe/sys_regs.c b/arch/arm64/kvm/hyp/nvhe/sys_regs.c
> > index 3aa76b018f70..cea3b099dc56 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/sys_regs.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/sys_regs.c
> > @@ -28,221 +28,237 @@ u64 id_aa64mmfr1_el1_sys_val;
> >  u64 id_aa64mmfr2_el1_sys_val;
> >  u64 id_aa64smfr0_el1_sys_val;
> >
> > -/*
> > - * Inject an unknown/undefined exception to an AArch64 guest while most of its
> > - * sysregs are live.
> > - */
> > -static void inject_undef64(struct kvm_vcpu *vcpu)
> > -{
> > -     u64 esr = (ESR_ELx_EC_UNKNOWN << ESR_ELx_EC_SHIFT);
> > -
> > -     *vcpu_pc(vcpu) = read_sysreg_el2(SYS_ELR);
> > -     *vcpu_cpsr(vcpu) = read_sysreg_el2(SYS_SPSR);
> > -
> > -     kvm_pend_exception(vcpu, EXCEPT_AA64_EL1_SYNC);
> > -
> > -     __kvm_adjust_pc(vcpu);
> > -
> > -     write_sysreg_el1(esr, SYS_ESR);
> > -     write_sysreg_el1(read_sysreg_el2(SYS_ELR), SYS_ELR);
> > -     write_sysreg_el2(*vcpu_pc(vcpu), SYS_ELR);
> > -     write_sysreg_el2(*vcpu_cpsr(vcpu), SYS_SPSR);
> > -}
> > +struct pvm_feature {
> > +     int shift;
> > +     int width;
> > +     u64 mask;
> > +     u64 max_supported;
> > +     bool is_signed;
>
> Most of this is very similar to struct arm64_ftr_bits. Can you align
> the types and names with it? And do you really need 'mask', which can
> be derived from shift and width?

Ack. I stored mask since it's already there in sysreg-defs.h, but I'll
recalculate it.

> max_supported as an unsigned value is also potentially troublesome for
> signed features. And does it need to be 64bit wide?

It doesn't need to be 64 bits. I will make it a u8. As for the sign,
the function that handles it, get_restricted_features(), uses bit
manipulation to handle it, since the sign bit is bit 3 for most (or
all) id regs, rather than rely on the sign, which is why I went with
unsigned.

> > +     bool (*vcpu_supported)(const struct kvm_vcpu *vcpu);
>
> We really need to find a way to kill this. The two features that stick
> out badly are SVE and PAuth, and I don't see why we can't eventually
> track them at the VM level instead (despite the userspace API).

It is actually being tracked per vm, and only calculated once per vm.
I went with the name vcpu_supported, since the functions (or macros)
it will point to are vcpu_has_ptrauth() and vcpu_has_sve(). I could at
least rename it here to vm_supported(), in anticipation of a wider
change.

That said, this should be fixed for all of kvm/arm64. I'll see if it's
a simple patch to add to this series as a respin, or something for
later.

Thanks again!

/fuad

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