Re: [PATCH v7 04/19] x86/VPMU: Make vpmu marcos a bit more efficient

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 06/06/14 18:40, Boris Ostrovsky wrote:
> Update macros that modify VPMU flags to allow changing multiple bits at once.

Modify how?  It appears to only be an introduction of "vcpu_are_all_set()".

>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Acked-by: Kevin Tian <kevin.tian@intel.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reviewed-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>
> Tested-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>
> ---
>  xen/arch/x86/hvm/vmx/vpmu_core2.c | 5 +----
>  xen/arch/x86/hvm/vpmu.c           | 3 +--
>  xen/include/asm-x86/hvm/vpmu.h    | 9 +++++----
>  3 files changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vpmu_core2.c b/xen/arch/x86/hvm/vmx/vpmu_core2.c
> index f5f249f..bf3cd33 100644
> --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c
> +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c
> @@ -326,10 +326,7 @@ static int core2_vpmu_save(struct vcpu *v)
>  {
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
>  
> -    if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_SAVE) )
> -        return 0;
> -
> -    if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) ) 
> +    if ( !vpmu_are_all_set(vpmu, VPMU_CONTEXT_SAVE | VPMU_CONTEXT_LOADED) )
>          return 0;
>  
>      __core2_vpmu_save(v);
> diff --git a/xen/arch/x86/hvm/vpmu.c b/xen/arch/x86/hvm/vpmu.c
> index 63765fa..d456aa6 100644
> --- a/xen/arch/x86/hvm/vpmu.c
> +++ b/xen/arch/x86/hvm/vpmu.c
> @@ -143,8 +143,7 @@ void vpmu_save(struct vcpu *v)
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
>      int pcpu = smp_processor_id();
>  
> -    if ( !(vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) &&
> -           vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED)) )
> +    if ( !vpmu_are_all_set(vpmu, VPMU_CONTEXT_ALLOCATED | VPMU_CONTEXT_LOADED) )
>         return;
>  
>      vpmu->last_pcpu = pcpu;
> diff --git a/xen/include/asm-x86/hvm/vpmu.h b/xen/include/asm-x86/hvm/vpmu.h
> index 40f63fb..7cd7266 100644
> --- a/xen/include/asm-x86/hvm/vpmu.h
> +++ b/xen/include/asm-x86/hvm/vpmu.h
> @@ -81,10 +81,11 @@ struct vpmu_struct {
>  #define VPMU_CPU_HAS_BTS                    0x200 /* Has Branch Trace Store */
>  
>  
> -#define vpmu_set(_vpmu, _x)    ((_vpmu)->flags |= (_x))
> -#define vpmu_reset(_vpmu, _x)  ((_vpmu)->flags &= ~(_x))
> -#define vpmu_is_set(_vpmu, _x) ((_vpmu)->flags & (_x))
> -#define vpmu_clear(_vpmu)      ((_vpmu)->flags = 0)
> +#define vpmu_set(_vpmu, _x)         ((_vpmu)->flags |= (_x))
> +#define vpmu_reset(_vpmu, _x)       ((_vpmu)->flags &= ~(_x))
> +#define vpmu_is_set(_vpmu, _x)      ((_vpmu)->flags & (_x))
> +#define vpmu_are_all_set(_vpmu, _x) (((_vpmu)->flags & (_x)) == (_x))
> +#define vpmu_clear(_vpmu)           ((_vpmu)->flags = 0)

These macros' implicit types don't quite match their implementation.

set, reset and clear are implicitly void. (and frankly clear and reset
are confusing given the other bitopt nomenclature, but I don't suggest
changing them).

is_set and are_all_set are implicitly bool, but don't have !!'s


I realise I am straying vastly into personal preference here, but I feel
these would be much nicer as static inlines with proper types, and const
correctness for {is,are_all}_set().

~Andrew

>  
>  int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content);
>  int vpmu_do_rdmsr(unsigned int msr, uint64_t *msr_content);