Re: [PATCH] Added one_reg interface for timer registers

[Scott Wood <scottwood@freescale.com>]

On 02/08/2013 04:06:14 AM, Bharat Bhushan wrote:
> If userspace wants to change some specific bits of TSR
> (timer status register) then it uses GET/SET_SREGS ioctl interface.
> So the steps will be:
>       i)   user-space will make get ioctl,
>       ii)  change TSR in userspace
>       iii) then make set ioctl.
> It can happen that TSR gets changed by kernel after step i) and
> before step iii).
> 
> To avoid this we have added below one_reg ioctls for oring and  
> clearing
> specific bits in TSR. This patch adds one registerface for:
>      1) setting specific bit in TSR (timer status register)
>      2) clearing specific bit in TSR (timer status register)
>      3) setting the TCR register. There are cases where we want to  
> only
>         change TCR and not TSR. Although we can uses SREGS without
>         KVM_SREGS_E_UPDATE_TSR flag but I think one reg is better. I  
> am open
>         if someone feels we should use SREGS only here.
> 
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> ---
>  arch/powerpc/include/uapi/asm/kvm.h |    4 ++++
>  arch/powerpc/kvm/booke.c            |   18 ++++++++++++++++++
>  2 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/include/uapi/asm/kvm.h  
> b/arch/powerpc/include/uapi/asm/kvm.h
> index 16064d0..b4ad5d4 100644
> --- a/arch/powerpc/include/uapi/asm/kvm.h
> +++ b/arch/powerpc/include/uapi/asm/kvm.h
> @@ -417,4 +417,8 @@ struct kvm_get_htab_header {
>  #define KVM_REG_PPC_EPCR	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x85)
>  #define KVM_REG_PPC_EPR		(KVM_REG_PPC | KVM_REG_SIZE_U32  
> | 0x86)
> 
> +/* Timer Status Register OR/CLEAR interface */
> +#define KVM_REG_PPC_OR_TSR	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x87)
> +#define KVM_REG_PPC_CLEAR_TSR	(KVM_REG_PPC | KVM_REG_SIZE_U32  
> | 0x88)
> +#define KVM_REG_PPC_SET_TCR	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x89)
>  #endif /* __LINUX_KVM_POWERPC_H */
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 020923e..983c06f 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -1481,6 +1481,24 @@ int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu  
> *vcpu, struct kvm_one_reg *reg)
>  		break;
>  	}
>  #endif
> +	case KVM_REG_PPC_OR_TSR: {
> +		u32 tsr_bits;
> +		r = get_user(tsr_bits, (u32 __user *)(long)reg->addr);
> +		kvmppc_set_tsr_bits(vcpu, tsr_bits);
> +		break;
> +	}
> +	case KVM_REG_PPC_CLEAR_TSR: {
> +		u32 tsr_bits;
> +		r = get_user(tsr_bits, (u32 __user *)(long)reg->addr);
> +		kvmppc_clr_tsr_bits(vcpu, tsr_bits);
> +		break;
> +	}
> +	case KVM_REG_PPC_SET_TCR: {
> +		u32 tcr;
> +		r = get_user(tcr, (u32 __user *)(long)reg->addr);
> +		kvmppc_set_tcr(vcpu, tcr);
> +		break;
> +	}

KVM_REG_PPC_SET_TCR should just be KVM_REG_PPC_TCR -- we should be able  
to "GET" it too...  Might as well add a KVM_REG_PPC_TSR as well that  
has normal read/write semantics (in addition to the CLEAR and OR  
versions), if we're going to do it for TCR.

-Scott