Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug registers

[Alexander Graf <agraf@suse.de>]

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 <bharat.bhushan@freescale.com>
>>>>> ---
>>>>> 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 {
    <lots of defines>
};

struct kvmppc_book3s_debug_reg {
    <lots of other defines>
};

void booke_foo() {
   struct kvmppc_booke_debug_reg r;
   r.dbcr0 = 0;
}

vs

struct kvmppc_debug_reg {
#ifdef CONFIG_BOOKE
   <lots of defines>
#else
   <lots of other defines>
#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