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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
