On 08.01.20 15:35, Janosch Frank wrote:
> On 1/8/20 3:28 PM, Christian Borntraeger wrote:
>>
>>
>> On 05.12.19 13:09, Janosch Frank wrote:
>> [...]
>>> +4.123 KVM_S390_CLEAR_RESET
>>> +
>>> +Capability: KVM_CAP_S390_VCPU_RESETS
>>> +Architectures: s390
>>> +Type: vcpu ioctl
>>> +Parameters: none
>>> +Returns: 0
>>> +
>>> +This ioctl resets VCPU registers and control structures that QEMU
>>> +can't access via the kvm_run structure. The clear reset is a superset
>>> +of the initial reset and additionally clears general, access, floating
>>> +and vector registers.
>>
>> As Thomas outlined, make it more obvious that userspace does the remaining
>> parts. I do not think that we want the kernel to do the things (unless it
>> helps you in some way for the ultravisor guests)
>
> Ok, will do
I changed my mind (see my other mail) but I would like Thomas, Conny or David
to ack/nack.
>
>>
>> [...]
>>>
>>> +static int kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu)
>>> +{
>>> + kvm_clear_async_pf_completion_queue(vcpu);
>>> + kvm_s390_clear_local_irqs(vcpu);
>>> + return 0;
>>
>> Shouldnt we also do
>> if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
>> kvm_s390_vcpu_stop(vcpu);
>>
>> here?
>
> Isn't userspace cpu state ctrl basically a prereq anyway for getting
> those new ioctls?
I do not see a reason why we should mandate userspace cpu state for the normal
reset. Using this if also enables the fallthrough below - which I like.
>
>>
>>
>>> +}
>>> +
>>> static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
>>> {
>>> kvm_s390_vcpu_initial_reset(vcpu);
>>> @@ -4363,9 +4371,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>>> r = kvm_arch_vcpu_ioctl_set_initial_psw(vcpu, psw);
>>> break;
>>> }
>>> +
>>> + case KVM_S390_CLEAR_RESET:
>>> + /* fallthrough */
>>> case KVM_S390_INITIAL_RESET:
>>> r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
>>> break;
>>
>> Then we could also do a fallthrough here and do:
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index d9e6bf3..c715ae3 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -2867,10 +2867,6 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu)
>> vcpu->arch.sie_block->pp = 0;
>> vcpu->arch.sie_block->fpf &= ~FPF_BPBC;
>> vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID;
>> - kvm_clear_async_pf_completion_queue(vcpu);
>> - if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
>> - kvm_s390_vcpu_stop(vcpu);
>> - kvm_s390_clear_local_irqs(vcpu);
>> }
>>
>> void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>>
>>
>>
>>
>>> + case KVM_S390_NORMAL_RESET:
>>> + r = kvm_arch_vcpu_ioctl_normal_reset(vcpu);
>>> + break;
>>> case KVM_SET_ONE_REG:
>>> case KVM_GET_ONE_REG: {
>
>
