| Message ID | 1343042364-30869-1-git-send-email-Bharat.Bhushan@freescale.com |
|---|---|
| State | New, archived |
| Headers | show |
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? > + > struct kvm_vcpu_arch { > ulong host_stack; > u32 host_pid; > @@ -436,9 +461,8 @@ struct kvm_vcpu_arch { > > u32 ccr0; > u32 ccr1; > - u32 dbcr0; > - u32 dbcr1; > u32 dbsr; > + struct kvmppc_debug_reg dbg_reg; > > u64 mmcr[3]; > u32 pmc[8]; > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c > index be71b8e..fa23158 100644 > --- a/arch/powerpc/kvm/booke.c > +++ b/arch/powerpc/kvm/booke.c > @@ -1353,12 +1353,72 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, > > int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg) > { > - return -EINVAL; > + int r = -EINVAL; > + > + switch(reg->id) { > + case KVM_REG_PPC_IAC1: > + r = copy_to_user((u64 __user *)(long)reg->addr, > + &vcpu->arch.dbg_reg.iac[0], sizeof(u64)); > + break; > + case KVM_REG_PPC_IAC2: > + r = copy_to_user((u64 __user *)(long)reg->addr, > + &vcpu->arch.dbg_reg.iac[1], sizeof(u64)); > + break; > + case KVM_REG_PPC_IAC3: > + r = copy_to_user((u64 __user *)(long)reg->addr, > + &vcpu->arch.dbg_reg.iac[2], sizeof(u64)); > + break; > + case KVM_REG_PPC_IAC4: > + r = copy_to_user((u64 __user *)(long)reg->addr, > + &vcpu->arch.dbg_reg.iac[3], sizeof(u64)); > + break; This will exceed the array size if userspace asks to access IAC3/4 on an e500-family chip. Why not just set the array at the max size unconditionally? -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----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. > > > + > > struct kvm_vcpu_arch { > > ulong host_stack; > > u32 host_pid; > > @@ -436,9 +461,8 @@ struct kvm_vcpu_arch { > > > > u32 ccr0; > > u32 ccr1; > > - u32 dbcr0; > > - u32 dbcr1; > > u32 dbsr; > > + struct kvmppc_debug_reg dbg_reg; > > > > u64 mmcr[3]; > > u32 pmc[8]; > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index > > be71b8e..fa23158 100644 > > --- a/arch/powerpc/kvm/booke.c > > +++ b/arch/powerpc/kvm/booke.c > > @@ -1353,12 +1353,72 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct > > kvm_vcpu *vcpu, > > > > int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, struct > > kvm_one_reg *reg) { > > - return -EINVAL; > > + int r = -EINVAL; > > + > > + switch(reg->id) { > > + case KVM_REG_PPC_IAC1: > > + r = copy_to_user((u64 __user *)(long)reg->addr, > > + &vcpu->arch.dbg_reg.iac[0], sizeof(u64)); > > + break; > > + case KVM_REG_PPC_IAC2: > > + r = copy_to_user((u64 __user *)(long)reg->addr, > > + &vcpu->arch.dbg_reg.iac[1], sizeof(u64)); > > + break; > > + case KVM_REG_PPC_IAC3: > > + r = copy_to_user((u64 __user *)(long)reg->addr, > > + &vcpu->arch.dbg_reg.iac[2], sizeof(u64)); > > + break; > > + case KVM_REG_PPC_IAC4: > > + r = copy_to_user((u64 __user *)(long)reg->addr, > > + &vcpu->arch.dbg_reg.iac[3], sizeof(u64)); > > + break; > > This will exceed the array size if userspace asks to access IAC3/4 on an e500- > family chip. No , is not the array size already set to maximum. But yes we should not let IAC3/4 being accessed for e500 (FSL_BOOKE). > > Why not just set the array at the max size unconditionally? This is already coded this way. Please see the struct is this patch. Thanks -Bharat > > -Scott
On 07/23/2012 10:48 AM, 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 >> >> This will exceed the array size if userspace asks to access IAC3/4 on an e500- >> family chip. > > No , is not the array size already set to maximum. But yes we should not let IAC3/4 being accessed for e500 (FSL_BOOKE). > >> >> Why not just set the array at the max size unconditionally? > > This is already coded this way. Please see the struct is this patch. Sorry, I misread the array declaration. -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----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 ? Thanks -Bharat
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 -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>>>> +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; kvmppc_booke_debug_reg or kvmppc_book3s_debug_reg ? Thanks -Bharat > 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 > > -- > To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body > of a message to majordomo@vger.kernel.org More majordomo info at > http://vger.kernel.org/majordomo-info.html
On 07/24/2012 03:26 PM, Bhushan Bharat-R65777 wrote: >>>>>>> +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; > kvmppc_booke_debug_reg or kvmppc_book3s_debug_reg ? In booke_foo() you certainly will never want to use struct kvmppc_book3s_debug_reg, no? Alex -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 +}; + struct kvm_vcpu_arch { ulong host_stack; u32 host_pid; @@ -436,9 +461,8 @@ struct kvm_vcpu_arch { u32 ccr0; u32 ccr1; - u32 dbcr0; - u32 dbcr1; u32 dbsr; + struct kvmppc_debug_reg dbg_reg; u64 mmcr[3]; u32 pmc[8]; diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index be71b8e..fa23158 100644 --- a/arch/powerpc/kvm/booke.c +++ b/arch/powerpc/kvm/booke.c @@ -1353,12 +1353,72 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg) { - return -EINVAL; + int r = -EINVAL; + + switch(reg->id) { + case KVM_REG_PPC_IAC1: + r = copy_to_user((u64 __user *)(long)reg->addr, + &vcpu->arch.dbg_reg.iac[0], sizeof(u64)); + break; + case KVM_REG_PPC_IAC2: + r = copy_to_user((u64 __user *)(long)reg->addr, + &vcpu->arch.dbg_reg.iac[1], sizeof(u64)); + break; + case KVM_REG_PPC_IAC3: + r = copy_to_user((u64 __user *)(long)reg->addr, + &vcpu->arch.dbg_reg.iac[2], sizeof(u64)); + break; + case KVM_REG_PPC_IAC4: + r = copy_to_user((u64 __user *)(long)reg->addr, + &vcpu->arch.dbg_reg.iac[3], sizeof(u64)); + break; + case KVM_REG_PPC_DAC1: + r = copy_to_user((u64 __user *)(long)reg->addr, + &vcpu->arch.dbg_reg.dac[0], sizeof(u64)); + break; + case KVM_REG_PPC_DAC2: + r = copy_to_user((u64 __user *)(long)reg->addr, + &vcpu->arch.dbg_reg.dac[1], sizeof(u64)); + break; + default: + break; + } + return r; } int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg) { - return -EINVAL; + int r = -EINVAL; + + switch(reg->id) { + case KVM_REG_PPC_IAC1: + r = copy_from_user(&vcpu->arch.dbg_reg.iac[0], + (u64 __user *)(long)reg->addr, sizeof(u64)); + break; + case KVM_REG_PPC_IAC2: + r = copy_from_user(&vcpu->arch.dbg_reg.iac[1], + (u64 __user *)(long)reg->addr, sizeof(u64)); + break; + case KVM_REG_PPC_IAC3: + r = copy_from_user(&vcpu->arch.dbg_reg.iac[2], + (u64 __user *)(long)reg->addr, sizeof(u64)); + break; + case KVM_REG_PPC_IAC4: + r = copy_from_user(&vcpu->arch.dbg_reg.iac[3], + (u64 __user *)(long)reg->addr, sizeof(u64)); + break; + case KVM_REG_PPC_DAC1: + r = copy_from_user(&vcpu->arch.dbg_reg.dac[0], + (u64 __user *)(long)reg->addr, sizeof(u64)); + break; + case KVM_REG_PPC_DAC2: + r = copy_from_user(&vcpu->arch.dbg_reg.dac[1], + (u64 __user *)(long)reg->addr, sizeof(u64)); + break; + default: + break; + } + return r; } int kvm_arch_vcpu_ioctl_get_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu) diff --git a/arch/powerpc/kvm/booke_emulate.c b/arch/powerpc/kvm/booke_emulate.c index 5a66ade..cc99a0b 100644 --- a/arch/powerpc/kvm/booke_emulate.c +++ b/arch/powerpc/kvm/booke_emulate.c @@ -133,10 +133,10 @@ int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn, ulong spr_val) vcpu->arch.csrr1 = spr_val; break; case SPRN_DBCR0: - vcpu->arch.dbcr0 = spr_val; + vcpu->arch.dbg_reg.dbcr0 = spr_val; break; case SPRN_DBCR1: - vcpu->arch.dbcr1 = spr_val; + vcpu->arch.dbg_reg.dbcr1 = spr_val; break; case SPRN_DBSR: vcpu->arch.dbsr &= ~spr_val; @@ -266,10 +266,10 @@ int kvmppc_booke_emulate_mfspr(struct kvm_vcpu *vcpu, int sprn, ulong *spr_val) *spr_val = vcpu->arch.csrr1; break; case SPRN_DBCR0: - *spr_val = vcpu->arch.dbcr0; + *spr_val = vcpu->arch.dbg_reg.dbcr0; break; case SPRN_DBCR1: - *spr_val = vcpu->arch.dbcr1; + *spr_val = vcpu->arch.dbg_reg.dbcr1; break; case SPRN_DBSR: *spr_val = vcpu->arch.dbsr;
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(-)