Re: [PATCH 4/4] KVM: Add support for in-kernel LAPIC model

["Gregory Haskins" <ghaskins-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org>]

>>> On Mon, May 7, 2007 at  6:17 AM, in message <463EFCC5.8050408-atKUWr5tajBWk0Htik3J/w@public.gmane.org>,
Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote: 
> Gregory Haskins wrote:
>> +
>> +struct kvm_kernint {
>> +	struct kvm_vcpu              *vcpu;
>> +	struct kvm_irqdevice         *self_irq;
>> +	struct kvm_irqdevice         *ext_irq;
>> +	struct kvm_irqdevice          apic_irq;
>> +
>> +};
>>   
> 
> Can you elaborate on what these are? I sort of understand ext_irq and 
> apic_irq, but what is self_irq? And why have the vcpu member?

vcpu is there so that the kernint object has proper VCPU/LAPIC context for functions like kvm_lapic_enabled()

ext/apic_irq you already understand

self_irq is a self reference to the kernints own irqdevice.  This is because the kernint object is really just an intermediate "glue" component that glues LAPIC, EXTINT, and VCPU together.  When interrupts come into the KERNINT from the LAPIC, they need to be forwarded to the VCPU as if they came from the KERNINT.  See kvm_apic_intr().

> 
>> +
>> +static struct kvm_irqdevice *get_irq_dev(struct kvm_kernint *s)
>> +{
>> +	struct kvm_irqdevice *dev;
>> +
>> +	if (kvm_lapic_enabled(s- >vcpu))
>> +		dev = &s- >apic_irq;
>> +	else
>> +		dev = s- >ext_irq;
>> +
>> +	if (!dev)
>> +		kvm_crash_guest(s- >vcpu- >kvm);
>>   
> 
> Can this happen? Doesn't there always have to be an external interrupt 
> controller when the lapic is disabled?

Well, if a non-BSP processor calls this, ext_irq could be null.  Its a pathological condition, thus the harsh punishment to the guest.  If its an AP, it *better* be using its LAPIC.  If its not, something is busted. 


> 
>> +
>> +static void kvm_ext_intr(struct kvm_irqsink *this,
>> +			 struct kvm_irqdevice *dev,
>> +			 kvm_irqpin_t pin)
>> +{
>> +	struct kvm_kernint *s = (struct kvm_kernint*)this- >private;
>> +
>> +	/*
>> +	 * If the EXTINT device sent us an interrupt, forward it to the LINT0
>> +	 * pin of the LAPIC
>> +	 */
>> +	if (pin != kvm_irqpin_localint)
>> +	    return;
>>   
> 
> Whitespace.

Fixed

> 
>> +
>> +	/*
>> +	 * "irq 0" = LINT0, 1 = LINT1
>> +	 */
>> +	kvm_irqdevice_set_pin(&s- >apic_irq, 0, 1);
>> +}
>> +
>> +int kvm_kernint_init(struct kvm_vcpu *vcpu)
>> +{
>> +	struct kvm_irqdevice *irqdev = &vcpu- >irq.dev;
>> +	struct kvm_kernint *s;
>> +
>> +	s = kzalloc(sizeof(*s), GFP_KERNEL);
>> +	if (!s)
>> +	    return - ENOMEM;
>>   
> 
> Whitespace.

Fixed

> 
>> +
>> +	s- >vcpu = vcpu;
>> +
>> +	/*
>> +	 * Configure the irqdevice interface
>> +	 */
>> +	irqdev- >ack         = kernint_irqdev_ack;
>> +	irqdev- >set_pin     = kernint_irqdev_set_pin;
>> +	irqdev- >destructor  = kernint_irqdev_destructor;
>> +
>> +	irqdev- >private = s;
>> +	s- >self_irq = irqdev;
>> +
>> +	/*
>> +	 * Configure the EXTINT device if this is the BSP processor
>> +	 */
>> +	if (!vcpu_slot(vcpu)) {
>> +		struct kvm_irqsink sink = {
>> +			.set_intr   = kvm_ext_intr,
>> +			.private    = s
>> +		};
>> +
>> +		s- >ext_irq = &vcpu- >kvm- >isa_irq;
>> +		kvm_irqdevice_register_sink(s- >ext_irq, &sink);
>> +	}
>> +
>> +	/*
>> +	 * Configure the LAPIC device
>> +	 */
>> +	kvm_irqdevice_init(&s- >apic_irq);
>> +
>> +	{
>> +		struct kvm_irqsink sink = {
>> +			.set_intr   = kvm_apic_intr,
>> +			.private    = s
>> +		};
>> +
>> +		kvm_irqdevice_register_sink(&s- >apic_irq, &sink);
>> +	}
>>   
> 
> Just move the variable definition to the top to avoid the wierd indentation.

Weirdness fixed ;)

> 
> 
>> +
>> +/*
>> + *-----------------------------------------------------------------------
>> + * KERNEL- TIMERS
>> + *
>> + * Unfortunately we really need HRTIMERs to do this right, but they might
>> + * not exist on olders kernels (sigh).  So we roughly abstract this 
> interface
>> + * to support nanosecond resolution, and then emulate it as best we can if
>> + * the real HRTIMERs arent available
>> + *-----------------------------------------------------------------------
>> + */
>>   
> 
> The correct way to use non- hrtimer kernels is to write the hrtimer api 
> in terms of the old api, in external- module- compat.h.
> 
>> +
>> +struct kvm_apic_timer {
>> +	int (*function)(void *private);
>> +	void *private;
>> +#ifdef KVM_NO_HRTIMER
>> +	struct timer_list dev;
>> +#else
>> +	struct hrtimer dev;
>> +#endif
>> +};
>>   
> 
> ... for example:
> #define hrtimer timer_list
> 
> I'd just drop it for now; it makes the code hard to read. We can re- add 
> it later.

By "drop it", I assume you mean drop the abstraction and just use native HRTIMER directly for now?  

> 
> 
> [I didn't audit the lapic code]
> 
> Where's vcpu- >cr8 gone?

I know you requested that this entry remain in the VCPU structure.  However, I couldnt make this work reasonably.  The APIC wants to maintain this value itself so the two can very easily get out of sync.  I suppose there could be ways to make the APIC use the vcpu->cr8 variable as storage for TPR (albeit messy), but this idea falls apart when we start looking at optimizations like TPR shadowing.

Based on all that, I felt it was best to just maintain CR8 as the TPR register in the model.





-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/