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

[Scott Wood <scottwood@freescale.com>]

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