> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Monday, July 23, 2012 9:12 PM
> To: Bhushan Bharat-R65777
> Cc: kvm-ppc@vger.kernel.org; ag...@suse.de; k...@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.bhus...@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
