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

[Avi Kivity <avi@redhat.com>]

On 08/11/2009 03:31 PM, Gleb Natapov wrote:


What is the motivation for this change?

Why a spinlock and not a mutex?

> 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.

> +		} 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.

>    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.

>
> -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.

> -	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.

>
>   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