Re: [PATCH v2] KVM: x86: Convert INIT and SIPI signals into synchronously handled requests

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2013-03-05 11:50, Paolo Bonzini wrote:
> Il 05/03/2013 10:37, Gleb Natapov ha scritto:
>>>>> Not at all. I'm keeping the state in a single place, mp_state. I just
>>>>> have to make sure that I do not loose asynchronous events - what INIT
>>>>> and SIPI are.
>>>>
>>>> As evident from this code:
>>>>  +           if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED ||
>>>>  +               test_bit(KVM_REQ_INIT, &vcpu->requests)) {
>>>> the state is in two places.
>>> That's just to protect the content of sipi_vector during delivery.
> 
> Is that even needed?  Can we just do an unconditional:
> 
> 	vcpu->arch.sipi_vector = vector;
> 	/* make sure sipi_vector is visible for the receiver */
> 	smp_wmb();
> 	kvm_make_request(KVM_REQ_SIPI, vcpu);
> 	kvm_vcpu_kick(vcpu);
> 
> and check in the request handler that we did get an INIT?
> 
>>> We could drop the complete if clause if we protected that variable differently.
>>
>> I understand why the code is here. I am saying that this is the evidence
>> that the state is in two places.
> 
> I agree with Gleb here.  The state is in two places.  I'm not saying that
> using requests is wrong, it sounds nice especially for nested INIT.  But
> there is something nasty in the patches so far.
> 
> BTW checking the requests in kvm_apic_has_interrupt seems nicer than
> synchronizing in kvm_arch_cpu_runnable; it is a bit ugly to have side
> effects in kvm_arch_cpu_runnable.  Then you can actually _process_
> the requests only in vcpu_enter_guest and kvm_arch_vcpu_ioctl_get_mpstate().
> In kvm_arch_vcpu_ioctl_put_mpstate(), KVM_MP_STATE_SIPI_RECEIVED has to
> become KVM_MP_STATE_INIT_RECEIVED with KVM_REQ_SIPI set.

An INIT or SIPI signal is not really an interrupt in the sense that
kvm_cpu_has_interrupt or kvm_get_apic_interrupt expect (the current
users of kvm_apic_has_interrupt). I think we would move some ugliness
around this way, not yet resolve it.

But it probably makes sense to refactor the kvm_check_init_and_sipi
service to something that the APIC provide (is that what you are
thinking of, Gleb?). That will not reduce the number of places where we
have to check, but it should encapsulate things a bit cleaner.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux