Re: [PATCH 6/8 v2] Move IO APIC to its own lock.

[Gleb Natapov <gleb@redhat.com>]

On Wed, Aug 12, 2009 at 11:27:13AM +0300, Avi Kivity wrote:
> On 08/11/2009 03:31 PM, Gleb Natapov wrote:
>
>
> What is the motivation for this change?
>
The motivation was explained in 0/0. I want to get rid of lock on
general irq injection path so the lock have to be pushed into ioapic
since multiple cpus can access it concurrently. PIC has such lock
already.

> Why a spinlock and not a mutex?
>
Protected sections are small and we do not sleep there.

>> diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
>> index 0ad09f0..dd7ef2d 100644
>> --- a/arch/ia64/kvm/kvm-ia64.c
>> +++ b/arch/ia64/kvm/kvm-ia64.c
>> @@ -850,9 +850,16 @@ static int kvm_vm_ioctl_get_irqchip(struct kvm *kvm,
>>
>>   	r = 0;
>>   	switch (chip->chip_id) {
>> -	case KVM_IRQCHIP_IOAPIC:
>> -		memcpy(&chip->chip.ioapic, ioapic_irqchip(kvm),
>> -				sizeof(struct kvm_ioapic_state));
>> +	case KVM_IRQCHIP_IOAPIC: {
>> +		struct kvm_ioapic *ioapic = ioapic_irqchip(kvm);
>> +		if (ioapic) {
>> +			spin_lock(&ioapic->lock);
>> +			memcpy(&chip->chip.ioapic, ioapic,
>> +			       sizeof(struct kvm_ioapic_state));
>> +			spin_unlock(&ioapic->lock);
>>    
>
> Better to add an accessor than to reach into internals like this.
>
Agree.

>> +		} else
>> +			r = -EINVAL;
>> +	}
>>   		break;
>>   	default:
>>   		r = -EINVAL;
>> @@ -867,10 +874,16 @@ static int kvm_vm_ioctl_set_irqchip(struct kvm *kvm, struct kvm_irqchip *chip)
>>
>>   	r = 0;
>>   	switch (chip->chip_id) {
>> -	case KVM_IRQCHIP_IOAPIC:
>> -		memcpy(ioapic_irqchip(kvm),
>> -				&chip->chip.ioapic,
>> -				sizeof(struct kvm_ioapic_state));
>> +	case KVM_IRQCHIP_IOAPIC: {
>> +		struct kvm_ioapic *ioapic = ioapic_irqchip(kvm);
>> +		if (ioapic) {
>> +			spin_lock(&ioapic->lock);
>> +			memcpy(ioapic,&chip->chip.ioapic,
>> +			       sizeof(struct kvm_ioapic_state));
>> +			spin_unlock(&ioapic->lock);
>> +		} else
>> +			r = -EINVAL;
>> +	}
>>    
>
> ... and better to deduplicate the code too.
>
>>   		break;
>>   	default:
>>   		r = -EINVAL;
>> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
>> index 01f1516..a988c0e 100644
>> --- a/arch/x86/kvm/i8259.c
>> +++ b/arch/x86/kvm/i8259.c
>> @@ -38,7 +38,9 @@ static void pic_clear_isr(struct kvm_kpic_state *s, int irq)
>>   	s->isr_ack |= (1<<  irq);
>>   	if (s !=&s->pics_state->pics[0])
>>   		irq += 8;
>> +	spin_unlock(&s->pics_state->lock);
>>   	kvm_notify_acked_irq(s->pics_state->kvm, SELECT_PIC(irq), irq);
>> +	spin_lock(&s->pics_state->lock);
>>   }
>>    
>
> Need to explain why this is safe.  I'm not sure it is, because we touch  
> state afterwards in pic_intack().  We need to do all vcpu-synchronous  
> operations before dropping the lock.
Forst pic_intack() calls pic_clear_isr() only in auto eoi mode and this mode
is already broken for assigned devices. Second for level triggered
interrupts pic_intack() does nothing after calling pic_clear_isr() and
third I can move pic_clear_isr() call to the end of pic_intack().
>
>>    void kvm_pic_clear_isr_ack(struct kvm *kvm)
>> @@ -238,7 +240,9 @@ void kvm_pic_reset(struct kvm_kpic_state *s)
>>   		if (vcpu0&&  kvm_apic_accept_pic_intr(vcpu0))
>>   			if (s->irr&  (1<<  irq) || s->isr&  (1<<  irq)) {
>>   				n = irq + irqbase;
>> +				spin_unlock(&s->pics_state->lock);
>>   				kvm_notify_acked_irq(kvm, SELECT_PIC(n), n);
>> +				spin_lock(&s->pics_state->lock);
>>    
>
> Ditto here, needs to be moved until after done changing state.
>
I am not sure this code is even needed. IOAPIC don't call notifiers on
reset.

>>
>> -static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int pin,
>> -				    int trigger_mode)
>> +static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
>> +				     int trigger_mode)
>>   {
>> -	union kvm_ioapic_redirect_entry *ent;
>> +	int i;
>> +
>> +	for (i = 0; i<  IOAPIC_NUM_PINS; i++) {
>> +		union kvm_ioapic_redirect_entry *ent =&ioapic->redirtbl[i];
>> +
>> +		if (ent->fields.vector != vector)
>> +			continue;
>>
>> -	ent =&ioapic->redirtbl[pin];
>> +		spin_unlock(&ioapic->lock);
>> +		kvm_notify_acked_irq(ioapic->kvm, KVM_IRQCHIP_IOAPIC, i);
>> +		spin_lock(&ioapic->lock);
>>
>>    
>
> I *think* we need to clear remote_irr before dropping the lock.  I  
> *know* there's a missing comment here.
I don't see why we clear remote_irr before dropping the lock. If, while
lock was dropped, interrupt was delivered to this entry it will be
injected when ack notifier returns.

>
>> -	kvm_notify_acked_irq(ioapic->kvm, KVM_IRQCHIP_IOAPIC, pin);
>> +		if (trigger_mode != IOAPIC_LEVEL_TRIG)
>> +			continue;
>>
>> -	if (trigger_mode == IOAPIC_LEVEL_TRIG) {
>>   		ASSERT(ent->fields.trig_mode == IOAPIC_LEVEL_TRIG);
>>   		ent->fields.remote_irr = 0;
>> -		if (!ent->fields.mask&&  (ioapic->irr&  (1<<  pin)))
>> -			ioapic_service(ioapic, pin);
>> +		if (!ent->fields.mask&&  (ioapic->irr&  (1<<  i)))
>> +			ioapic_service(ioapic, i);
>>   	}
>>   }
>>    
>
> To make the patch easier to read, suggest keeping the loop in the other  
> function.
>
I don't follow. All __kvm_ioapic_update_eoi() contains is the loop, so
the loop is already in its own function. Do you mean move the context of
the loop into the other function and leave only for(;;) fun(); in
__kvm_ioapic_update_eoi()?

>>
>>   void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode)
>>   {
>>   	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
>> -	int i;
>>
>> -	for (i = 0; i<  IOAPIC_NUM_PINS; i++)
>> -		if (ioapic->redirtbl[i].fields.vector == vector)
>> -			__kvm_ioapic_update_eoi(ioapic, i, trigger_mode);
>> +	spin_lock(&ioapic->lock);
>> +	__kvm_ioapic_update_eoi(ioapic, vector, trigger_mode);
>> +	spin_unlock(&ioapic->lock);
>>   }
>>
>>    
>
> -- 
> error compiling committee.c: too many arguments to function

--
			Gleb.