[RFC PATCH 02/17] KVM: PPC64: booke: Add EPCR support in mtspr/mfspr emulation

[Alexander Graf]

On 04.07.2012, at 16:14, Caraman Mihai Claudiu-B02008 wrote:

>> -----Original Message-----
>> From: kvm-ppc-owner at vger.kernel.org [mailto:kvm-ppc-
>> owner at vger.kernel.org] On Behalf Of Alexander Graf
>> Sent: Wednesday, July 04, 2012 4:22 PM
>> To: Caraman Mihai Claudiu-B02008
>> Cc: kvm-ppc at vger.kernel.org; kvm at vger.kernel.org; linuxppc-
>> dev at lists.ozlabs.org; qemu-ppc at nongnu.org
>> Subject: Re: [RFC PATCH 02/17] KVM: PPC64: booke: Add EPCR support in
>> mtspr/mfspr emulation
>> 
>> 
>> On 25.06.2012, at 14:26, Mihai Caraman wrote:
>> 
>>> Add EPCR support in booke mtspr/mfspr emulation. EPCR register is
>> defined
>>> only for 64-bit and HV categories, so it shoud be available only on 64-
>> bit
>>> virtual processors. Undefine the support for 32-bit builds.
>>> Define a reusable setter function for vcpu's EPCR.
>>> 
>>> Signed-off-by: Mihai Caraman <mihai.caraman at freescale.com>
>>> ---
>>> arch/powerpc/kvm/booke.c         |   12 +++++++++++-
>>> arch/powerpc/kvm/booke.h         |    6 ++++++
>>> arch/powerpc/kvm/booke_emulate.c |   13 ++++++++++++-
>>> 3 files changed, 29 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>>> index 72f13f4..f9fa260 100644
>>> --- a/arch/powerpc/kvm/booke.c
>>> +++ b/arch/powerpc/kvm/booke.c
>>> @@ -13,7 +13,7 @@
>>> * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301,
>> USA.
>>> *
>>> * Copyright IBM Corp. 2007
>>> - * Copyright 2010-2011 Freescale Semiconductor, Inc.
>>> + * Copyright 2010-2012 Freescale Semiconductor, Inc.
>>> *
>>> * Authors: Hollis Blanchard <hollisb at us.ibm.com>
>>> *          Christian Ehrhardt <ehrhardt at linux.vnet.ibm.com>
>>> @@ -1243,6 +1243,16 @@ void kvmppc_core_commit_memory_region(struct kvm
>> *kvm,
>>> {
>>> }
>>> 
>>> +#ifdef CONFIG_64BIT
>>> +void kvmppc_set_epcr(struct kvm_vcpu *vcpu, u32 new_epcr)
>>> +{
>>> +	vcpu->arch.epcr = new_epcr;
>>> +	vcpu->arch.shadow_epcr &= ~SPRN_EPCR_GICM;
>>> +	if (vcpu->arch.epcr  & SPRN_EPCR_ICM)
>>> +		vcpu->arch.shadow_epcr |= SPRN_EPCR_GICM;
>> 
>> Why would the setter be #ifdef CONFIG_64BIT? EPCR exists on e500mc too,
>> no? Please only #ifdef the GICM bits out.
> 
> kvmppc_set_epcr deals with guest EPCR and EPCR does not exist on a virtual e500mc
> as detailed in patch's comment. All callers are also guarded by #ifdef CONFIG_64BIT,
> my assumption was that we will not support a virtual core with 64-bit category
> on a 32-bit host.

My main concern is that every #ifdef potentially breaks things without us knowing. So the less #ifdef's we have, the better off we are. The spec only says that we don't _have_ to implement EPCR for non-hv non-64bit systems. It doesn't forbid to do so, right?


Alex