Re: [PATCH v2 1/5] arm: KVM: export vcpi->pause state via MP_STATE ioctls

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

On Mon, Mar 09, 2015 at 04:34:21PM +0000, Alex Bennée wrote:
> 
> Christoffer Dall <christoffer.dall@linaro.org> writes:
> 
> > Hi Alex,
> >
> > The subject of this change has a typo, and I also think it's not about
> > exposing the pause state (that's just an internal name/concept), but
> > about exposing the PSCI state, or simply the VCPU power state.
> 
> arm: KVM: export VCPU power state via MP_STATE ioctl?
> 

arm/arm64: KVM: 

otherwise looks good to me ;)


> >
> > On Mon, Mar 02, 2015 at 01:29:00PM +0000, 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.
> >> 
> >> We use the existing KVM_GET/SET_MP_STATE ioctl to do this. The arm/arm64
> >> interface is a lot simpler as the only valid states are
> >> KVM_MP_STATE_RUNNABLE and KVM_MP_STATE_HALTED.
> >> 
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> 
> >> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> >> index b112efc..602156f 100644
> >> --- a/Documentation/virtual/kvm/api.txt
> >> +++ b/Documentation/virtual/kvm/api.txt
> >> @@ -997,7 +997,7 @@ for vm-wide capabilities.
> >>  4.38 KVM_GET_MP_STATE
> >>  
> >>  Capability: KVM_CAP_MP_STATE
> >> -Architectures: x86, s390
> >> +Architectures: x86, s390, arm, arm64
> >>  Type: vcpu ioctl
> >>  Parameters: struct kvm_mp_state (out)
> >>  Returns: 0 on success; -1 on error
> >> @@ -1027,15 +1027,21 @@ Possible values are:
> >>   - KVM_MP_STATE_LOAD:            the vcpu is in a special load/startup state
> >>                                   [s390]
> >>  
> >> -On x86, this ioctl is only useful after KVM_CREATE_IRQCHIP. Without an
> >> -in-kernel irqchip, the multiprocessing state must be maintained by userspace on
> >> +For x86:
> >> +
> >> +This ioctl is only useful after KVM_CREATE_IRQCHIP.  Without an in-kernel
> >> +irqchip, the multiprocessing state must be maintained by userspace on
> >
> > Nit: I would not taint the git log with this change but instead just
> > introduce a paragraph below starting with "On arm/arm64, " and you would
> > get the same effect.
> >
> >>  these architectures.
> >>  
> >> +For arm/arm64:
> >> +
> >> +The only states that are valid are KVM_MP_STATE_HALTED and
> >> +KVM_MP_STATE_RUNNABLE which reflect if the vcpu is paused or not.
> >
> > As suggested on the QEMU series, HALTED is probably not the right thing
> > to use.
> 
> KVM_MP_STATE_STOPPED is currently only used for s390 but seems to fit.
> I'm wary of adding yet another define.
> 

sounds fine, as long as it doesn't have some inherently different
meaning with s390.

> >
> >>  
> >>  4.39 KVM_SET_MP_STATE
> >>  
> >>  Capability: KVM_CAP_MP_STATE
> >> -Architectures: x86, s390
> >> +Architectures: x86, s390, arm, arm64
> >>  Type: vcpu ioctl
> >>  Parameters: struct kvm_mp_state (in)
> >>  Returns: 0 on success; -1 on error
> >> @@ -1043,10 +1049,16 @@ Returns: 0 on success; -1 on error
> >>  Sets the vcpu's current "multiprocessing state"; see KVM_GET_MP_STATE for
> >>  arguments.
> >>  
> >> -On x86, this ioctl is only useful after KVM_CREATE_IRQCHIP. Without an
> >> -in-kernel irqchip, the multiprocessing state must be maintained by userspace on
> >> +For x86:
> >> +
> >> +This ioctl is only useful after KVM_CREATE_IRQCHIP.  Without an in-kernel
> >> +irqchip, the multiprocessing state must be maintained by userspace on
> >>  these architectures.
> >>  
> >> +For arm/arm64:
> >> +
> >> +The only states that are valid are KVM_MP_STATE_HALTED and
> >> +KVM_MP_STATE_RUNNABLE which reflect if the vcpu should be paused or not.
> >
> > same as above
> >
> >>  
> >>  4.40 KVM_SET_IDENTITY_MAP_ADDR
> >>  
> >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> >> index 5560f74..8531536 100644
> >> --- a/arch/arm/kvm/arm.c
> >> +++ b/arch/arm/kvm/arm.c
> >> @@ -183,6 +183,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> >>  	case KVM_CAP_ARM_PSCI:
> >>  	case KVM_CAP_ARM_PSCI_0_2:
> >>  	case KVM_CAP_READONLY_MEM:
> >> +	case KVM_CAP_MP_STATE:
> >>  		r = 1;
> >>  		break;
> >>  	case KVM_CAP_COALESCED_MMIO:
> >> @@ -313,13 +314,29 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> >>  int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
> >>  				    struct kvm_mp_state *mp_state)
> >>  {
> >> -	return -EINVAL;
> >> +	if (vcpu->arch.pause)
> >> +		mp_state->mp_state = KVM_MP_STATE_HALTED;
> >> +	else
> >> +		mp_state->mp_state = KVM_MP_STATE_RUNNABLE;
> >> +
> >> +	return 0;
> >>  }
> >>  
> >>  int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
> >>  				    struct kvm_mp_state *mp_state)
> >>  {
> >> -	return -EINVAL;
> >> +	switch (mp_state->mp_state) {
> >> +	case KVM_MP_STATE_RUNNABLE:
> >> +		vcpu->arch.pause = false;
> >> +		break;
> >> +	case KVM_MP_STATE_HALTED:
> >> +		vcpu->arch.pause = true;
> >> +		break;
> >> +	default:
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	return 0;
> >>  }
> >>  
> >>  /**
> >
> > Are we capturing the vcpu features in some way or do we expect userspace
> > to migrate these on its own?  The reason I'm asking, is if you create
> > multiple VCPUs, where all the non-primary VCPUs are started in power off
> > mode, and then you boot your guest which powers on all VCPUs, and then
> > you restart your guest (with PSCI RESET), the system will not power on
> > all the non-primary VCPUs but hold them in power-off.
> >
> > We need to make sure this behavior is preserved for a reboot across a
> > migration.  Is it?
> 
> Isn't that behaviour orthogonal to the migration case?
> 
> - Boot
> - Power on secondary CPUs
> - Power off one secondary CPU
> - Migrate to file (cpu_powered reflects state of each CPU)
> 
> - Start fresh QEMU
> - Restore from file (cpu_powered -> vcpu->paused via ioctl)
> - Run (we continue with the same power state pre-migrate)
> 
> - PSCI RESET
> - Does what it does, power all secondaries down?
> - Kernel boots, turns them on?
> 
Hmmm. As long as QEMU always inits all VCPUs in the exact same way
(including the KVM_ARM_VCPU_POWER_OFF feature bit) I guess it works and
that's probably a reasonable requirement for migration.

-Christoffer