On Tue, 23 Nov 2021 04:39:27 +0000,
Reiji Watanabe <reijiw@xxxxxxxxxx> wrote:
>
> On Sun, Nov 21, 2021 at 4:37 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> >
> > On Wed, 17 Nov 2021 06:43:32 +0000,
> > Reiji Watanabe <reijiw@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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.
