On 02/18/2013 06:44:20 PM, Christoffer Dall wrote:
On Wed, Feb 13, 2013 at 9:49 PM, Scott Wood <scottwood@xxxxxxxxxxxxx>
wrote:
> 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.
> + */
This comment is on the heavy side (if at all needed). If you want to
remind people of idr, just put that in a single line. A define is a
define is a define.
OK.
> +#define KVM_CREATE_DEVICE _IOWR(KVMIO, 0xac, struct
kvm_create_device)
> +#define KVM_SET_DEVICE_ATTR _IOW(KVMIO, 0xad, struct
kvm_device_attr)
> +#define KVM_GET_DEVICE_ATTR _IOW(KVMIO, 0xae, struct
kvm_device_attr)
_IOWR ?
struct kvm_device_attr itself is write-only, though the data pointed to
by the addr field goes the other way for GET. ONE_REG is in the same
situation and also uses _IOW for both.
> +static int kvm_ioctl_create_device(struct kvm *kvm,
> + struct kvm_create_device *cd)
> +{
> + struct kvm_device *dev = NULL;
> + bool test = cd->flags & KVM_CREATE_DEVICE_TEST;
> + int id;
> + int r;
> +
> + mutex_lock(&kvm->lock);
> +
> + id = kvm->num_devices;
> + if (id >= KVM_MAX_DEVICES && !test) {
> + r = -ENOSPC;
> + goto out;
> + }
> +
> + switch (cd->type) {
> + default:
> + r = -ENODEV;
> + goto out;
> + }
do we really believe that there will be any arch-generic recognition
of types? shouldn't this be a call to an arch-specific function
instead. Which makes me wonder whether the device type IDs should be
arch specific as well...
I prefer to look at it from the other direction -- is there any reason
why this *should* be architecture specific? What will that make easier?
By doing device recognition here we don't need a separate copy of this
per arch (including some #ifdef or modifying every arch at once --
including ARM which I can't modify yet because it's not merged), and
*if* we should end up with an in-kernel-emulated device that gets used
across multiple architectures, it would be annoying to have to modify
all relevant architectures (and worse to deal with per-arch
numberspaces).
-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
