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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux