Re: [RFC PATCH 1/6] kvm: add device control API

[Scott Wood <scottwood@freescale.com>]

On 02/18/2013 11:50:44 PM, Christoffer Dall wrote:
> On Mon, Feb 18, 2013 at 4:53 PM, Scott Wood <scottwood@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  
> <scottwood@freescale.com>
> >> > +#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