Re: [RFC PATCH 1/6] kvm: add device control API

[Paul Mackerras <paulus@samba.org>]

On Wed, Feb 13, 2013 at 11:49:15PM -0600, Scott Wood wrote:
> Currently, devices that are emulated inside KVM are configured in a
> hardcoded manner based on an assumption that any given architecture
> only has one way to do it.  If there's any need to access device state,
> it is done through inflexible one-purpose-only IOCTLs (e.g.
> KVM_GET/SET_LAPIC).  Defining new IOCTLs for every little thing is
> cumbersome and depletes a limited numberspace.
> 
> This API provides a mechanism to instantiate a device of a certain
> type, returning an ID that can be used to set/get attributes of the
> device.  Attributes may include configuration parameters (e.g.
> register base address), device state, operational commands, etc.  It
> is similar to the ONE_REG API, except that it acts on devices rather
> than vcpus.
> 
> Both device types and individual attributes can be tested without having
> to create the device or get/set the attribute, without the need for
> separately managing enumerated capabilities.

The API looks fine to me.  Ultimately I could use a version of the
get/set attribute ioctls that get or set multiple attributes within a
group, but that can come later.

Were you thinking that the attribute codes should encode the size of
the attribute, like the one_reg register IDs do?  If so it would be
good to define the bitfield and values for that in this patch.

The one comment I have on the implementation is that it doesn't seem
to conveniently allow for multiple instances of a device type, since
there is no instance-specific pointer stored anywhere.  More comments
below...

> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 0350e0d..dbaf012 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -335,6 +335,25 @@ struct kvm_memslots {
>  	short id_to_index[KVM_MEM_SLOTS_NUM];
>  };
>  
> +/*
> + * The worst case number of simultaneous devices will likely be very low
> + * (usually zero or one) for the forseeable future.  If the worst case
> + * exceeds this, then it can be increased, or we can convert to idr.
> + */
> +#define KVM_MAX_DEVICES 4
> +
> +struct kvm_device {
> +	u32 type;
> +
> +	int (*set_attr)(struct kvm *kvm, struct kvm_device *dev,
> +			struct kvm_device_attr *attr);
> +	int (*get_attr)(struct kvm *kvm, struct kvm_device *dev,
> +			struct kvm_device_attr *attr);
> +	int (*has_attr)(struct kvm *kvm, struct kvm_device *dev,
> +			struct kvm_device_attr *attr);
> +	void (*destroy)(struct kvm *kvm, struct kvm_device *dev);
> +};

This is more of a device class definition than a device instance
definition.  There is nothing in this struct that would be different
between different instances of a given device, and in fact it would
make sense to use the one copy of this struct for all instances of a
given type.  However, the functions listed here only take the struct
kvm_device pointer, meaning that to distinguish between instances,
these functions would have to do some sort of container_of trick to
know which instance to operate on.

I think it would make more sense either to add a void * instance data
pointer to struct kvm_device, or to add a void * argument to those
functions as an instance data pointer and add another field to struct
kvm like this:

> +
>  struct kvm {
>  	spinlock_t mmu_lock;
>  	struct mutex slots_lock;
> @@ -385,6 +404,8 @@ struct kvm {
>  	long mmu_notifier_count;
>  #endif
>  	long tlbs_dirty;
> +	struct kvm_device *devices[KVM_MAX_DEVICES];

+	void *device_instance[KVM_MAX_DEVICES];

Paul.