Re: [PATCH] Support for in-kernel mmio handlers

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

This has significant changes, so merits a review.


Gregory Haskins wrote:

> diff --git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
> index fceeb84..c1923df 100644
> --- a/drivers/kvm/kvm.h
> +++ b/drivers/kvm/kvm.h
> @@ -236,6 +236,56 @@ struct kvm_pio_request {
>  	int rep;
>  };
>  
> +struct kvm_io_device {
> +	unsigned long (*read)(struct kvm_io_device *this,
> +			      gpa_t addr,
> +			      int length);
> +	void (*write)(struct kvm_io_device *this,
> +		      gpa_t addr,
> +		      int length,
> +		      unsigned long val);
> +	int (*in_range)(struct kvm_io_device *this, gpa_t addr);
> +
> +	void             *private;
> +};
> +
> +/* It would be nice to use something smarter than a linear search, TBD...
> + * Thankfully we dont expect many devices to register (famous last words :),
> + * so until then it will suffice.  At least its abstracted so we can change
> + * in one place.
> + */
>   

/*
 * comment
 */

> +struct kvm_io_bus {
> +	int                   dev_count;
> +#define NR_IOBUS_DEVS 6
> +	struct kvm_io_device *devs[NR_IOBUS_DEVS];
> +};
> +
> +static inline void kvm_io_bus_init(struct kvm_io_bus *bus)
> +{
> +	memset(bus, 0, sizeof(*bus));
> +}
> +
> +static inline struct kvm_io_device *kvm_io_bus_find_dev(struct kvm_io_bus *bus, gpa_t addr)
> +{
> +	int i;
> +
> +	for(i=0; i<bus->dev_count; i++) {
>   

spaces, after for and around operators

> +		struct kvm_io_device *pos = bus->devs[i];
> +
> +		if (pos->in_range(pos, addr))
> +			return pos;
> +	}
> +
> +	return NULL;
> +}
>   

this is too long for an inline function.  the others can also be made 
out-of-line.

> +
> +static inline void kvm_io_bus_register_dev(struct kvm_io_bus *bus, struct kvm_io_device *dev)
> +{
> +	BUG_ON(bus->dev_count >= (NR_IOBUS_DEVS-1));
> +
> +	bus->devs[bus->dev_count++] = dev;
> +}
> +
>   

I actually meant an array of objects, not pointers, but in the interest 
of reducing the amount of churn we'll change it if and when we see a 
problem there.

>  
> +static struct kvm_io_device* vcpu_find_mmio_dev(struct kvm_vcpu *vcpu, 
> +						gpa_t addr)
>   

"struct kvm_io_device *..."


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV