RE: [PATCH 1/5] x86/asm: Rename FS/GS base helpers

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Sent: Wednesday, September 9, 2020 5:59 PM
> 
> They are currently named after the FSGSBASE instructions, but are not thin
> wrappers around said instructions, and therefore do not accurately reflect
> the
> logic they perform, especially when it comes to functioning safely in non
> FSGSBASE context.
> 
> Rename them to {read,write}_{fs,gs}_base() to avoid confusion.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>

> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
> ---
>  xen/arch/x86/domain.c          | 10 +++++-----
>  xen/arch/x86/hvm/vmx/vmx.c     |  8 ++++----
>  xen/arch/x86/pv/domain.c       |  2 +-
>  xen/arch/x86/pv/emul-priv-op.c | 14 +++++++-------
>  xen/arch/x86/x86_64/mm.c       |  8 ++++----
>  xen/arch/x86/x86_64/traps.c    |  6 +++---
>  xen/include/asm-x86/msr.h      | 12 ++++++------
>  7 files changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index e8e91cf080..2271bee36a 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1581,9 +1581,9 @@ static void load_segments(struct vcpu *n)
> 
>      if ( !fs_gs_done && !compat )
>      {
> -        wrfsbase(n->arch.pv.fs_base);
> -        wrgsshadow(n->arch.pv.gs_base_kernel);
> -        wrgsbase(n->arch.pv.gs_base_user);
> +        write_fs_base(n->arch.pv.fs_base);
> +        write_gs_shadow(n->arch.pv.gs_base_kernel);
> +        write_gs_base(n->arch.pv.gs_base_user);
> 
>          /* If in kernel mode then switch the GS bases around. */
>          if ( (n->arch.flags & TF_kernel_mode) )
> @@ -1710,9 +1710,9 @@ static void save_segments(struct vcpu *v)
> 
>      if ( !is_pv_32bit_vcpu(v) )
>      {
> -        unsigned long gs_base = rdgsbase();
> +        unsigned long gs_base = read_gs_base();
> 
> -        v->arch.pv.fs_base = rdfsbase();
> +        v->arch.pv.fs_base = read_fs_base();
>          if ( v->arch.flags & TF_kernel_mode )
>              v->arch.pv.gs_base_kernel = gs_base;
>          else
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index c4b40bf3cb..d26e102970 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -512,12 +512,12 @@ static void vmx_save_guest_msrs(struct vcpu *v)
>       * We cannot cache SHADOW_GS_BASE while the VCPU runs, as it can
>       * be updated at any time via SWAPGS, which we cannot trap.
>       */
> -    v->arch.hvm.vmx.shadow_gs = rdgsshadow();
> +    v->arch.hvm.vmx.shadow_gs = read_gs_shadow();
>  }
> 
>  static void vmx_restore_guest_msrs(struct vcpu *v)
>  {
> -    wrgsshadow(v->arch.hvm.vmx.shadow_gs);
> +    write_gs_shadow(v->arch.hvm.vmx.shadow_gs);
>      wrmsrl(MSR_STAR,           v->arch.hvm.vmx.star);
>      wrmsrl(MSR_LSTAR,          v->arch.hvm.vmx.lstar);
>      wrmsrl(MSR_SYSCALL_MASK,   v->arch.hvm.vmx.sfmask);
> @@ -2958,7 +2958,7 @@ static int vmx_msr_read_intercept(unsigned int
> msr, uint64_t *msr_content)
>          break;
> 
>      case MSR_SHADOW_GS_BASE:
> -        *msr_content = rdgsshadow();
> +        *msr_content = read_gs_shadow();
>          break;
> 
>      case MSR_STAR:
> @@ -3190,7 +3190,7 @@ static int vmx_msr_write_intercept(unsigned int
> msr, uint64_t msr_content)
>          else if ( msr == MSR_GS_BASE )
>              __vmwrite(GUEST_GS_BASE, msr_content);
>          else
> -            wrgsshadow(msr_content);
> +            write_gs_shadow(msr_content);
> 
>          break;
> 
> diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
> index 44e4ea2582..663e76c773 100644
> --- a/xen/arch/x86/pv/domain.c
> +++ b/xen/arch/x86/pv/domain.c
> @@ -452,7 +452,7 @@ void toggle_guest_mode(struct vcpu *v)
>       * Update the cached value of the GS base about to become inactive, as a
>       * subsequent context switch won't bother re-reading it.
>       */
> -    gs_base = rdgsbase();
> +    gs_base = read_gs_base();
>      if ( v->arch.flags & TF_kernel_mode )
>          v->arch.pv.gs_base_kernel = gs_base;
>      else
> diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-
> op.c
> index a192160f84..9dd1d59423 100644
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -511,10 +511,10 @@ static int read_segment(enum x86_segment seg,
>              reg->base = 0;
>              break;
>          case x86_seg_fs:
> -            reg->base = rdfsbase();
> +            reg->base = read_fs_base();
>              break;
>          case x86_seg_gs:
> -            reg->base = rdgsbase();
> +            reg->base = read_gs_base();
>              break;
>          }
> 
> @@ -871,13 +871,13 @@ static int read_msr(unsigned int reg, uint64_t *val,
>      case MSR_FS_BASE:
>          if ( is_pv_32bit_domain(currd) )
>              break;
> -        *val = rdfsbase();
> +        *val = read_fs_base();
>          return X86EMUL_OKAY;
> 
>      case MSR_GS_BASE:
>          if ( is_pv_32bit_domain(currd) )
>              break;
> -        *val = rdgsbase();
> +        *val = read_gs_base();
>          return X86EMUL_OKAY;
> 
>      case MSR_SHADOW_GS_BASE:
> @@ -993,19 +993,19 @@ static int write_msr(unsigned int reg, uint64_t val,
>      case MSR_FS_BASE:
>          if ( is_pv_32bit_domain(currd) || !is_canonical_address(val) )
>              break;
> -        wrfsbase(val);
> +        write_fs_base(val);
>          return X86EMUL_OKAY;
> 
>      case MSR_GS_BASE:
>          if ( is_pv_32bit_domain(currd) || !is_canonical_address(val) )
>              break;
> -        wrgsbase(val);
> +        write_gs_base(val);
>          return X86EMUL_OKAY;
> 
>      case MSR_SHADOW_GS_BASE:
>          if ( is_pv_32bit_domain(currd) || !is_canonical_address(val) )
>              break;
> -        wrgsshadow(val);
> +        write_gs_shadow(val);
>          curr->arch.pv.gs_base_user = val;
>          return X86EMUL_OKAY;
> 
> diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
> index b69cf2dc4f..0d11a9f500 100644
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -1030,7 +1030,7 @@ long do_set_segment_base(unsigned int which,
> unsigned long base)
>      {
>      case SEGBASE_FS:
>          if ( is_canonical_address(base) )
> -            wrfsbase(base);
> +            write_fs_base(base);
>          else
>              ret = -EINVAL;
>          break;
> @@ -1038,7 +1038,7 @@ long do_set_segment_base(unsigned int which,
> unsigned long base)
>      case SEGBASE_GS_USER:
>          if ( is_canonical_address(base) )
>          {
> -            wrgsshadow(base);
> +            write_gs_shadow(base);
>              v->arch.pv.gs_base_user = base;
>          }
>          else
> @@ -1047,7 +1047,7 @@ long do_set_segment_base(unsigned int which,
> unsigned long base)
> 
>      case SEGBASE_GS_KERNEL:
>          if ( is_canonical_address(base) )
> -            wrgsbase(base);
> +            write_gs_base(base);
>          else
>              ret = -EINVAL;
>          break;
> @@ -1096,7 +1096,7 @@ long do_set_segment_base(unsigned int which,
> unsigned long base)
>                         : [flat] "r" (FLAT_USER_DS32) );
> 
>          /* Update the cache of the inactive base, as read from the GDT/LDT. 
> */
> -        v->arch.pv.gs_base_user = rdgsbase();
> +        v->arch.pv.gs_base_user = read_gs_base();
> 
>          asm volatile ( safe_swapgs );
>          break;
> diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
> index 93af0c5e87..4f262122b7 100644
> --- a/xen/arch/x86/x86_64/traps.c
> +++ b/xen/arch/x86/x86_64/traps.c
> @@ -47,9 +47,9 @@ static void read_registers(struct cpu_user_regs *regs,
> unsigned long crs[8])
>      regs->es = read_sreg(es);
>      regs->fs = read_sreg(fs);
>      regs->gs = read_sreg(gs);
> -    crs[5] = rdfsbase();
> -    crs[6] = rdgsbase();
> -    crs[7] = rdgsshadow();
> +    crs[5] = read_fs_base();
> +    crs[6] = read_gs_base();
> +    crs[7] = read_gs_shadow();
>  }
> 
>  static void _show_registers(
> diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
> index 5c44c79600..5e141ac5a5 100644
> --- a/xen/include/asm-x86/msr.h
> +++ b/xen/include/asm-x86/msr.h
> @@ -156,7 +156,7 @@ static inline unsigned long __rdgsbase(void)
>      return base;
>  }
> 
> -static inline unsigned long rdfsbase(void)
> +static inline unsigned long read_fs_base(void)
>  {
>      unsigned long base;
> 
> @@ -168,7 +168,7 @@ static inline unsigned long rdfsbase(void)
>      return base;
>  }
> 
> -static inline unsigned long rdgsbase(void)
> +static inline unsigned long read_gs_base(void)
>  {
>      unsigned long base;
> 
> @@ -180,7 +180,7 @@ static inline unsigned long rdgsbase(void)
>      return base;
>  }
> 
> -static inline unsigned long rdgsshadow(void)
> +static inline unsigned long read_gs_shadow(void)
>  {
>      unsigned long base;
> 
> @@ -196,7 +196,7 @@ static inline unsigned long rdgsshadow(void)
>      return base;
>  }
> 
> -static inline void wrfsbase(unsigned long base)
> +static inline void write_fs_base(unsigned long base)
>  {
>      if ( read_cr4() & X86_CR4_FSGSBASE )
>  #ifdef HAVE_AS_FSGSBASE
> @@ -208,7 +208,7 @@ static inline void wrfsbase(unsigned long base)
>          wrmsrl(MSR_FS_BASE, base);
>  }
> 
> -static inline void wrgsbase(unsigned long base)
> +static inline void write_gs_base(unsigned long base)
>  {
>      if ( read_cr4() & X86_CR4_FSGSBASE )
>  #ifdef HAVE_AS_FSGSBASE
> @@ -220,7 +220,7 @@ static inline void wrgsbase(unsigned long base)
>          wrmsrl(MSR_GS_BASE, base);
>  }
> 
> -static inline void wrgsshadow(unsigned long base)
> +static inline void write_gs_shadow(unsigned long base)
>  {
>      if ( read_cr4() & X86_CR4_FSGSBASE )
>      {
> --
> 2.11.0