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

Reply via email to