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: Mon, 7 Oct 2013 17:20:26 +0200 Message-ID: <8D11C792-C036-4825-9D1D-1F3FED79334C@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> <594B19BD-31F8-469F-8C3A-A0C08FADBB5A@suse.de> <6A3DF150A5B70D4F9B66A25E3F7C888D071944A6@039-SN2MPN1-011.03 9d.mgd.msft.net> Mime-Version: 1.0 (Mac OS X Mail 7.0 \(1812\)) 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: Received: from cantor2.suse.de ([195.135.220.15]:50764 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752100Ab3JGPUc convert rfc822-to-8bit (ORCPT ); Mon, 7 Oct 2013 11:20:32 -0400 In-Reply-To: <6A3DF150A5B70D4F9B66A25E3F7C888D071944A6@039-SN2MPN1-011.039d.mgd.msft.net> Sender: kvm-owner@vger.kernel.org List-ID: On 07.10.2013, at 17:15, Bhushan Bharat-R65777 wrote: > > >> -----Original Message----- >> From: Alexander Graf [mailto:agraf@suse.de] >> Sent: Friday, October 04, 2013 4:46 PM >> To: Bhushan Bharat-R65777 >> Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org >> Subject: Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to >> epapr_hypercall() >> >> >> 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). > > Please describe which stub you are talking about. kvm_hypercall is always available, regardless of the config option, which makes all its subfunctions always available as well. Alex --- #ifdef CONFIG_KVM_GUEST #include static inline int kvm_para_available(void) { struct device_node *hyper_node; hyper_node = of_find_node_by_path("/hypervisor"); if (!hyper_node) return 0; if (!of_device_is_compatible(hyper_node, "linux,kvm")) return 0; return 1; } extern unsigned long kvm_hypercall(unsigned long *in, unsigned long *out, unsigned long nr); #else static inline int kvm_para_available(void) { return 0; } static unsigned long kvm_hypercall(unsigned long *in, unsigned long *out, unsigned long nr) { return EV_UNIMPLEMENTED; } #endif From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Graf Date: Mon, 07 Oct 2013 15:20:26 +0000 Subject: Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to epapr_hypercall() Message-Id: <8D11C792-C036-4825-9D1D-1F3FED79334C@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> <594B19BD-31F8-469F-8C3A-A0C08FADBB5A@suse.de> <6A3DF150A5B70D4F9B66A25E3F7C888D071944A6@039-SN2MPN1-011.03 9d.mgd.msft.net> In-Reply-To: <6A3DF150A5B70D4F9B66A25E3F7C888D071944A6@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 07.10.2013, at 17:15, Bhushan Bharat-R65777 wrote: > > >> -----Original Message----- >> From: Alexander Graf [mailto:agraf@suse.de] >> Sent: Friday, October 04, 2013 4:46 PM >> To: Bhushan Bharat-R65777 >> Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org >> Subject: Re: [PATCH 1/2] kvm/powerpc: rename kvm_hypercall() to >> epapr_hypercall() >> >> >> 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). > > Please describe which stub you are talking about. kvm_hypercall is always available, regardless of the config option, which makes all its subfunctions always available as well. Alex --- #ifdef CONFIG_KVM_GUEST #include static inline int kvm_para_available(void) { struct device_node *hyper_node; hyper_node = of_find_node_by_path("/hypervisor"); if (!hyper_node) return 0; if (!of_device_is_compatible(hyper_node, "linux,kvm")) return 0; return 1; } extern unsigned long kvm_hypercall(unsigned long *in, unsigned long *out, unsigned long nr); #else static inline int kvm_para_available(void) { return 0; } static unsigned long kvm_hypercall(unsigned long *in, unsigned long *out, unsigned long nr) { return EV_UNIMPLEMENTED; } #endif