From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoffer Dall Subject: Re: [PATCH] arm64: KVM: export current vcpu->pause state via pseudo regs Date: Thu, 31 Jul 2014 18:38:05 +0200 Message-ID: <20140731163805.GK11610@cbox> References: <1404914112-7298-1-git-send-email-alex.bennee@linaro.org> <20140731143538.GI11610@cbox> <87mwbpimgz.fsf@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, Marc Zyngier , Catalin Marinas , Will Deacon , Gleb Natapov , Paolo Bonzini , open list To: Alex =?iso-8859-1?Q?Benn=E9e?= Return-path: Content-Disposition: inline In-Reply-To: <87mwbpimgz.fsf@linaro.org> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On Thu, Jul 31, 2014 at 04:14:51PM +0100, Alex Benn=E9e wrote: >=20 > Christoffer Dall writes: >=20 > > On Wed, Jul 09, 2014 at 02:55:12PM +0100, Alex Benn=E9e wrote: > >> To cleanly restore an SMP VM we need to ensure that the current pa= use > >> state of each vcpu is correctly recorded. Things could get confuse= d if > >> the CPU starts running after migration restore completes when it w= as > >> paused before it state was captured. > >>=20 > > >> +/* Power state (PSCI), not real registers */ > >> +#define KVM_REG_ARM_PSCI (0x0014 << KVM_REG_ARM_COPROC_SHIFT) > >> +#define KVM_REG_ARM_PSCI_REG(n) \ > >> + (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | KVM_REG_ARM_PSCI | \ > >> + (n & ~KVM_REG_ARM_COPROC_MASK)) > > > > I don't understand this mask, why isn't this > > (n & 0xffff)) >=20 > I was trying to use the existing masks, but of course if anyone chang= es > that it would be an ABI change so probably not worth it. >=20 the KVM_REG_ARM_COPROC_MASK is part of the uapi IIRC, so that's not the issue, but that mask doesn't cover all the upper bits, so it feels weir= d to use that to me. > > > >> +#define KVM_REG_ARM_PSCI_STATE KVM_REG_ARM_PSCI_REG(0) > >> +#define NUM_KVM_PSCI_REGS 1 > >> + > > > > you're missing updates to Documentation/virtual/kvm/api.txt here. >=20 > Will add. >=20 > >> /* Device Control API: ARM VGIC */ > >> #define KVM_DEV_ARM_VGIC_GRP_ADDR 0 > >> #define KVM_DEV_ARM_VGIC_GRP_DIST_REGS 1 > >> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > >> index 205f0d8..31d6439 100644 > >> --- a/arch/arm64/kvm/guest.c > >> +++ b/arch/arm64/kvm/guest.c > >> @@ -189,6 +189,54 @@ static int get_timer_reg(struct kvm_vcpu *vcp= u, const struct kvm_one_reg *reg) > >> } > >> =20 > >> /** > >> + * PSCI State > >> + * > >> + * These are not real registers as they do not actually exist in = the > >> + * hardware but represent the current power state of the vCPU > > > > full stop > > > >> + */ > >> + > >> +static bool is_psci_reg(u64 index) > >> +{ > >> + switch (index) { > >> + case KVM_REG_ARM_PSCI_STATE: > >> + return true; > >> + } > >> + return false; > >> +} > >> + > >> +static int copy_psci_indices(struct kvm_vcpu *vcpu, u64 __user *u= indices) > >> +{ > >> + if (put_user(KVM_REG_ARM_PSCI_STATE, uindices)) > >> + return -EFAULT; > >> + return 0; > >> +} > >> + > >> +static int set_psci_reg(struct kvm_vcpu *vcpu, const struct kvm_o= ne_reg *reg) > >> +{ > >> + void __user *uaddr =3D (void __user *)(long)reg->addr; > >> + u64 val; > >> + int ret; > >> + > >> + ret =3D copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id)); > >> + if (ret !=3D 0) > >> + return ret; > >> + > >> + vcpu->arch.pause =3D (val & 0x1) ? false : true; > > > > tabs >=20 > Hmmm, apparently the GNU Emacs "linux" style doesn't actually enforce > that. Who knew? I'll beat the editor into submission. >=20 > > I really need the documentation of the ABI, why is bit[0] =3D=3D 1 = not > > paused? >=20 > I figured 1 =3D=3D running, but I can switch it round if you want to = to map > directly to the .pause flag. >=20 don't care very deeply, just want something to describe it clearly. > > If we are not complaining when setting the pause value to false if = it > > was true before, then we probably also need to wake up the thread i= n > > case this is called from another thread, right? > > > > or perhaps we should just return an error if you're trying to un-pa= use a > > CPU through this interface, hmmmm. >=20 > Wouldn't it be an error to mess with any register when the system is = not > in a quiescent state? I was assuming that the wake state is dealt wit= h > when the run loop finally restarts.=20 >=20 The ABI doesn't really define it as an error (the ABI doesn't enforce anything right now) so the question is, does it ever make sense to clea= r the pause flag through this ioctl? If not, I think we should just err on the side of caution and specify in the docs that this is not supported and return an error. > > > > > please check for use of tabs versus spaces, checkpatch.pl should > > complain. > > > > Can you add the 32-bit counterpart as part of this patch? >=20 > Same patch? Sure. really up to you if you want to split it up into two patches, but I think it's small enough that you can just create one patch. Thanks, -Christoffer