Re: In kernel PIC support: kernel patch

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.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/