Re: [PATCH v4 28/36] KVM: s390: protvirt: Report CPU state to Ultravisor

[Christian Borntraeger <borntraeger@xxxxxxxxxx>]

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;
+               }
+       }
+
        for (i = 0; i < online_vcpus; i++) {
                if (!is_vcpu_stopped(vcpu->kvm->vcpus[i]))
                        started_vcpus++;
@@ -4481,22 +4496,31 @@ void kvm_s390_vcpu_start(struct kvm_vcpu *vcpu)
         */
        kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
        spin_unlock(&vcpu->kvm->arch.start_stop_lock);
-       return;
+       return r;
 }