On Wed, 2013-10-02 at 19:54 +0200, Alexander Graf wrote:
> On 02.10.2013, at 19:49, Scott Wood wrote:
>
> > On Wed, 2013-10-02 at 19:46 +0200, Alexander Graf wrote:
> >> On 02.10.2013, at 19:42, Scott Wood wrote:
> >>
> >>> On Wed, 2013-10-02 at 19:17 +0200, Alexander Graf wrote:
> >>>> On 02.10.2013, at 19:04, Scott Wood wrote:
> >>>>
> >>>>> On Wed, 2013-10-02 at 18:53 +0200, Alexander Graf wrote:
> >>>>>> On 02.10.2013, at 18:40, Scott Wood wrote:
> >>>>>>
> >>>>>>> On Wed, 2013-10-02 at 16:19 +0200, Alexander Graf wrote:
> >>>>>>>> Won't this break when CONFIG_EPAPR_PARAVIRT=n? We wouldn't have epapr_hcalls.S compiled into the code base then and the bl above would reference an unknown function.
> >>>>>>>
> >>>>>>> KVM_GUEST selects EPAPR_PARAVIRT.
> >>>>>>
> >>>>>> But you can not select KVM_GUEST and still call these inline functions, no?
> >>>>>
> >>>>> No.
> >>>>>
> >>>>>> Like kvm_arch_para_features().
> >>>>>
> >>>>> Where does that get called without KVM_GUEST?
> >>>>>
> >>>>> How would that work currently, with the call to kvm_hypercall() in
> >>>>> arch/powerpc/kernel/kvm.c (which calls epapr_hypercall, BTW)?
> >>>>
> >>>> It wouldn't ever get called because kvm_hypercall() ends up always returning EV_UNIMPLEMENTED when #ifndef CONFIG_KVM_GUEST.
> >>>
> >>> OK, so the objection is to removing that stub? Where would we actually
> >>> want to call this without knowing that KVM_GUEST or EPAPR_PARAVIRT are
> >>> enabled?
> >>
> >> In probing code. I usually prefer
> >>
> >> if (kvm_feature_available(X)) {
> >> ...
> >> }
> >>
> >> over
> >>
> >> #ifdef CONFIG_KVM_GUEST
> >> if (kvm_feature_available(X)) {
> >> ...
> >> }
> >> #endif
> >>
> >> at least when I can avoid it. With the current code the compiler would be smart enough to just optimize out the complete branch.
> >
> > Sure. My point is, where would you be calling that where the entire
> > file isn't predicated on (or selecting) CONFIG_KVM_GUEST or similar?
> >
> > We don't do these stubs for every single function in the kernel -- only
> > ones where the above is a reasonable use case.
>
> Yeah, I'm fine on dropping it, but we need to make that a conscious decision and verify that no caller relies on it.
kvm_para_has_feature() is called from arch/powerpc/kernel/kvm.c,
arch/x86/kernel/kvm.c, and arch/x86/kernel/kvmclock.c, all of which are
enabled by CONFIG_KVM_GUEST.
I did find one example of kvm_para_available() being used in an
unexpected place -- sound/pci/intel8x0.c. It defines its own
non-CONFIG_KVM_GUEST stub, even though x86 defines kvm_para_available()
using inline CPUID stuff which should work without CONFIG_KVM_GUEST.
I'm not sure why it even needs to do that, though -- shouldn't the
subsequent PCI subsystem vendor/device check should be sufficient? No
hypercalls are involved.
That said, the possibility that some random driver might want to make
use of paravirt features is a decent argument for keeping the stub.
-Scott
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
