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

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

On 2013-03-04 19:08, Gleb Natapov wrote:
> On Sun, Mar 03, 2013 at 09:21:43PM +0100, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> A VCPU sending INIT or SIPI to some other VCPU races for setting the
>> remote VCPU's mp_state. When we were unlucky, KVM_MP_STATE_INIT_RECEIVED
>> was overwritten by kvm_emulate_halt and, thus, got lost.
>>
>> Fix this by raising requests on the sender side that will then be
>> handled synchronously over the target VCPU context.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> Turned out to be simpler than expected. I'm no longer able to reproduce
>> the race I saw before.
>>
>>  arch/x86/kvm/lapic.c     |    9 ++++-----
>>  arch/x86/kvm/x86.c       |   16 +++++++++++++++-
>>  include/linux/kvm_host.h |    2 ++
>>  3 files changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 02b51dd..be1e37a 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -731,8 +731,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>>  	case APIC_DM_INIT:
>>  		if (!trig_mode || level) {
>>  			result = 1;
>> -			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
>> -			kvm_make_request(KVM_REQ_EVENT, vcpu);
>> +			kvm_make_request(KVM_REQ_INIT, vcpu);
>>  			kvm_vcpu_kick(vcpu);
>>  		} else {
>>  			apic_debug("Ignoring de-assert INIT to vcpu %d\n",
>> @@ -743,11 +742,11 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>>  	case APIC_DM_STARTUP:
>>  		apic_debug("SIPI to vcpu %d vector 0x%02x\n",
>>  			   vcpu->vcpu_id, vector);
>> -		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
>> +		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED ||
>> +		    test_bit(KVM_REQ_INIT, &vcpu->requests)) {
>>  			result = 1;
>>  			vcpu->arch.sipi_vector = vector;
>> -			vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
>> -			kvm_make_request(KVM_REQ_EVENT, vcpu);
>> +			kvm_make_request(KVM_REQ_SIPI, vcpu);
>>  			kvm_vcpu_kick(vcpu);
>>  		}
>>  		break;
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index d0cf737..8c8843c 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -5641,6 +5641,18 @@ static void update_eoi_exitmap(struct kvm_vcpu *vcpu)
>>  	kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
>>  }
>>  
>> +static bool kvm_check_init_and_sipi(struct kvm_vcpu *vcpu)
>> +{
>> +	if (kvm_check_request(KVM_REQ_INIT, vcpu))
>> +		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
>> +	if (kvm_check_request(KVM_REQ_SIPI, vcpu) &&
>> +	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
>> +		vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
>> +		return true;
>> +	}
>> +	return false;
>> +}
>> +
>>  static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>>  {
>>  	int r;
>> @@ -5649,6 +5661,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>>  	bool req_immediate_exit = 0;
>>  
>>  	if (vcpu->requests) {
>> +		kvm_check_init_and_sipi(vcpu);
>>  		if (kvm_check_request(KVM_REQ_MMU_RELOAD, vcpu))
>>  			kvm_mmu_unload(vcpu);
>>  		if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu))
>> @@ -6977,10 +6990,11 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
>>  
>>  int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
>>  {
>> +	if (kvm_check_init_and_sipi(vcpu))
>> +		return 1;
>>  	return (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE &&
>>  		!vcpu->arch.apf.halted)
>>  		|| !list_empty_careful(&vcpu->async_pf.done)
>> -		|| vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED
>>  		|| atomic_read(&vcpu->arch.nmi_queued) ||
>>  		(kvm_arch_interrupt_allowed(vcpu) &&
>>  		 kvm_cpu_has_interrupt(vcpu));
> This makes two subsequent calls to kvm_arch_vcpu_runnable() return
> different values if SIPI is pending. While it may not cause problem to
> current code (I haven't thought it through) with such semantics you
> gonna have a bad time.

If I manage to follow Paolo's suggestion to eliminate the SIPI_RECEIVED
state and all the staged logic around it, that might change. Will be
more invasive but likely cleaner in its result.

Jan

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