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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
