From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Subject: Re: [RFC PATCH 1/6] kvm: add device control API Date: Tue, 19 Feb 2013 14:16:06 -0600 Message-ID: <1361304966.29654.8@snotra> References: Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; delsp=Yes format=Flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Alexander Graf , , To: Christoffer Dall Return-path: In-Reply-To: (from cdall@cs.columbia.edu on Mon Feb 18 23:50:44 2013) Content-Disposition: inline Sender: kvm-ppc-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On 02/18/2013 11:50:44 PM, Christoffer Dall wrote: > On Mon, Feb 18, 2013 at 4:53 PM, Scott Wood = =20 > wrote: > > On 02/18/2013 06:44:20 PM, Christoffer Dall wrote: > >> > >> On Wed, Feb 13, 2013 at 9:49 PM, Scott Wood =20 > > >> > +#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 =20 > pointed to by > > the addr field goes the other way for GET. ONE_REG is in the same = =20 > situation > > and also uses _IOW for both. > > > > >=20 > ok. >=20 > Btw., what about the size of the attr? implicitly defined through the= =20 > 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 =20 > this per > > arch (including some #ifdef or modifying every arch at once -- =20 > including ARM > > which I can't modify yet because it's not merged), and *if* we =20 > 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). >=20 > I would say that's exactly what you're going to need with your =20 > approach: >=20 > switch (cd->type) { > case KVM_ARM_VGIC_V2_0: > kvm_arm_vgic_v2_0_create(...); > } >=20 >=20 > are you going to ifdef here in this function, or? I think it's cleane= r > 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 =20 support. But it's the same ifdef that gets used to determine whether =20 the device code gets built in. Nothing special needs to be added; no =20 per-architecture hoop to jump through. Note that we would still need per-device ifdefs in the arch code, =20 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 =20 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 =20 ia64 -- even if APIC has no need for anything beyond existing API, it =20 shows that it's not a crazy possibility. Freescale already has some =20 devices that are shared between PPC and ARM, and that set will expand =20 (probably not to irq controllers, though the probability is non-zero) =20 with Layerscape, about which Freeescale says, "The unique, =20 core-agnostic architecture incorporates the optimum core for the given = =20 application=E2=80=94ARM cores or Power Architecture cores." We already= need to =20 go back and non-ppc-ize various drivers, including their reliance on =20 I/O accessors that are defined in architecture-specific ways... Making= =20 things gratuitiously architecture specific is just a bad idea, even if = =20 the "use case" for it actually being used on multiple architectures is = =20 remote. Normal kernel drivers tend to go in drivers/, not arch/, even if =20 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 wit= h > me. We at least need the numberspace to not be architecture-specific if we = =20 want to retain the possibility of changing later -- not to mention what= =20 happens if architectures merge. I see that "arm" and "arm64" are =20 separate, despite the fact that other architectures that used to be =20 split this way have since merged. Maybe "arm64" is too different from = =20 "arm" for that to happen, but who knows... =2E..and if they don't merge, wouldn't that be a likely case for device= s =20 shared across architectures? Does arm64 use gic/vgic? This post =20 suggests that there is at least something in common (the bit about =20 "once the GIC code is shared between arm and arm64"): http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/135= 836.html -Scott