Re: [PATCH] arm64: KVM: export current vcpu->pause state via pseudo regs

[Christoffer Dall <christoffer.dall@linaro.org>]

On Fri, Aug 01, 2014 at 10:11:52AM +0100, Alex Bennée wrote:
> 
> Christoffer Dall writes:
> 
> > On Thu, Jul 31, 2014 at 04:14:51PM +0100, Alex Bennée wrote:
> >> 
> >> Christoffer Dall writes:
> >> 
> >> > On Wed, Jul 09, 2014 at 02:55:12PM +0100, Alex Bennée wrote:
> >> >> To cleanly restore an SMP VM we need to ensure that the current pause
> >> >> state of each vcpu is correctly recorded. Things could get confused if
> >> >> the CPU starts running after migration restore completes when it was
> >> >> paused before it state was captured.
> >> >> 
> >> <snip>
> >> >> +/* 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))
> >> 
> >> I was trying to use the existing masks, but of course if anyone changes
> >> that it would be an ABI change so probably not worth it.
> >> 
> >
> > 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 weird
> > to use that to me.
> 
> Yeah I missed that. I could do a:
> 
> #define KVM_REG_ARM_COPROC_INDEX_MASK   ((1<<KVM_REG_ARM_COPROC_SHIFT)-1)
> 
> and use that. I'm generally try to avoid hardcoded numbers but I could
> be being a little OCD here ;-)
> 
> >> > Can you add the 32-bit counterpart as part of this patch?
> >> 
> >> 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.
> 
> Given the similarity of this code between arm and arm64 I'm wondering if
> it's worth doing a arch/arm/kvm/guest_common.c or something to reduce
> the amount of copy paste stuff?
> 
We've gotten by without it so far.  I fear we end up with a bunch of
complications due to differences in sizeof(unsigned long) etc., but I
may be wrong.

The amount of code that is copied should be trivial boilerplate stuff,
but if you think it's worth unifying, then I'd be happy to review the
patch.

Thanks,
-Christoffer