From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Subject: Re: [PATCH 2/6] KVM: PPC: Book3E: Refactor SPE/FP exit handling Date: Wed, 3 Jul 2013 13:44:33 -0500 Message-ID: <1372877073.8183.140@snotra> References: <1372876139.8183.135@snotra> <5BA99D8C-1B46-472C-97F7-4A174A39A6D7@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; delsp=Yes; format=Flowed Content-Transfer-Encoding: 8BIT Cc: Caraman Mihai Claudiu-B02008 , "kvm-ppc@vger.kernel.org" , "kvm@vger.kernel.org" , "linuxppc-dev@lists.ozlabs.org" To: Alexander Graf Return-path: In-Reply-To: <5BA99D8C-1B46-472C-97F7-4A174A39A6D7@suse.de> (from agraf@suse.de on Wed Jul 3 13:42:12 2013) Content-Disposition: inline Sender: kvm-ppc-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On 07/03/2013 01:42:12 PM, Alexander Graf wrote: > > On 03.07.2013, at 20:28, Scott Wood wrote: > > > On 07/03/2013 10:13:57 AM, Alexander Graf wrote: > >> There is no chip that supports SPE and HV at the same time. So > we'll never hit this anyway, since kvmppc_supports_spe() always > returns false on HV capable systems. > >> Just add a comment saying so and remove the ifdef :). > > > > kvmppc_vcpu_enable_spe isn't defined unless CONFIG_SPE is defined. > More seriously, MSR_SPE is the same as MSR_VEC, so we shouldn't > interpret it as SPE unless CONFIG_SPE is defined. And we can't rely > on the "if (kvmppc_supports_spe())" here because a later patch > changes it to "if (kvmppc_supports_altivec() || > kvmppc_supports_spe())". So I think we still need the ifdef > CONFIG_SPE here. > > > > As for the HV ifndef, we should try not to confuse HV/PR with > e500mc/e500v2, even if we happen to only run HV on e500mc and PR on > e500v2. We would not want to call kvmppc_vcpu_enable_spe() here on a > hypothetical HV target with SPE. And we *would* want to call > kvmppc_vcpu_enable_fp() here on a hypothetical PR target with normal > FP. It's one thing to leave out the latter, since it would involve > writing actual code that we have no way to test at this point, but > quite another to leave out the proper conditions for when we want to > run code that we do have. > > So we should make this an #ifdef CONFIG_SPE rather than #ifndef > CONFIG_KVM_BOOKE_HV? I think it should be "#if !defined(CONFIG_KVM_BOOKE_HV) && defined(CONFIG_SPE)" for the reasons I described in my second paragraph. -Scott