Re: [RFC PATCH v3 02/29] KVM: arm64: Save ID registers' sanitized value per vCPU

[Reiji Watanabe <reijiw@google.com>]

On Sun, Nov 21, 2021 at 4:37 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Wed, 17 Nov 2021 06:43:32 +0000,
> Reiji Watanabe <reijiw@google.com> wrote:
> >
> > Extend sys_regs[] of kvm_cpu_context for ID registers and save ID
> > registers' sanitized value in the array for the vCPU at the first
> > vCPU reset. Use the saved ones when ID registers are read by
> > userspace (via KVM_GET_ONE_REG) or the guest.
> >
> > Signed-off-by: Reiji Watanabe <reijiw@google.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h | 10 +++++++
> >  arch/arm64/kvm/sys_regs.c         | 43 +++++++++++++++++++------------
> >  2 files changed, 37 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index edbe2cb21947..72db73c79403 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -146,6 +146,14 @@ struct kvm_vcpu_fault_info {
> >       u64 disr_el1;           /* Deferred [SError] Status Register */
> >  };
> >
> > +/*
> > + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2),
> > + * where 0<=crm<8, 0<=op2<8.
> > + */
> > +#define KVM_ARM_ID_REG_MAX_NUM 64
> > +#define IDREG_IDX(id)                ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id))
> > +#define IDREG_SYS_IDX(id)    (ID_REG_BASE + IDREG_IDX(id))
> > +
> >  enum vcpu_sysreg {
> >       __INVALID_SYSREG__,   /* 0 is reserved as an invalid value */
> >       MPIDR_EL1,      /* MultiProcessor Affinity Register */
> > @@ -210,6 +218,8 @@ enum vcpu_sysreg {
> >       CNTP_CVAL_EL0,
> >       CNTP_CTL_EL0,
> >
> > +     ID_REG_BASE,
> > +     ID_REG_END = ID_REG_BASE + KVM_ARM_ID_REG_MAX_NUM - 1,
>
> It is rather unclear to me why we want these registers to be
> replicated on a per-CPU basis. Yes, this fits the architecture, but
> that's also a total waste of memory if you have more than a single
> CPU, because we make a point in only exposing homogeneous properties
> to the VM (I don't think anyone intends to support vcpu asymmetry in a
> VM, and 64 registers per vcpu is not an insignificant memory usage).
>
> If there are no reasons for this to be per-CPU, please move it to be
> global to the VM. This also mean that once a vcpu has reset, it
> shouldn't be possible to affect the registers. This shouldn't affect
> the userspace API though.


Currently, userspace can configure different CPU features for each vCPU
with KVM_ARM_VCPU_INIT, which indirectly affect ID registers.
I'm not sure if anyone actually does that though.

Since I personally thought having ID registers per vCPU more naturally
fits the behavior of KVM_ARM_VCPU_INIT and makes more straightforward
behavior of KVM_SET_ONE_REG, I chose that.
(That would be also better in terms of vCPUs scalability for live migration
 considering a case where userspace tries to restore ID registers for
 many vCPUs in parallel during live migration.  Userspace could avoid that,
 and there are ways for KVM to mitigate that though.)

Having ID registers per-VM is of course feasible even while maintaining
the current behavior of KVM_ARM_VCPU_INIT though.

Thanks,
Reiji