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

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

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?

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

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

> +
> +	/*
> +	 * "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.

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


> +
> +/*
> + *-----------------------------------------------------------------------
> + * 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.


[I didn't audit the lapic code]

Where's vcpu->cr8 gone?


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


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