Re: [PATCH] KVM: add kvm_arch_cpu_kick

["Radim Krčmář" <rkrcmar@redhat.com>]

2017-02-20 12:35+0100, David Hildenbrand:
> Am 20.02.2017 um 12:12 schrieb Christian Borntraeger:
>> On 02/17/2017 06:10 PM, David Hildenbrand wrote:
>>>
>>>>> Yes, it would.  There's some parallel with QEMU's qemu_cpu_kick, where
>>>>> the signal would be processed immediately after entering KVM_RUN.
>>>>
>>>> Something like 
>>>>
>>>> ---snip-----
>>>>         struct kvm_s390_sie_block *scb = READ_ONCE(vcpu->arch.vsie_block);
>>>>
>>>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>>>>         if (scb)
>>>> 		atomic_or(CPUSTAT_STOP_INT, &scb->cpuflags);
>>>> ---snip-----
>>>>
>>>> or 
>>>> ---snip-----
>>>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>>>> 	kvm_s390_vsie_kick(vcpu);
>>>> ---snip-----
>>>
>>> I'd go for the latter one. Keep the vsie stuff isolated. Please note
>> 
>> Yes makes sense.
>> 
>> Radim, if you go with this patch something like this can be used as the
>> s390 variant of kvm_arch_cpu_kick:
>> 
>> ---snip---
>> 	/*
>> 	 * The stop indication is reset in the interrupt code. As the CPU
>> 	 * loop handles requests after interrupts, we will
>> 	 * a: miss the request handler and enter the guest, but then the
>> 	 * stop request will exit the CPU and handle the request in the next
>> 	 * round or
>> 	 * b: handle the request directly before entering the guest
>> 	 */
>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>> 	kvm_s390_vsie_kick(vcpu);
>> 
>> ---snip---
>> feel free to add that to your patch. I can also send a fixup patch later
>> on if you prefer that.
> 
> kvm_arch_vcpu_should_kick() then also has to be changed to return 1 for now.
> 
> An interesting thing to note is how vcpu->cpu is used.
> 
> Again, as s390x can preempt just before entering the guest, vcpu_kick()
> might see vcpu->cpu = -1. Therefore, kvm_arch_vcpu_should_kick() won't
> even be called. So our cpu might go into guest mode and stay there
> longer than expected (as we won't kick it).
> 
> On x86, it is the following way:
> 
> If vcpu->cpu is -1, no need to kick the VCPU. It will check for requests
> when preemption is disabled, therefore when rescheduled.
> 
> If vcpu->cpu is set, kvm_arch_vcpu_should_kick() remembers if the VCPU
> has already been kicked while in the critical section. It will get
> kicked by smp resched as soon as entering guest mode.
> 
> So here, disabled preemption + checks in the section with disabled
> preemption (for requests and EXITING_GUEST_MODE) make sure that the
> guest will leave guest mode and process requests in a timely fashion.
> 
> On s390x, this is not 100% true. vcpu->cpu cannot be used as an
> indicator whether a kick is necessary. Either that is ok for now, or the
> vcpu->cpu != -1 check has to be disabled for s390x, e.g. by moving the
> check into kvm_arch_vcpu_should_kick().

Good point.

So s390 doesn't need vcpu->cpu and only sets it because other arches do?

And do I understand it correctly that the s390 SIE block operations have
no side-effect, apart from changed memory, when outside of guest-mode?
(We have cpu->mode mostly because interrupts are expensive. :])

In the end, I'd like to use kvm_vcpu_kick() for kvm_s390_vcpu_wakeup().
s390 sets vcpu->preempted to get a performance boost, which makes
touching it less than desirable ...
On s390, vcpu->preempted is only used in __diag_time_slice_end(), which
seems to be a type of spinning-on-a-taken-lock hypercall -- any reason
why that optimization shouldn't work on other architectures?

Thanks.