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

[Alexander Graf <agraf@suse.de>]

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.


Alex