From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Graf Subject: Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug registers Date: Tue, 24 Jul 2012 14:46:27 +0200 Message-ID: <500E9923.8070504@suse.de> References: <1343042364-30869-1-git-send-email-Bharat.Bhushan@freescale.com> <500D70C9.2080609@freescale.com> <6A3DF150A5B70D4F9B66A25E3F7C888D03DD59C8@039-SN2MPN1-023.039d.mgd.msft.net> <500D8CE1.1050707@suse.de> <6A3DF150A5B70D4F9B66A25E3F7C888D03DD61E6@039-SN2MPN1-023.039d.mgd.msft.net> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Wood Scott-B07421 , "kvm-ppc@vger.kernel.org" , "kvm@vger.kernel.org" To: Bhushan Bharat-R65777 Return-path: In-Reply-To: <6A3DF150A5B70D4F9B66A25E3F7C888D03DD61E6@039-SN2MPN1-023.039d.mgd.msft.net> Sender: kvm-ppc-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On 07/24/2012 03:04 AM, Bhushan Bharat-R65777 wrote: > >> -----Original Message----- >> From: Alexander Graf [mailto:agraf@suse.de] >> Sent: Monday, July 23, 2012 11:12 PM >> To: Bhushan Bharat-R65777 >> Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org >> Subject: Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug >> registers >> >> On 07/23/2012 05:48 PM, Bhushan Bharat-R65777 wrote: >>>> -----Original Message----- >>>> From: Wood Scott-B07421 >>>> Sent: Monday, July 23, 2012 9:12 PM >>>> To: Bhushan Bharat-R65777 >>>> Cc: kvm-ppc@vger.kernel.org; agraf@suse.de; kvm@vger.kernel.org; >>>> Bhushan Bharat- >>>> R65777 >>>> Subject: Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC >>>> debug registers >>>> >>>> On 07/23/2012 06:19 AM, Bharat Bhushan wrote: >>>>> IAC/DAC are defined as 32 bit while they are 64 bit wide. So ONE_REG >>>>> interface is added to set/get them. >>>>> >>>>> Signed-off-by: Bharat Bhushan >>>>> --- >>>>> v2: >>>>> - Using copy_to/from_user() apis. >>>>> >>>>> arch/powerpc/include/asm/kvm.h | 12 ++++++ >>>>> arch/powerpc/include/asm/kvm_host.h | 28 ++++++++++++++- >>>>> arch/powerpc/kvm/booke.c | 64 >> +++++++++++++++++++++++++++++++++- >>>>> arch/powerpc/kvm/booke_emulate.c | 8 ++-- >>>>> 4 files changed, 104 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/arch/powerpc/include/asm/kvm.h >>>>> b/arch/powerpc/include/asm/kvm.h index 1bea4d8..3c14202 100644 >>>>> --- a/arch/powerpc/include/asm/kvm.h >>>>> +++ b/arch/powerpc/include/asm/kvm.h >>>>> @@ -221,6 +221,12 @@ struct kvm_sregs { >>>>> >>>>> __u32 dbsr; /* KVM_SREGS_E_UPDATE_DBSR */ >>>>> __u32 dbcr[3]; >>>>> + /* >>>>> + * iac/dac registers are 64bit wide, while this API >>>>> + * interface provides only lower 32 bits on 64 bit >>>>> + * processors. ONE_REG interface is added for 64bit >>>>> + * iac/dac registers. >>>>> + */ >>>>> __u32 iac[4]; >>>>> __u32 dac[2]; >>>>> __u32 dvc[2]; >>>>> @@ -326,5 +332,11 @@ struct kvm_book3e_206_tlb_params { }; >>>>> >>>>> #define KVM_REG_PPC_HIOR (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x1) >>>>> +#define KVM_REG_PPC_IAC1 (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x2) >>>>> +#define KVM_REG_PPC_IAC2 (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x3) >>>>> +#define KVM_REG_PPC_IAC3 (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x4) >>>>> +#define KVM_REG_PPC_IAC4 (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x5) >>>>> +#define KVM_REG_PPC_DAC1 (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x6) >>>>> +#define KVM_REG_PPC_DAC2 (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x7) >>>>> >>>>> #endif /* __LINUX_KVM_POWERPC_H */ diff --git >>>>> a/arch/powerpc/include/asm/kvm_host.h >>>>> b/arch/powerpc/include/asm/kvm_host.h >>>>> index 43cac48..2c0f015 100644 >>>>> --- a/arch/powerpc/include/asm/kvm_host.h >>>>> +++ b/arch/powerpc/include/asm/kvm_host.h >>>>> @@ -342,6 +342,31 @@ struct kvmppc_slb { >>>>> bool class : 1; >>>>> }; >>>>> >>>>> +#ifdef CONFIG_BOOKE >>>>> +# ifdef CONFIG_PPC_FSL_BOOK3E >>>>> +#define KVMPPC_IAC_NUM 2 >>>>> +#define KVMPPC_DAC_NUM 2 >>>>> +# else >>>>> +#define KVMPPC_IAC_NUM 4 >>>>> +#define KVMPPC_DAC_NUM 2 >>>>> +# endif >>>>> +#define KVMPPC_MAX_IAC 4 >>>>> +#define KVMPPC_MAX_DAC 2 >>>>> +#endif >>>>> + >>>>> +struct kvmppc_debug_reg { >>>>> +#ifdef CONFIG_BOOKE >>>>> + u32 dbcr0; >>>>> + u32 dbcr1; >>>>> + u32 dbcr2; >>>>> +#ifdef CONFIG_KVM_E500MC >>>>> + u32 dbcr4; >>>>> +#endif >>>>> + u64 iac[KVMPPC_MAX_IAC]; >>>>> + u64 dac[KVMPPC_MAX_DAC]; >>>>> +#endif >>>>> +}; >>>> Is there any reason for this to be a separate struct? >>> No specific reason, The rest of debug ( which will follow sometime soon) uses >> this structure and looks to make code look easy. >> >> So why not use an fsl / booke specific struct for the debug patches then? Debug >> registers are really nothing common between book3s and booke, so we shouldn't >> treat them as such by using the same struct definition. >> > All elements of struct are under #ifdef CONFIG_BOOKE? So for book3s it is as good as void, only struct type if declared. Do you want the struct type also under config_booke ? struct kvmppc_booke_debug_reg { }; struct kvmppc_book3s_debug_reg { }; void booke_foo() { struct kvmppc_booke_debug_reg r; r.dbcr0 = 0; } vs struct kvmppc_debug_reg { #ifdef CONFIG_BOOKE #else #endif }; void booke_foo() { struct kvmppc_debug_reg r; r.dbcr0 = 0; } Which one has less #ifdefs? Keep in mind that the less #ifdefs you have, the more readable and maintainable your code becomes, because config options have less effect on your syntax. Alex