Re: apic+ioapiuc patch

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

Dong, Eddie wrote:
> Avi:
> 	Per our discussion, we will only support all user level irqchip
> or all kernel level irqchip.  
> Here is the patch against lapic2 that passed RHEL5U test. Please give
> comments.
>
>
> thx,eddie
>
>   

General comments:

 - please split into a hlt patch, lapic patch, and ioapic patch.  the
last patch can enable the irqchip capability, but intermediate results
have to be compilable.
 - document files that were taken from Xen or qemu; specify what
revision was used as base


More comments below.

>
>
>
> diff --git a/drivers/kvm/Makefile b/drivers/kvm/Makefile
> index 952dff3..b29651b 100644
> --- a/drivers/kvm/Makefile
> +++ b/drivers/kvm/Makefile
> @@ -2,7 +2,7 @@
>  # Makefile for Kernel-based Virtual Machine module
>  #
>  
> -kvm-objs := kvm_main.o mmu.o x86_emulate.o i8259.o irq.o
> +kvm-objs := kvm_main.o mmu.o x86_emulate.o i8259.o kvm_irq.o lapic.o
> ioapic.o
>   

irq.c was renamed to kvm_irq.c? why?

> +
> +static void ioapic_inj_irq(struct kvm_ioapic *ioapic,
> +			   struct kvm_lapic *target,
> +			   u8 vector, u8 trig_mode, u8 delivery_mode)
> +{
> +	ioapic_debug("irq %d trig %d deliv %d", vector, trig_mode,
> +		     delivery_mode);
> +
> +	ASSERT((delivery_mode == dest_Fixed) ||
> +	       (delivery_mode == dest_LowestPrio));
> +
> +	if (kvm_apic_set_irq(target, vector, trig_mode))
> +		kvm_vcpu_kick(target->vcpu);
> +}
>   

Put kvm_vcpu_kick() into kvm_apic_set_irq() so that callers need not do
that themselves.

> +
> +	switch (delivery_mode) {
> +	case dest_LowestPrio:
>   

Wierd constant.  How about IOAPIC_DEST_LOWEST_PRIO?

> +		target =
> +		    kvm_apic_round_robin(ioapic->kvm, vector,
> deliver_bitmask);
> +		if (target != NULL) {
> +			ioapic_inj_irq(ioapic, target, vector,
> +				       trig_mode, delivery_mode);
> +		} else {
> +			ioapic_debug("null round robin: "
> +				     "mask=%x vector=%x
> delivery_mode=%x",
> +				     deliver_bitmask, vector,
> dest_LowestPrio);
> +		}
>   

Unnecessary {}.

> +
> +static int get_eoi_gsi(struct kvm_ioapic *ioapic, int vector)
> +{
> +	int i;
> +
> +	for (i = 0; i < IOAPIC_NUM_PINS; i++)
> +		if (ioapic->redirtbl[i].fields.vector == vector)
> +			return i;
> +
> +	return -1;
> +}
>   

blank line.

> +void kvm_ioapic_update_eoi(struct kvm *kvm, int vector)
> +{
> +	struct kvm_ioapic *ioapic = kvm->vioapic;
> +	union ioapic_redir_entry *ent;
> +	int gsi;
> +
> +	gsi = get_eoi_gsi(ioapic, vector);
> +	if (gsi == -1) {
> +		printk(KERN_WARNING "Can't find redir item for %d
> EOI\n",
> +		       vector);
> +		return;
> +	}
> +
> +	ent = &ioapic->redirtbl[gsi];
> +	ASSERT(ent->fields.trig_mode == IOAPIC_LEVEL_TRIG);
> +
> +	ent->fields.remote_irr = 0;
> +	if (!ent->fields.mask && (ioapic->irr & (1 << gsi))) {
> +		ioapic_deliver(ioapic, gsi);
> +	}
>   

excess braces.


> +
> +}
> +
> +static int ioapic_in_range(struct kvm_io_device *this, gpa_t addr)
> +{
> +	struct kvm_ioapic *ioapic = (struct kvm_ioapic *)this->private;
> +
> +	return ((addr >= ioapic->base_address &&
> +		 (addr < ioapic->base_address + IOAPIC_MEM_LENGTH)));
> +}
> +
> +static void ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr,
> int len,
> +			     void *val)
> +{
> +	struct kvm_ioapic *ioapic = (struct kvm_ioapic *)this->private;
> +	u32 result;
> +
> +	ioapic_debug("addr %lx", (unsigned long)addr);
> +	ASSERT(!(addr & 0xf));	/* check alignment */
> +
> +	addr &= 0xff;
> +
> +	switch (addr) {
> +	case IOAPIC_REG_SELECT:
> +		result = ioapic->ioregsel;
> +		break;
> +
> +	case IOAPIC_REG_WINDOW:
> +		result = ioapic_read_indirect(ioapic, addr, len);
> +		break;
> +
> +	default:
> +		result = 0;
> +		break;
> +	}
> +	switch (len) {
> +	case 1:
> +	case 2:
> +	case 4:
> +	case 8:
> +		memcpy(val, (char *)&result, len);
>   

If len == 8, you're copying a bit of kernel stack into the guest.  While
it's hardly a security hole, we'd better not do that.

> +static void vcpu_kick_intr(void *info)
> +{
> +#ifdef DEBUG
> +	struct kvm_vcpu *vcpu = (struct kvm_vcpu *)info;
> +	printk(KERN_DEBUG "vcpu_kick_intr %p \n", vcpu);
> +#endif
> +}
> +
> +void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
> +{
> +	int ipi_pcpu = vcpu->cpu;
> +
> +	if (vcpu->guest_mode)
> +		smp_call_function_single(ipi_pcpu, vcpu_kick_intr, vcpu,
> 0, 0);
> +}
>   

What if it's in hlt state?

> +
> diff --git a/drivers/kvm/irq.h b/drivers/kvm/irq.h
> index a6b3869..4b1b4b7 100644
> --- a/drivers/kvm/irq.h
> +++ b/drivers/kvm/irq.h
> @@ -26,12 +26,11 @@
>  
>  typedef void irq_request_func(void *opaque, int level);
>  
> -struct kvm_pic;
>  struct kvm_pic_state {
> -	u8 last_irr;	/* edge detection */
> -	u8 irr;		/* interrupt request register */
> -	u8 imr;		/* interrupt mask register */
> -	u8 isr;		/* interrupt service register */
> +	u8 last_irr;		/* edge detection */
> +	u8 irr;			/* interrupt request register */
> +	u8 imr;			/* interrupt mask register */
> +	u8 isr;			/* interrupt service register */
>  	u8 priority_add;	/* highest irq priority */
>  	u8 irq_base;
>  	u8 read_reg_select;
> @@ -48,7 +47,7 @@ struct kvm_pic_state {
>  };
>  
>  struct kvm_pic {
> -	struct kvm_pic_state pics[2]; /* 0 is master pic, 1 is slave pic
> */
> +	struct kvm_pic_state pics[2];	/* 0 is master pic, 1 is slave
> pic */
>  	irq_request_func *irq_request;
>  	void *irq_request_opaque;
>  	int output;		/* intr from master PIC */
>   

Please separate the pic changes so I can fold them into the existing pic
patch.

> +
> +struct kvm_ioapic {
> +	struct kvm_io_device dev;
> +	unsigned long base_address;
> +	struct kvm *kvm;
> +	u32 ioregsel;
> +	u32 id;
> +	u32 irr;
> +	union ioapic_redir_entry {
> +		u64 bits;
> +		struct {
> +			u8 vector;
> +			u8 delivery_mode:3;
> +			u8 dest_mode:1;
> +			u8 delivery_status:1;
> +			u8 polarity:1;
> +			u8 remote_irr:1;
> +			u8 trig_mode:1;
> +			u8 mask:1;
> +			u8 reserve:7;
> +			u8 reserved[4];
> +			u8 dest_id;
> +		} fields;
> +	} redirtbl[IOAPIC_NUM_PINS];
> +};
>   

Which lock protects this?

> +
> +struct kvm_lapic {
> +	spinlock_t lock;	/* TODO: need? */
>   

I think not.  Maybe when we have msi support?

> +
> +#ifdef DEBUG
> +#define ASSERT(x)
> \
> +	if (!(x)) {
> \
> +		printk(KERN_EMERG "assertion failed %s: %d: %s\n",
> \
> +		       __FILE__, __LINE__, #x);
> \
> +		BUG();
> \
> +	}
>   

Wrap in a do { } while (0) to avoid surprises.


>  #endif
> diff --git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
> index f1a6773..a886ba9 100644
> --- a/drivers/kvm/kvm.h
> +++ b/drivers/kvm/kvm.h
> @@ -334,15 +334,13 @@ struct kvm_vcpu {
>  	};
>  	struct mutex mutex;
>  	int   cpu;
> -	int   launched;
> +	char  vcpu_id;
>   

There is already a vcpu_id in kvm.git...

> +	char  launched;
>   


???

>  
> +unsigned long get_cr8(struct kvm_vcpu *vcpu)
> +{
> +	if (irqchip_in_kernel(vcpu->kvm))
> +		return kvm_lapic_get_cr8(vcpu);
> +	else
> +		return vcpu->cr8;
> +}
> +EXPORT_SYMBOL_GPL(get_cr8);
>   

How about keep vcpu->cr8 even with kernel lapic?  then we don't need this.

> +
> +u64 kvm_get_apic_base(struct kvm_vcpu *vcpu)
> +{
> +	if (irqchip_in_kernel(vcpu->kvm))
> +		return vcpu->apic->base_msr;
> +	else
> +		return vcpu->apic_base;
> +}
> +EXPORT_SYMBOL_GPL(kvm_get_apic_base);
>   

ditto.

> +
> +#define VEC_POS(v) ((v) & (32 - 1))
> +#define REG_POS(v) (((v) >> 5) << 4)
> +#define apic_test_and_set_vector(vec, bitmap)                 \
> +    test_and_set_bit(VEC_POS(vec), (bitmap) + REG_POS(vec))
> +#define apic_test_and_clear_vector(vec, bitmap)               \
> +    test_and_clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec))
> +#define apic_set_vector(vec, bitmap)                          \
> +    set_bit(VEC_POS(vec), (bitmap) + REG_POS(vec))
> +#define apic_clear_vector(vec, bitmap)                        \
> +    clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec))
> +
> +#define apic_hw_enabled(apic)	((apic)->base_msr &
> MSR_IA32_APICBASE_ENABLE)
> +#define apic_sw_enabled(apic)	(!((apic)->status & APIC_SW_DISABLE))
> +#define apic_enabled(apic)	(apic_sw_enabled(apic) &&	\
> +	apic_hw_enabled(apic))
>   

These would be better as inline functions (type checking, etc.)

> +
> +#define KVM_APIC_ID(apic)	\
> +	(GET_APIC_ID(apic_get_reg(apic, APIC_ID)))
> +
> +#define apic_lvt_enabled(apic, lvt_type)	\
> +	(!(apic_get_reg(apic, lvt_type) & APIC_LVT_MASKED))
> +
> +#define apic_lvt_vector(apic, lvt_type)		\
> +	(apic_get_reg(apic, lvt_type) & APIC_VECTOR_MASK)
> +
> +#define apic_lvt_dm(apic, lvt_type)	\
> +	(apic_get_reg(apic, lvt_type) & APIC_MODE_MASK)
> +
> +#define apic_lvtt_period(apic)	\
> +	(apic_get_reg(apic, APIC_LVTT) & APIC_LVT_TIMER_PERIODIC)
>   

more functions


Where are the state save/restore?  they can be added later, so long as
the capability is enabled only after everything is working.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


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