On 25.02.20 09:29, Christian Borntraeger wrote:
>
>
> On 24.02.20 20:05, David Hildenbrand wrote:
>> On 24.02.20 12:40, Christian Borntraeger wrote:
>>> From: Janosch Frank <frankja@xxxxxxxxxxxxx>
>>>
>>> VCPU states have to be reported to the ultravisor for SIGP
>>> interpretation, kdump, kexec and reboot.
>>>
>>> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
>>> Reviewed-by: Thomas Huth <thuth@xxxxxxxxxx>
>>> Reviewed-by: Cornelia Huck <cohuck@xxxxxxxxxx>
>>> [borntraeger@xxxxxxxxxx: patch merging, splitting, fixing]
>>> Signed-off-by: Christian Borntraeger <borntraeger@xxxxxxxxxx>
>>
>> [...]
>>
>>>
>>> -void kvm_s390_vcpu_stop(struct kvm_vcpu *vcpu)
>>> +int kvm_s390_vcpu_stop(struct kvm_vcpu *vcpu)
>>> {
>>> - int i, online_vcpus, started_vcpus = 0;
>>> + int i, online_vcpus, r = 0, started_vcpus = 0;
>>> struct kvm_vcpu *started_vcpu = NULL;
>>>
>>> if (is_vcpu_stopped(vcpu))
>>> - return;
>>> + return 0;
>>>
>>> trace_kvm_s390_vcpu_start_stop(vcpu->vcpu_id, 0);
>>> /* Only one cpu at a time may enter/leave the STOPPED state. */
>>> @@ -4501,6 +4509,9 @@ void kvm_s390_vcpu_stop(struct kvm_vcpu *vcpu)
>>> kvm_s390_clear_stop_irq(vcpu);
>>>
>>> kvm_s390_set_cpuflags(vcpu, CPUSTAT_STOPPED);
>>> + /* Let's tell the UV that we successfully stopped the vcpu */
>>> + if (kvm_s390_pv_cpu_is_protected(vcpu))
>>> + r = kvm_s390_pv_set_cpu_state(vcpu, PV_CPU_STATE_STP);
>>> __disable_ibs_on_vcpu(vcpu);
>>>
>>> for (i = 0; i < online_vcpus; i++) {
>>> @@ -4519,7 +4530,7 @@ void kvm_s390_vcpu_stop(struct kvm_vcpu *vcpu)
>>> }
>>>
>>> spin_unlock(&vcpu->kvm->arch.start_stop_lock);
>>> - return;
>>> + return r;
>>> }
>>
>> So... you stopped the CPU, the UV call failed, and you'll return an
>> error. But you did stop the CPU. What is user space expected to do with
>> that error?
>
> If that call returns with an error the QEMU and ultravisor are out of sync.
> The only possible solution in such a case is probably to pause the guest (or
> exit with an error) or to do a system_restart.
>
> To make the system restart possible I will move the pv_set_cpu_state to the
> beginning of the function and do an early exit on error.
>
>>
>> After all, it can't retrigger a STOP, due to if (is_vcpu_stopped(vcpu)).
>> Same applies to the start path.
>
> Looks now like:
>
> @@ -4445,18 +4451,27 @@ static void __enable_ibs_on_vcpu(struct kvm_vcpu *vcpu)
> kvm_s390_sync_request(KVM_REQ_ENABLE_IBS, vcpu);
> }
>
> -void kvm_s390_vcpu_start(struct kvm_vcpu *vcpu)
> +int kvm_s390_vcpu_start(struct kvm_vcpu *vcpu)
> {
> - int i, online_vcpus, started_vcpus = 0;
> + int i, online_vcpus, r = 0, started_vcpus = 0;
>
> if (!is_vcpu_stopped(vcpu))
> - return;
> + return 0;
>
> trace_kvm_s390_vcpu_start_stop(vcpu->vcpu_id, 1);
> /* Only one cpu at a time may enter/leave the STOPPED state. */
> spin_lock(&vcpu->kvm->arch.start_stop_lock);
> online_vcpus = atomic_read(&vcpu->kvm->online_vcpus);
>
> + /* Let's tell the UV that we want to change into the operating state */
> + if (kvm_s390_pv_cpu_is_protected(vcpu)) {
> + r = kvm_s390_pv_set_cpu_state(vcpu, PV_CPU_STATE_OPR);
> + if (r) {
> + spin_unlock(&vcpu->kvm->arch.start_stop_lock);
> + return r;
> + }
> + }
> +
You can "return 0;" at the end of the function now. With the same
handling on the stop path
Reviewed-by: David Hildenbrand <david@xxxxxxxxxx>
--
Thanks,
David / dhildenb
