Linux Virtualization - Jun 2008 - PCI device assignment to guests

The main change from the patches I sent out earlier this week is support for
guests that use the PIC. A callback for PIC irq ack handling is also introduced.

Currently, there's no mechanism to register/unregister callers to the irq
ack callbacks, but they can be added when there's more than one user for the
functionality.

Please review.

This function injects an interrupt into the guest given the kvm struct,
the (guest) irq number and the interrupt level.

Signed-off-by: Amit Shah <amit.shah at qumranet.com>
---
 arch/x86/kvm/irq.c |   11 +++++++++++
 arch/x86/kvm/irq.h |    2 ++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
index 76d736b..0d9e552 100644
--- a/arch/x86/kvm/irq.c
+++ b/arch/x86/kvm/irq.c
@@ -100,3 +100,14 @@ void __kvm_migrate_timers(struct kvm_vcpu *vcpu)
 	__kvm_migrate_apic_timer(vcpu);
 	__kvm_migrate_pit_timer(vcpu);
 }
+
+/* This should be called with the kvm->lock mutex held */
+void kvm_set_irq(struct kvm *kvm, int irq, int level)
+{
+	/* Not possible to detect if the guest uses the PIC or the
+	 * IOAPIC.  So set the bit in both. The guest will ignore
+	 * writes to the unused one.
+	 */
+	kvm_ioapic_set_irq(kvm->arch.vioapic, irq, level);
+	kvm_pic_set_irq(pic_irqchip(kvm), irq, level);
+}
diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index 2a15be2..ba4e3bf 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -80,6 +80,8 @@ static inline int irqchip_in_kernel(struct kvm *kvm)
 
 void kvm_pic_reset(struct kvm_kpic_state *s);
 
+void kvm_set_irq(struct kvm *kvm, int irq, int level);
+
 void kvm_timer_intr_post(struct kvm_vcpu *vcpu, int vec);
 void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu);
 void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu);
-- 
1.5.4.3

This will be useful for acking irqs of assigned devices

Signed-off-by: Amit Shah <amit.shah at qumranet.com>
---
 virt/kvm/ioapic.c |    3 +++
 virt/kvm/ioapic.h |    1 +
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 9d02136..6d99a35 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -295,6 +295,9 @@ static void __kvm_ioapic_update_eoi(struct kvm_ioapic
*ioapic, int gsi)
 	ent->fields.remote_irr = 0;
 	if (!ent->fields.mask && (ioapic->irr & (1 << gsi)))
 		ioapic_deliver(ioapic, gsi);
+
+	if (ioapic->ack_notifier)
+		ioapic->ack_notifier(ioapic->kvm, gsi);
 }
 
 void kvm_ioapic_update_eoi(struct kvm *kvm, int vector)
diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
index 7f16675..a42743f 100644
--- a/virt/kvm/ioapic.h
+++ b/virt/kvm/ioapic.h
@@ -58,6 +58,7 @@ struct kvm_ioapic {
 	} redirtbl[IOAPIC_NUM_PINS];
 	struct kvm_io_device dev;
 	struct kvm *kvm;
+	void (*ack_notifier)(void *opaque, int irq);
 };
 
 #ifdef DEBUG
-- 
1.5.4.3

This is useful for acking irqs of assigned devices

Signed-off-by: Amit Shah <amit.shah at qumranet.com>
---
 arch/x86/kvm/i8259.c |    6 +++++-
 arch/x86/kvm/irq.c   |    2 +-
 arch/x86/kvm/irq.h   |    3 ++-
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 5857f59..3ba5e5c 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -151,9 +151,10 @@ static inline void pic_intack(struct kvm_kpic_state *s, int
irq)
 		s->irr &= ~(1 << irq);
 }
 
-int kvm_pic_read_irq(struct kvm_pic *s)
+int kvm_pic_read_irq(struct kvm *kvm)
 {
 	int irq, irq2, intno;
+	struct kvm_pic *s = pic_irqchip(kvm);
 
 	irq = pic_get_irq(&s->pics[0]);
 	if (irq >= 0) {
@@ -178,6 +179,9 @@ int kvm_pic_read_irq(struct kvm_pic *s)
 		irq = 7;
 		intno = s->pics[0].irq_base + irq;
 	}
+	if (kvm->arch.vpic->ack_notifier)
+		kvm->arch.vpic->ack_notifier(kvm, irq);
+
 	pic_update_irq(s);
 
 	return intno;
diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
index 0d9e552..3529620 100644
--- a/arch/x86/kvm/irq.c
+++ b/arch/x86/kvm/irq.c
@@ -72,7 +72,7 @@ int kvm_cpu_get_interrupt(struct kvm_vcpu *v)
 		if (kvm_apic_accept_pic_intr(v)) {
 			s = pic_irqchip(v->kvm);
 			s->output = 0;		/* PIC */
-			vector = kvm_pic_read_irq(s);
+			vector = kvm_pic_read_irq(v->kvm);
 		}
 	}
 	return vector;
diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index ba4e3bf..bef9127 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -61,11 +61,12 @@ struct kvm_pic {
 	void *irq_request_opaque;
 	int output;		/* intr from master PIC */
 	struct kvm_io_device dev;
+	void (*ack_notifier)(void *opaque, int irq);
 };
 
 struct kvm_pic *kvm_create_pic(struct kvm *kvm);
 void kvm_pic_set_irq(void *opaque, int irq, int level);
-int kvm_pic_read_irq(struct kvm_pic *s);
+int kvm_pic_read_irq(struct kvm *kvm);
 void kvm_pic_update_irq(struct kvm_pic *s);
 
 static inline struct kvm_pic *pic_irqchip(struct kvm *kvm)
-- 
1.5.4.3

Amit Shah wrote:
> This function injects an interrupt into the guest given the kvm struct,
> the (guest) irq number and the interrupt level.
>
> Signed-off-by: Amit Shah <amit.shah at qumranet.com>
> ---
>  arch/x86/kvm/irq.c |   11 +++++++++++
>  arch/x86/kvm/irq.h |    2 ++
>  2 files changed, 13 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
> index 76d736b..0d9e552 100644
> --- a/arch/x86/kvm/irq.c
> +++ b/arch/x86/kvm/irq.c
> @@ -100,3 +100,14 @@ void __kvm_migrate_timers(struct kvm_vcpu *vcpu)
>  	__kvm_migrate_apic_timer(vcpu);
>  	__kvm_migrate_pit_timer(vcpu);
>  }
> +
> +/* This should be called with the kvm->lock mutex held */
> +void kvm_set_irq(struct kvm *kvm, int irq, int level)
> +{
> +	/* Not possible to detect if the guest uses the PIC or the
> +	 * IOAPIC.  So set the bit in both. The guest will ignore
> +	 * writes to the unused one.
> +	 */
> +	kvm_ioapic_set_irq(kvm->arch.vioapic, irq, level);
> +	kvm_pic_set_irq(pic_irqchip(kvm), irq, level);
> +}
>   
pic supports only irqs 0-15.

I guess instead of adding the check here, better move it into i8259.c 
and ioapic.c.


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

On Sat, 2008-07-05 at 14:05 +0300, Avi Kivity wrote:
> Amit Shah wrote:
> > This function injects an interrupt into the guest given the kvm
struct,
> > the (guest) irq number and the interrupt level.
> >
> > Signed-off-by: Amit Shah <amit.shah at qumranet.com>
> > ---
> >  arch/x86/kvm/irq.c |   11 +++++++++++
> >  arch/x86/kvm/irq.h |    2 ++
> >  2 files changed, 13 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
> > index 76d736b..0d9e552 100644
> > --- a/arch/x86/kvm/irq.c
> > +++ b/arch/x86/kvm/irq.c
> > @@ -100,3 +100,14 @@ void __kvm_migrate_timers(struct kvm_vcpu *vcpu)
> >  	__kvm_migrate_apic_timer(vcpu);
> >  	__kvm_migrate_pit_timer(vcpu);
> >  }
> > +
> > +/* This should be called with the kvm->lock mutex held */
> > +void kvm_set_irq(struct kvm *kvm, int irq, int level)
> > +{
> > +	/* Not possible to detect if the guest uses the PIC or the
> > +	 * IOAPIC.  So set the bit in both. The guest will ignore
> > +	 * writes to the unused one.
> > +	 */
> > +	kvm_ioapic_set_irq(kvm->arch.vioapic, irq, level);
> > +	kvm_pic_set_irq(pic_irqchip(kvm), irq, level);
> > +}
> >   
> 
> pic supports only irqs 0-15.
> 
> I guess instead of adding the check here, better move it into i8259.c 
> and ioapic.c.
> 
> 
Do you mean something like the patch below?
( in the ioapic code there is already a check for the irq: ?if (irq >= 0
&& irq < IOAPIC_NUM_PINS) )

Thanks,
Ben

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 3ba5e5c..c2c4884 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -130,8 +130,10 @@ void kvm_pic_set_irq(void *opaque, int irq, int level)
 {
 	struct kvm_pic *s = opaque;
 
-	pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
-	pic_update_irq(s);
+	if (irq >= 0 && irq < PIC_NUM_PINS) {
+		pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
+		pic_update_irq(s);
+	}
 }
 
 /*
diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index bef9127..e9c7b3f 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -30,6 +30,8 @@
 #include "ioapic.h"
 #include "lapic.h"
 
+#define PIC_NUM_PINS 15
+
 struct kvm;
 struct kvm_vcpu;

Ben-Ami Yassour wrote:
> On Sat, 2008-07-05 at 14:05 +0300, Avi Kivity wrote:
>   
>> Amit Shah wrote:
>>     
>>> This function injects an interrupt into the guest given the kvm
struct,
>>> the (guest) irq number and the interrupt level.
>>>
>>> Signed-off-by: Amit Shah <amit.shah at qumranet.com>
>>> ---
>>>  arch/x86/kvm/irq.c |   11 +++++++++++
>>>  arch/x86/kvm/irq.h |    2 ++
>>>  2 files changed, 13 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
>>> index 76d736b..0d9e552 100644
>>> --- a/arch/x86/kvm/irq.c
>>> +++ b/arch/x86/kvm/irq.c
>>> @@ -100,3 +100,14 @@ void __kvm_migrate_timers(struct kvm_vcpu
*vcpu)
>>>  	__kvm_migrate_apic_timer(vcpu);
>>>  	__kvm_migrate_pit_timer(vcpu);
>>>  }
>>> +
>>> +/* This should be called with the kvm->lock mutex held */
>>> +void kvm_set_irq(struct kvm *kvm, int irq, int level)
>>> +{
>>> +	/* Not possible to detect if the guest uses the PIC or the
>>> +	 * IOAPIC.  So set the bit in both. The guest will ignore
>>> +	 * writes to the unused one.
>>> +	 */
>>> +	kvm_ioapic_set_irq(kvm->arch.vioapic, irq, level);
>>> +	kvm_pic_set_irq(pic_irqchip(kvm), irq, level);
>>> +}
>>>   
>>>       
>> pic supports only irqs 0-15.
>>
>> I guess instead of adding the check here, better move it into i8259.c 
>> and ioapic.c.
>>
>>
>>     
>
> Do you mean something like the patch below?
> ( in the ioapic code there is already a check for the irq: ?if (irq >= 0
&& irq < IOAPIC_NUM_PINS) )
>
> Thanks,
> Ben
>
> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> index 3ba5e5c..c2c4884 100644
> --- a/arch/x86/kvm/i8259.c
> +++ b/arch/x86/kvm/i8259.c
> @@ -130,8 +130,10 @@ void kvm_pic_set_irq(void *opaque, int irq, int level)
>  {
>  	struct kvm_pic *s = opaque;
>  
> -	pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
> -	pic_update_irq(s);
> +	if (irq >= 0 && irq < PIC_NUM_PINS) {
> +		pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
> +		pic_update_irq(s);
> +	}
>  }
>  
>  /*
> diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
> index bef9127..e9c7b3f 100644
> --- a/arch/x86/kvm/irq.h
> +++ b/arch/x86/kvm/irq.h
> @@ -30,6 +30,8 @@
>  #include "ioapic.h"
>  #include "lapic.h"
>  
> +#define PIC_NUM_PINS 15
> +
>   
16

Also, needs signoff and changelog entry.


-- 
error compiling committee.c: too many arguments to function

Amit Shah wrote:
> This will be useful for acking irqs of assigned devices
>
>   
And also for improving time drift tracking.

Please make this more generic by having a list of callbacks.  There 
could also be just one list, rather than one for the ioapic and one for 
the pic as implemented now.

It may also make sense to filter the irq number before calling the 
callback rather than relying on the callback to ignore uninteresting irqs.
> Signed-off-by: Amit Shah <amit.shah at qumranet.com>
> ---
>  virt/kvm/ioapic.c |    3 +++
>  virt/kvm/ioapic.h |    1 +
>  2 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index 9d02136..6d99a35 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -295,6 +295,9 @@ static void __kvm_ioapic_update_eoi(struct kvm_ioapic
*ioapic, int gsi)
>  	ent->fields.remote_irr = 0;
>  	if (!ent->fields.mask && (ioapic->irr & (1 <<
gsi)))
>  		ioapic_deliver(ioapic, gsi);
> +
> +	if (ioapic->ack_notifier)
> +		ioapic->ack_notifier(ioapic->kvm, gsi);
>  }
>  
>  void kvm_ioapic_update_eoi(struct kvm *kvm, int vector)
> diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
> index 7f16675..a42743f 100644
> --- a/virt/kvm/ioapic.h
> +++ b/virt/kvm/ioapic.h
> @@ -58,6 +58,7 @@ struct kvm_ioapic {
>  	} redirtbl[IOAPIC_NUM_PINS];
>  	struct kvm_io_device dev;
>  	struct kvm *kvm;
> +	void (*ack_notifier)(void *opaque, int irq);
>  };
>  
>  #ifdef DEBUG
>   

-- 
error compiling committee.c: too many arguments to function

On Thu, 2008-07-10 at 16:57 +0300, Avi Kivity wrote:
> Ben-Ami Yassour wrote:
> > On Mon, 2008-07-07 at 13:08 +0300, Avi Kivity wrote:
> >   
> >> Amit Shah wrote:
> >>     
> >>> This will be useful for acking irqs of assigned devices
> >>>
> >>>   
> >>>       
> >> And also for improving time drift tracking.
> >>
> >> Please make this more generic by having a list of callbacks. 
There
> >> could also be just one list, rather than one for the ioapic and
one for
> >> the pic as implemented now.
> >>
> >> It may also make sense to filter the irq number before calling the
> >> callback rather than relying on the callback to ignore
uninteresting irqs.
> >>
> >>     
> > Avi,
> >
> > Did you mean something like the patch below?
> >   
> 
> I did, and have something very similar queued.
The notification list might help simplify the assigned device code.
Are you planning to merge the patch you have queued, or should I use the
one that I sent you?

Thanks,
Ben