RE: [PATCH V3 1/2] x86/msr: add msr_set/clear_bit_on_cpu/cpus access functions

[Liang, Kan]

.
> > msr_set/clear_bit() are not protected by anyhting. And in your call
> > site this is invoked from fully preemptible context. What protects
> > against context switch and interrupts fiddling with DEBUGMSR?
> 
> And thinking more about that whole interface. It's just overkill.
> 
> diff --git a/arch/x86/lib/msr.c b/arch/x86/lib/msr.c index
> d1dee753b949..35763927adaa 100644
> --- a/arch/x86/lib/msr.c
> +++ b/arch/x86/lib/msr.c
> @@ -58,7 +58,7 @@ int msr_write(u32 msr, struct msr *m)
>  	return wrmsrl_safe(msr, m->q);
>  }
> 
> -static inline int __flip_bit(u32 msr, u8 bit, bool set)
> +int msr_flip_bit(u32 msr, u8 bit, bool set)
>  {
>  	struct msr m, m1;
>  	int err = -EINVAL;
> @@ -85,6 +85,7 @@ static inline int __flip_bit(u32 msr, u8 bit, bool set)
> 
>  	return 1;
>  }
> +EXPORT_SYMBOL_GPL(msr_flip_bit);
> 
>  /**
>   * Set @bit in a MSR @msr.
> @@ -96,7 +97,7 @@ static inline int __flip_bit(u32 msr, u8 bit, bool set)
>   */
>  int msr_set_bit(u32 msr, u8 bit)
>  {
> -	return __flip_bit(msr, bit, true);
> +	return msr_flip_bit(msr, bit, true);
>  }
> 
>  /**
> @@ -109,7 +110,7 @@ int msr_set_bit(u32 msr, u8 bit)
>   */
>  int msr_clear_bit(u32 msr, u8 bit)
>  {
> -	return __flip_bit(msr, bit, false);
> +	return msr_flip_bit(msr, bit, false);
>  }
> 
>  #ifdef CONFIG_TRACEPOINTS
> 
> And in the driver:
> 
> static void flip_smm_bit(void *data)
> {
> 	int val = *(int *)data;
> 
> 	msr_flip_bit(DEBUGMSR, SMMBIT, val);
> }
> 
> And in the write function:
> 
>        smp_call_function(flip_smm_bit, &val, 1);
> 
> That avoids all the extra interfaces and requires less code and less text foot
> print when unused .....
> 

Thanks. It simplify the code very much.
I think we still need to protect the smp_call_function in the driver, right?
Would be the following code enough?

get_online_cpus();
preempt_disable();
smp_call_function(flip_smm_bit, &val, 1);
preempt_enable();
put_online_cpus();

Thanks,
Kan