From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Graf Subject: Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall() Date: Fri, 4 Oct 2013 13:16:01 +0200 Message-ID: <594B19BD-31F8-469F-8C3A-A0C08FADBB5A@suse.de> References: <1379913839-11347-1-git-send-email-Bharat.Bhushan@freescale.com> <1379913839-11347-2-git-send-email-Bharat.Bhushan@freescale.com> <90F2E123-022D-4F4E-A350-44CA1051B484@suse.de> <1380732029.12932.3.camel@snotra.buserror.net> <684A050C-5885-4678-8C6A-7E57001DD341@suse.de> <1380733492.12932.17.camel@snotra.buserror.net> <24087AF3-C8BB-4D5C-8A61-9E2AAA13511B@suse.de> <1380735772.12932.24.camel@snotra.buserror.net> <3E8A25A6-B87F-4D5E-B0B8-58CA848BB2A5@suse.de> <1380736197.12932.31.camel@snotra.buserror.net> <82DD4404-F93F-4310-8E20-DF9265D59C8F@suse.de> <1380738834.12932.44.camel@snotra.buserror.net> <6A3DF150A5B70D4F9B66A25E3F7C888D0718F9FC@039-SN2MPN1-011.039d.mgd.msft.net> Mime-Version: 1.0 (Apple Message framework v1278) Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 8BIT Cc: Wood Scott-B07421 , "kvm-ppc@vger.kernel.org" , "kvm@vger.kernel.org" To: Bhushan Bharat-R65777 Return-path: In-Reply-To: <6A3DF150A5B70D4F9B66A25E3F7C888D0718F9FC@039-SN2MPN1-011.039d.mgd.msft.net> Sender: kvm-ppc-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On 04.10.2013, at 06:26, Bhushan Bharat-R65777 wrote: > > >> -----Original Message----- >> From: Wood Scott-B07421 >> Sent: Thursday, October 03, 2013 12:04 AM >> To: Alexander Graf >> Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Bhushan >> Bharat-R65777 >> Subject: Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to >> epapr_hypercall() >> >> 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. >> > > I am not sure where we are agreeing on? > Do we want to remove the stub in arch/powerpc/include/asm/kvm_para.h ? as there is no caller without KVM_GUEST and in future caller ensure this to be called only from code selected by KVM_GUEST? > > Or let this stub stay to avoid any random driver calling this ? I think the most reasonable way forward is to add a stub for non-CONFIG_EPAPR to the epapr code, then replace the kvm bits with generic epapr bits (which your patches already do). With that we should be 100% equivalent to today's code, just with a lot less lines of code :). Alex From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Graf Date: Fri, 04 Oct 2013 11:16:01 +0000 Subject: Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall() Message-Id: <594B19BD-31F8-469F-8C3A-A0C08FADBB5A@suse.de> List-Id: References: <1379913839-11347-1-git-send-email-Bharat.Bhushan@freescale.com> <1379913839-11347-2-git-send-email-Bharat.Bhushan@freescale.com> <90F2E123-022D-4F4E-A350-44CA1051B484@suse.de> <1380732029.12932.3.camel@snotra.buserror.net> <684A050C-5885-4678-8C6A-7E57001DD341@suse.de> <1380733492.12932.17.camel@snotra.buserror.net> <24087AF3-C8BB-4D5C-8A61-9E2AAA13511B@suse.de> <1380735772.12932.24.camel@snotra.buserror.net> <3E8A25A6-B87F-4D5E-B0B8-58CA848BB2A5@suse.de> <1380736197.12932.31.camel@snotra.buserror.net> <82DD4404-F93F-4310-8E20-DF9265D59C8F@suse.de> <1380738834.12932.44.camel@snotra.buserror.net> <6A3DF150A5B70D4F9B66A25E3F7C888D0718F9FC@039-SN2MPN1-011.039d.mgd.msft.net> In-Reply-To: <6A3DF150A5B70D4F9B66A25E3F7C888D0718F9FC@039-SN2MPN1-011.039d.mgd.msft.net> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Bhushan Bharat-R65777 Cc: Wood Scott-B07421 , "kvm-ppc@vger.kernel.org" , "kvm@vger.kernel.org" On 04.10.2013, at 06:26, Bhushan Bharat-R65777 wrote: > > >> -----Original Message----- >> From: Wood Scott-B07421 >> Sent: Thursday, October 03, 2013 12:04 AM >> To: Alexander Graf >> Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Bhushan >> Bharat-R65777 >> Subject: Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to >> epapr_hypercall() >> >> 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. >> > > I am not sure where we are agreeing on? > Do we want to remove the stub in arch/powerpc/include/asm/kvm_para.h ? as there is no caller without KVM_GUEST and in future caller ensure this to be called only from code selected by KVM_GUEST? > > Or let this stub stay to avoid any random driver calling this ? I think the most reasonable way forward is to add a stub for non-CONFIG_EPAPR to the epapr code, then replace the kvm bits with generic epapr bits (which your patches already do). With that we should be 100% equivalent to today's code, just with a lot less lines of code :). Alex