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

[Marc Zyngier <maz@kernel.org>]

On Tue, 23 Nov 2021 04:39:27 +0000,
Reiji Watanabe <reijiw@google.com> wrote:
> 
> 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.

But the way the ID regs are affected is always global AFAICT. For
example, if you instantiate a PMU, you do so on all vcpus, even of the
architecture allows you to build something completely asymmetric.

> 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.

I agree that this is the logical approach from an architectural PoV.

> (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.)

I think these are orthogonal things. We can expose a per-vcpu view,
but there is no need to have per-vcpu storage and to allow asymmetric
VMs. If I have anything to say about the future of KVM/arm64, it will
be that I don't want to support this at all.

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

Exactly. per-VM storage, and per-vcpu visibility. It will prevent all
sort of odd behaviours.

	M.

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