[PATCH v5 01/12] KVM: ARM: Introduce KVM_SET_DEVICE_ADDRESS ioctl

[Christoffer Dall]

On Tue, Jan 8, 2013 at 6:29 PM, Scott Wood <scottwood at freescale.com> wrote:
> On 01/08/2013 05:17:05 PM, Christoffer Dall wrote:
>>
>> On Tue, Jan 8, 2013 at 5:36 PM, Scott Wood <scottwood at freescale.com>
>> wrote:
>> > On 01/08/2013 12:41:30 PM, Christoffer Dall wrote:
>> >> +struct kvm_device_address {
>> >> +       __u64 id;
>> >> +       __u64 addr;
>> >> +};
>> >
>> >
>> > What about this is really specific to addresses?  Can't we set other
>> > device
>> > parameters this way?
>> >
>> > Sort of like a device equivalent of PPC's one-reg interface.
>> >
>> This has been discussed a number of times,
>
>
> Sorry, this patch was just pointed out to me today.  I googled the patch
> title but couldn't find this discussion.
>
>

I believe it was mainly discussed at the KVM Forum in person.

>> and one or the other there
>> is a need for userspace to tell KVM to present memory-mapped devices
>> at a given address. It was also considered to make this specific to
>> irqchip initialization, but irqchips are different and a lot of that
>> code is x86-specific, so that approach was discarded.
>>
>> This *could* look something like this:
>>
>> struct kvm_device_param {
>>     u64 dev_id;
>>     u64 param_id;
>>     u64 value;
>> };
>>
>> but that has less clear, or at least less specific, semantics.
>
>
> Why is it less clear?  You need to have device-specific documentation for
> what "id" means, so why not also an enumeration of "param"s?  Or just keep
> it as is, and rename "address" to "value".  Whether "dev" and "param" are
> combined is orthogonal from whether it's used for non-address things.

less clear in the sense that you have to look at more code to see what
it does. I'm not saying that it's too unclear, at all, I'm actually
fine with it, but to make my point, we can make an ioctl that's called
do_something() that takes a struct with val0, val1, val2, val3, ...

>
> If you leave it as "address", either we'll have it being used for
> non-address things regardless of the name ("Not a typewriter!"), or there'll
> end up being yet more unnecessary ioctls, or device-specific things will end
> up getting shoved into CPU interfaces such as one-reg.  For example, on MPIC
> we need to be able to specify the version of the chip to emulate in addition
> to the address at which it lives.
>
> Also, why is it documented as an "arm" interface?  Shouldn't it be a generic
> interface, with other architectures currently not implementing any IDs?
> What in the kvm_arch_vm_ioctl() wrapper is arm-specific?
>
As I remember the argument for keeping this the point was that there
were other preferred methods for other archs to do this, and that ARM
was the only platform that had this explicit need, but maybe I'm
making this up.

I'll let Peter and Alex respond this as well, and if they're fine with
changing it to what I proposed above, then let's do that, and we can
make it a non arm-specific interface.

-Christoffer