On Tue, Nov 23, 2021 at 2:03 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> 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.
Thank you so much for all your comments.
I will make it per VM storage.
Regards,
Reiji
