On 02/18/2013 11:50:44 PM, Christoffer Dall wrote:
On Mon, Feb 18, 2013 at 4:53 PM, Scott Wood <scottwood@xxxxxxxxxxxxx>
wrote:
> On 02/18/2013 06:44:20 PM, Christoffer Dall wrote:
>>
>> On Wed, Feb 13, 2013 at 9:49 PM, Scott Wood
<scottwood@xxxxxxxxxxxxx>
>> > +#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?
Yes, same as in ONE_REG.
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.
I don't follow.
> 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.
There's an ifdef, as you can see from the patch that adds MPIC
support. But it's the same ifdef that gets used to determine whether
the device code gets built in. Nothing special needs to be added; no
per-architecture hoop to jump through.
Note that we would still need per-device ifdefs in the arch code,
because not all PPC KVM builds are going to have MPIC support.
What if, instead of a switch statement and ifdefs, it operated on a
registration basis?
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.
I don't think it's that far fetched. APIC is shared between x86 and
ia64 -- even if APIC has no need for anything beyond existing API, it
shows that it's not a crazy possibility. Freescale already has some
devices that are shared between PPC and ARM, and that set will expand
(probably not to irq controllers, though the probability is non-zero)
with Layerscape, about which Freeescale says, "The unique,
core-agnostic architecture incorporates the optimum core for the given
application—ARM cores or Power Architecture cores." We already need to
go back and non-ppc-ize various drivers, including their reliance on
I/O accessors that are defined in architecture-specific ways... Making
things gratuitiously architecture specific is just a bad idea, even if
the "use case" for it actually being used on multiple architectures is
remote.
Normal kernel drivers tend to go in drivers/, not arch/, even if
they're only used on one architecture...
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.
We at least need the numberspace to not be architecture-specific if we
want to retain the possibility of changing later -- not to mention what
happens if architectures merge. I see that "arm" and "arm64" are
separate, despite the fact that other architectures that used to be
split this way have since merged. Maybe "arm64" is too different from
"arm" for that to happen, but who knows...
...and if they don't merge, wouldn't that be a likely case for devices
shared across architectures? Does arm64 use gic/vgic? This post
suggests that there is at least something in common (the bit about
"once the GIC code is shared between
arm and arm64"):
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/135836.html
-Scott
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
