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
