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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Monday, July 23, 2012 9:12 PM
> To: Bhushan Bharat-R65777
> Cc: kvm-ppc@xxxxxxxxxxxxxxx; agraf@xxxxxxx; kvm@xxxxxxxxxxxxxxx; 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@xxxxxxxxxxxxx>
> > ---
> > 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

��.n��������+%������w��{.n�����o�^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�

[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux