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/
_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
