On Mon, Feb 18, 2013 at 4:53 PM, Scott Wood <scottw...@freescale.com> wrote:
> On 02/18/2013 06:44:20 PM, Christoffer Dall wrote:
>>
>> On Wed, Feb 13, 2013 at 9:49 PM, Scott Wood <scottw...@freescale.com>
>> 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.
>
>
ok.
Btw., what about the size of the attr? implicitly defined through the attr id?
>> > +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?
>
The fact that you don't have to create static inlines for the
architectures that don't define the functions that get called or have
to similar #ifdef tricks, and I also think it's easier to read the
arch-specific bits of the code that way, instead of some arbitrary
function that you have to trace through to figure out where it's
called from.
> 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).
I would say that's exactly what you're going to need with your approach:
switch (cd->type) {
case KVM_ARM_VGIC_V2_0:
kvm_arm_vgic_v2_0_create(...);
}
are you going to ifdef here in this function, or? I think it's cleaner
to have the single arch-specific hooks and handle the cases there.
The use case of having a single device which is so central to the
system that we emulate it inside the kernel and is shared across
multiple archs is pretty far fetched to me.
However, this is internal and can always be changed, so if everyone
agrees on the overall API, whichever way you implement it is fine with
me.
>
> -Scott
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
