From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: In kernel PIC support: kernel patch Date: Mon, 02 Jul 2007 16:53:10 -0700 Message-ID: <46898FE6.7040709@qumranet.com> References: <10EA09EFD8728347A513008B6B0DA77A01AE9255@pdsmsx411.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org To: "Dong, Eddie" Return-path: In-Reply-To: <10EA09EFD8728347A513008B6B0DA77A01AE9255-wq7ZOvIWXbNpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.org 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/