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

Scott Wood
2013-01-09T00:13:09Z

On 01/08/2013 05:49:40 PM, Christoffer Dall wrote:
> On Tue, Jan 8, 2013 at 6:29 PM, Scott Wood <scottwood@freescale.com>
> wrote:
> > On 01/08/2013 05:17:05 PM, Christoffer Dall wrote:
> >> 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, ...

Such an IOCTL would add nothing other than trading the limited and
cumbersome ioctl namespace for something structured a bit differently
(which isn't such a bad thing...). A set device attribute ioctl would
constrain it to "take this number and convey it to the enumerated
device for the enumerated configuration purpose". There's already room
for device-specific semantics since you can have multiple address types.

Regarding the dev/param split, it looks like you're doing the split
anyway -- might as well make them separate struct fields rather than an
architecture-specific bitfield encoding.

> > 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.

Well, at least PPC has this explicit need as well. :-)

Only the toplevel mechanism would be generic; it would be up to each
device to decide which (if any) configuration parameters it wants to
expose through it.

-Scott