Dong, Eddie wrote: > Here is the updated patch with all comments fixed. > > > Meanwhile I enabled SVM support by copying code from Xen and managed to > find an AMD box to have a shoot, it works! > Please also find the svm patch. > > > > > drivers/kvm/kvm_irq.c | 59 ++++++ > drivers/kvm/kvm_irq.h | 64 ++++++ >
These can be named as irq.c and irq.h. No need for the kvm prefix. > +void kvm_pic_set_irq_new(void *opaque, int irq, int level) > Remove _new from the name. It's just an artifact of qemu's development history; we don't need it. > + > +int kvm_pic_read_irq(struct kvm_pic *s) > +{ > + int irq, irq2, intno; > + > + irq = pic_get_irq(&s->pics[0]); > + if (irq >= 0) { > + pic_intack(&s->pics[0], irq); > + if (irq == 2) { > + irq2 = pic_get_irq(&s->pics[1]); > + if (irq2 >= 0) { > + pic_intack(&s->pics[1], irq2); > + } else { > + /* > + * spurious IRQ on slave controller > + */ > + irq2 = 7; > + } > A few braces can be removed here. > + > +static uint32_t pic_poll_read(struct kvm_pic_state *s, uint32_t addr1) > u32 > +static uint32_t pic_ioport_read(void *opaque, uint32_t addr1) > here too, and a few other places. > + > +static void picdev_write(struct kvm_io_device *this, > + gpa_t addr, int len, const void *val) > +{ > + struct kvm_pic *s = this->private; > + unsigned char data = *(unsigned char *)val; > + > + if (len != 1) { > + printk(KERN_ERR "PIC: non byte write\n"); > + return; > + } > This can be used to spam the host kernel log, if the guest issues lots of nonbyte accesses. You can use printk_ratelimit() or something similar. This goes for the read function too. > > +struct kvm_pic; > struct kvm { > spinlock_t lock; /* protects everything except vcpus */ > int naliases; > @@ -456,8 +457,19 @@ struct kvm { > struct file *filp; > struct kvm_io_bus mmio_bus; > struct kvm_io_bus pio_bus; > + struct kvm_pic *vpic; > }; > Why not include it inline and avoid the pointer dereference? > > +static inline struct kvm_pic *pic_irqchip(struct kvm *kvm) > +{ > + return kvm->vpic; > +} > + > +static inline int irqchip_in_kernel(struct kvm *kvm) > +{ > + return pic_irqchip(kvm) != 0; > +} > That's why... > } > + case KVM_CREATE_PIC: > + r = -ENOMEM; > + kvm->vpic = kvm_create_pic(kvm); > + if (kvm->vpic) { > + r = 0; > + } > braces. also, the usual logic is to 'goto out' if an error occured. > }; > > +/* for KVM_SET_IRQ_LEVEL */ > +struct kvm_irq_level { > + __u32 irq; > + __u32 level; > +}; > Please add an irqchip index, so this can be used for configurations with multiple irqchips (likely ioapics). You'll also need to add padding so the structure is 64-bit aligned. Other than these minor issues, the patch looks fine. ------------------------------------------------------------------------- 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