Re: In kernel PIC support: kernel patch

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

Dong, Eddie wrote:

First, sorry for the delayed review.

> +#include "virq.h"
> +#include <linux/mm.h>
>   

Includes from <linux/> before includes from local directory please.

> +
> +/* set irq level. If an edge is detected, then the IRR is set to 1 */
>   

Kernel comments like this:

/*
 * blah blah blah
 */

This comment applies to the rest of the patch.

> diff --git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
> index a7c5e6b..1e2d3ba 100644
> --- a/drivers/kvm/kvm.h
> +++ b/drivers/kvm/kvm.h
> @@ -456,8 +456,15 @@ struct kvm {
>  	struct file *filp;
>  	struct kvm_io_bus mmio_bus;
>  	struct kvm_io_bus pio_bus;
> +	void *vpic;
>   

Why void?

>  };
>  
> +#define kernel_pic(kvm) ((kvm)->vpic)
>   

Please, no function-like macros.  In this case, just put kvm->vpic
everywhere.

>  	spin_lock(&kvm_lock);
>  	list_del(&kvm->vm_list);
>  	spin_unlock(&kvm_lock);
> +	if (kernel_pic(kvm))
> +		kfree(kernel_pic(kvm));
>   

kfree() knows about null pointers, no need for the check.

> +	case KVM_CREATE_PIC:
> +		r = -ENOMEM;
> +		kernel_pic(kvm) = create_pic(kvm);
>   

Ow!

> +		if (kernel_pic(kvm)) {
> +			r = 0;
> +		}
>   

No {} for single-line if statements in the kernel.

> +		break;
> +	case KVM_SET_ISA_IRQ_LEVEL: {
> +		struct kvm_irq_level irq_event;
>   

blank line here.

> +#include "kvm.h"
> +#include "virq.h"
> +
> +/*
> + * check if there is pending interrupt without
> + * intack.
> + */
> +int cpu_has_interrupt(struct kvm_vcpu *v)
> +{
> +	struct pic_state2 *s = kernel_pic(v->kvm);
> +
>   

Isn't the PIC connected to just one cpu?

> +	/* PIC */
> +	if (int_output(s))
> +		return 1;
> +	/* TODO: APIC */
> +	return 0;
> +}
> +
> +EXPORT_SYMBOL_GPL(cpu_has_interrupt);
>   

exported symbols (or even non-static symbols) should begin with kvm_.

> +
> +/*
> + * Read pending interrupt vector and intack.
> + */
> +int cpu_get_interrupt(struct kvm_vcpu *v)
> +{
> +	struct pic_state2 *s = kernel_pic(v->kvm);
> +	int vector;
> +
> +	int_output(s) = 0;
> +	vector = pic_read_irq(s);
> +	if (vector != -1)
> +		return vector;
> +	/* TODO: APIC */
> +	return -1;
> +}
> +
> +EXPORT_SYMBOL_GPL(cpu_get_interrupt);
>   

kvm_.

> +
> +typedef void SetIRQFunc(void *opaque, int irq_num, int level);
> +typedef void IRQRequestFunc(void *opaque, int level);
>   

Lower case names.

> +
> +struct PicState2;
> +struct pic_state {
> +	uint8_t last_irr;	/* edge detection */
> +	uint8_t irr;		/* interrupt request register */
> +	uint8_t imr;		/* interrupt mask register */
> +	uint8_t isr;		/* interrupt service register */
> +	uint8_t priority_add;	/* highest irq priority */
> +	uint8_t irq_base;
> +	uint8_t read_reg_select;
> +	uint8_t poll;
> +	uint8_t special_mask;
> +	uint8_t init_state;
> +	uint8_t auto_eoi;
> +	uint8_t rotate_on_auto_eoi;
> +	uint8_t special_fully_nested_mode;
> +	uint8_t init4;		/* true if 4 byte init */
> +	uint8_t elcr;		/* PIIX edge/trigger selection */
> +	uint8_t elcr_mask;
> +	struct pic_state2 *pics_state;
> +};
>   

Use u8, not uint8_t in kernel code.

> +
> +struct pic_state2 {
> +	/* 0 is master pic, 1 is slave pic */
> +	struct pic_state pics[2];
> +	IRQRequestFunc *irq_request;
> +	void *irq_request_opaque;
> +	/* IOAPIC callback support */
> +	SetIRQFunc *alt_irq_func;
> +	void *alt_irq_opaque;
> +	int output;		/* intr from master PIC */
> +	struct kvm_io_device dev;
> +};
>   

Prefix structure name with kvm_.

> +
> +#define int_output(s)		(((struct pic_state2 *)s)->output)
>   

This is a dangerous macro!  It takes anything and treats it as a pic_state2.

Please remove.


> +struct pic_state2 *create_pic(struct kvm *kvm);
> +void pic_set_irq_new(void *opaque, int irq, int level);
> +int pic_read_irq(struct pic_state2 *s);
> +int cpu_get_interrupt(struct kvm_vcpu *v);
> +int cpu_has_interrupt(struct kvm_vcpu *v);
>   

Prefix globally visible names with kvm_.

>  
> +void enable_irq_window(struct kvm_vcpu *vcpu)
> +{
> +	u32 cpu_based_vm_exec_control;
> +
> +	cpu_based_vm_exec_control =
> vmcs_read32(CPU_BASED_VM_EXEC_CONTROL);
> +	cpu_based_vm_exec_control |= CPU_BASED_VIRTUAL_INTR_PENDING;
> +	vmcs_write32(CPU_BASED_VM_EXEC_CONTROL,
> cpu_based_vm_exec_control);
> +}
>   

static.
> +
> +void do_intr_assist(struct kvm_vcpu *vcpu)
> +{
> +	u32 idtv_info_field, intr_info_field;
> +	int has_ext_irq, interrupt_window_open;
> +	/* TODO: Move IDT_Vectoring here */
> +
> +	has_ext_irq = cpu_has_interrupt(vcpu);
> +	intr_info_field = vmcs_read32(VM_ENTRY_INTR_INFO_FIELD);
> +	idtv_info_field = vmcs_read32(IDT_VECTORING_INFO_FIELD);
> +	if (intr_info_field & INTR_INFO_VALID_MASK) {
> +		if (idtv_info_field & INTR_INFO_VALID_MASK) {
> +			/* TODO: fault when IDT_Vectoring */
> +			printk(KERN_ERR "Fault when IDT_Vectoring\n");
> +		}
> +		if (has_ext_irq)
> +			enable_irq_window(vcpu);
> +		return;
> +	}
> +	if (unlikely(idtv_info_field & INTR_INFO_VALID_MASK)) {
> +		vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, idtv_info_field);
> +		vmcs_write32(VM_ENTRY_INSTRUCTION_LEN,
> +				vmcs_read32(VM_EXIT_INSTRUCTION_LEN));
> +
> +		if ( unlikely(idtv_info_field & 0x800) ) /* valid error
> code */
> +			vmcs_write32(VM_ENTRY_EXCEPTION_ERROR_CODE,
> +				vmcs_read32(IDT_VECTORING_ERROR_CODE));
> +		if ( unlikely(has_ext_irq) )
> +			enable_irq_window(vcpu);
> +		return;
> +	}
>   

This isn't Xen.  No spaces inside if () condition.

>  
> +/* for KVM_SET_IRQ_LEVEL */
> +struct kvm_irq_level {
> +	__u32 irq;
> +	__u32 level;
> +};
>   

As discussed, we might want to make this a list, and add a chip ID so
this can be used for the ioapic too.

> +
>  enum kvm_exit_reason {
>  	KVM_EXIT_UNKNOWN          = 0,
>  	KVM_EXIT_EXCEPTION        = 1,
> @@ -282,6 +288,9 @@ struct kvm_signal_mask {
>  #define KVM_CREATE_VCPU           _IO(KVMIO,  0x41)
>  #define KVM_GET_DIRTY_LOG         _IOW(KVMIO, 0x42, struct
> kvm_dirty_log)
>  #define KVM_SET_MEMORY_ALIAS      _IOW(KVMIO, 0x43, struct
> kvm_memory_alias)
> +/* Device model IOC */
> +#define KVM_CREATE_PIC		  _IO(KVMIO,  0x60)
> +#define KVM_SET_ISA_IRQ_LEVEL	  _IO(KVMIO,  0x61)
>  
>   

Need to add a way to detect if this functionality is available, via
KVM_CHECK_EXTENSION.

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