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

Avi Kivity
2007-04-08T08:49:24Z

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.