Re: [PATCH v3 2/6] KVM: arm64: Save ID registers' sanitized value per guest

[Jing Zhang <jingzhangos@google.com>]

Hi Oliver,

On Tue, Mar 7, 2023 at 2:44 PM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> Hi Jing,
>
> On Tue, Feb 28, 2023 at 06:22:42AM +0000, Jing Zhang wrote:
> > From: Reiji Watanabe <reijiw@google.com>
> >
> > Introduce id_regs[] in kvm_arch as a storage of guest's ID registers,
> > and save ID registers' sanitized value in the array at KVM_CREATE_VM.
> > Use the saved ones when ID registers are read by the guest or
> > userspace (via KVM_GET_ONE_REG).
> >
> > No functional change intended.
> >
> > Signed-off-by: Reiji Watanabe <reijiw@google.com>
> > Co-developed-by: Jing Zhang <jingzhangos@google.com>
> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h | 12 +++++++++
> >  arch/arm64/kvm/arm.c              |  1 +
> >  arch/arm64/kvm/id_regs.c          | 44 ++++++++++++++++++++++++-------
> >  arch/arm64/kvm/sys_regs.c         |  2 +-
> >  arch/arm64/kvm/sys_regs.h         |  1 +
> >  5 files changed, 50 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index a1892a8f6032..5c1cec4efa37 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -245,6 +245,16 @@ struct kvm_arch {
> >        * the associated pKVM instance in the hypervisor.
> >        */
> >       struct kvm_protected_vm pkvm;
> > +
> > +     /*
> > +      * Save ID registers for the guest in id_regs[].
> > +      * (Op0, Op1, CRn, CRm, Op2) of the ID registers to be saved in it
> > +      * is (3, 0, 0, crm, op2), where 1<=crm<8, 0<=op2<8.
> > +      */
> > +#define KVM_ARM_ID_REG_NUM   56
> > +#define IDREG_IDX(id)                (((sys_reg_CRm(id) - 1) << 3) | sys_reg_Op2(id))
> > +#define IDREG(kvm, id)               kvm->arch.id_regs[IDREG_IDX(id)]
>
> I feel like the IDREG(...) macro just obfuscates what is otherwise a
> simple array access.
>
Sure, will use array access.
> > +static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r)
> > +{
> > +     if (sysreg_visible_as_raz(vcpu, r))
> > +             return 0;
> > +
> > +     return kvm_arm_read_id_reg_with_encoding(vcpu, reg_to_encoding(r));
>
> nit: you could probably drop the '_with_encoding' suffix, as I don't
> believe there are any other flavors of accessors.
>
Yes, will do.
> > +}
> > +
> >  /* cpufeature ID register access trap handlers */
> >
> >  static bool access_id_reg(struct kvm_vcpu *vcpu,
> > @@ -504,3 +505,28 @@ int kvm_arm_walk_id_regs(struct kvm_vcpu *vcpu, u64 __user *uind)
> >       }
> >       return total;
> >  }
> > +
> > +/*
> > + * Set the guest's ID registers that are defined in id_reg_descs[]
> > + * with ID_SANITISED() to the host's sanitized value.
> > + */
> > +void kvm_arm_set_default_id_regs(struct kvm *kvm)
> > +{
> > +     int i;
> > +     u32 id;
> > +     u64 val;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) {
> > +             id = reg_to_encoding(&id_reg_descs[i]);
> > +             if (WARN_ON_ONCE(!is_id_reg(id)))
> > +                     /* Shouldn't happen */
> > +                     continue;
>
> Could you instead wire in a check to kvm_sys_reg_table_init() or do
> something similar to it? Benefit of going that route is we outright
> refuse to run KVM with such an egregious bug.
>
The check is already added in the first commit, which is
kvm_arm_check_idreg_table() and improved progressively in later
commits.
> > +
> > +             if (id_reg_descs[i].visibility == raz_visibility)
> > +                     /* Hidden or reserved ID register */
> > +                     continue;
>
> This sort of check works only for registers known to be RAZ at compile
> time, but not for others that depend on runtime configuration. Is it
> possible to reset the ID register values on the first call to
> KVM_ARM_VCPU_INIT?
>
> The set of configured features is available at that point so you can
> actually handle things like SVE and 32-bit ID registers correctly.
>
This function actually will be replaced with another init function as
you suggested in a later commit (6/6), which would be called in
kvm_arch_init_vm.
> --
> Thanks,
> Oliver