On 03/05/2013 06:59:26 PM, Paul Mackerras wrote:
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.
Ah, forgot that ONE_REG did that. I think I'd rather just make size a
separate field in the struct.
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...
The device implementation creates the kvm_device struct, and can embed
it in some other struct to provide instance-specific context, using
container_of() to extract it. MPIC does this in patch 6/6.
MPIC currently forbids multiple instances because of semantic issues
(if there are multiple MPICs, which one gets the connection to the
vcpus, and where do the outputs of the others go?), not anything to do
with the device control API.
> 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,
Its address would be different, which can be used with container_of()
as described above.
and in fact it would make sense to use the one copy of this struct
for all instances of a
given type.
We could do that, but then we'd need to wrap it with a struct that
points to the class data and the instance data (the latter either as a
pointer or through container_of). Given that both the number of
instances and the number of struct members are small, I didn't think it
was worth separating it out.
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.
container_of is fairly idiomatic in Linux. The existing kvm_io_device
works this way as well. What is the advantage of using an explicit
pointer?
-Scott
--
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
