> From: Andrew Cooper <andrew.coop...@citrix.com>
> Sent: Friday, January 21, 2022 7:23 PM
>
> This is a trivial accessor for an MSR, so use hvm_get_reg() rather than a
> dedicated hook. In arch_get_info_guest(), rework the logic to read
> GS_SHADOW
> only once.
>
> get_hvm_registers() is called on current, meaning that diagnostics print a
> stale GS_SHADOW from the previous vcpu context switch. Adjust both
> implementations to obtain the correct value.
>
> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
Reviewed-by: Kevin Tian <kevin.t...@intel.com>
> ---
> CC: Jan Beulich <jbeul...@suse.com>
> CC: Roger Pau Monné <roger....@citrix.com>
> CC: Wei Liu <w...@xen.org>
> CC: Jun Nakajima <jun.nakaj...@intel.com>
> CC: Kevin Tian <kevin.t...@intel.com>
>
> If we care to backport the bugfix aspect, a far less invasive option would be
> to read MSR_SHADOW_GS_BASE directly.
>
> The only case where that goes wrong is when vmcb->kerngsbase has been
> modified
> and is pending a VMLOAD. I'm fairly sure this can only occur when we need
> vcpu diagnostics, after an emulated write of MSR_SHADOW_GS_BASE.
> ---
> xen/arch/x86/domctl.c | 8 ++++++--
> xen/arch/x86/hvm/svm/svm.c | 12 ++++++------
> xen/arch/x86/hvm/vmx/vmx.c | 16 ++++++++++------
> xen/arch/x86/include/asm/hvm/hvm.h | 7 -------
> xen/arch/x86/x86_64/traps.c | 2 +-
> 5 files changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 7d102e0647ec..e49f9e91b9fa 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1447,6 +1447,7 @@ void arch_get_info_guest(struct vcpu *v,
> vcpu_guest_context_u c)
> if ( is_hvm_domain(d) )
> {
> struct segment_register sreg;
> + unsigned long gs_shadow;
>
> c.nat->ctrlreg[0] = v->arch.hvm.guest_cr[0];
> c.nat->ctrlreg[2] = v->arch.hvm.guest_cr[2];
> @@ -1465,15 +1466,18 @@ void arch_get_info_guest(struct vcpu *v,
> vcpu_guest_context_u c)
> c.nat->fs_base = sreg.base;
> hvm_get_segment_register(v, x86_seg_gs, &sreg);
> c.nat->user_regs.gs = sreg.sel;
> +
> + gs_shadow = hvm_get_reg(v, MSR_SHADOW_GS_BASE);
> +
> if ( ring_0(&c.nat->user_regs) )
> {
> c.nat->gs_base_kernel = sreg.base;
> - c.nat->gs_base_user = hvm_get_shadow_gs_base(v);
> + c.nat->gs_base_user = gs_shadow;
> }
> else
> {
> c.nat->gs_base_user = sreg.base;
> - c.nat->gs_base_kernel = hvm_get_shadow_gs_base(v);
> + c.nat->gs_base_kernel = gs_shadow;
> }
> }
> else
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index bb6b8e560a9f..e3bc88e78058 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -752,11 +752,6 @@ static void svm_set_segment_register(struct vcpu *v,
> enum x86_segment seg,
> }
> }
>
> -static unsigned long svm_get_shadow_gs_base(struct vcpu *v)
> -{
> - return v->arch.hvm.svm.vmcb->kerngsbase;
> -}
> -
> static int svm_set_guest_pat(struct vcpu *v, u64 gpat)
> {
> struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
> @@ -2471,10 +2466,16 @@ static bool svm_get_pending_event(struct vcpu
> *v, struct x86_event *info)
>
> static uint64_t svm_get_reg(struct vcpu *v, unsigned int reg)
> {
> + struct vcpu *curr = current;
> struct domain *d = v->domain;
>
> switch ( reg )
> {
> + case MSR_SHADOW_GS_BASE:
> + if ( v == curr )
> + svm_sync_vmcb(v, vmcb_in_sync);
> + return v->arch.hvm.svm.vmcb->kerngsbase;
> +
> default:
> printk(XENLOG_G_ERR "%s(%pv, 0x%08x) Bad register\n",
> __func__, v, reg);
> @@ -2513,7 +2514,6 @@ static struct hvm_function_table __initdata
> svm_function_table = {
> .get_cpl = svm_get_cpl,
> .get_segment_register = svm_get_segment_register,
> .set_segment_register = svm_set_segment_register,
> - .get_shadow_gs_base = svm_get_shadow_gs_base,
> .update_guest_cr = svm_update_guest_cr,
> .update_guest_efer = svm_update_guest_efer,
> .cpuid_policy_changed = svm_cpuid_policy_changed,
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index c44cf8f5d425..27c36af6027d 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1183,11 +1183,6 @@ static void vmx_set_segment_register(struct vcpu
> *v, enum x86_segment seg,
> vmx_vmcs_exit(v);
> }
>
> -static unsigned long vmx_get_shadow_gs_base(struct vcpu *v)
> -{
> - return v->arch.hvm.vmx.shadow_gs;
> -}
> -
> static int vmx_set_guest_pat(struct vcpu *v, u64 gpat)
> {
> if ( !paging_mode_hap(v->domain) ||
> @@ -2401,6 +2396,7 @@ static int vmtrace_reset(struct vcpu *v)
>
> static uint64_t vmx_get_reg(struct vcpu *v, unsigned int reg)
> {
> + struct vcpu *curr = current;
> struct domain *d = v->domain;
> uint64_t val = 0;
> int rc;
> @@ -2417,6 +2413,15 @@ static uint64_t vmx_get_reg(struct vcpu *v,
> unsigned int reg)
> domain_crash(d);
> }
> return val;
> +
> + case MSR_SHADOW_GS_BASE:
> + if ( v == curr )
> + {
> + rdmsrl(MSR_SHADOW_GS_BASE, val);
> + return val;
> + }
> + else
> + return v->arch.hvm.vmx.shadow_gs;
> }
>
> /* Logic which maybe requires remote VMCS acquisition. */
> @@ -2489,7 +2494,6 @@ static struct hvm_function_table __initdata
> vmx_function_table = {
> .get_cpl = _vmx_get_cpl,
> .get_segment_register = vmx_get_segment_register,
> .set_segment_register = vmx_set_segment_register,
> - .get_shadow_gs_base = vmx_get_shadow_gs_base,
> .update_host_cr3 = vmx_update_host_cr3,
> .update_guest_cr = vmx_update_guest_cr,
> .update_guest_efer = vmx_update_guest_efer,
> diff --git a/xen/arch/x86/include/asm/hvm/hvm.h
> b/xen/arch/x86/include/asm/hvm/hvm.h
> index 842f98763c4b..feb9d221a6a6 100644
> --- a/xen/arch/x86/include/asm/hvm/hvm.h
> +++ b/xen/arch/x86/include/asm/hvm/hvm.h
> @@ -128,7 +128,6 @@ struct hvm_function_table {
> struct segment_register *reg);
> void (*set_segment_register)(struct vcpu *v, enum x86_segment seg,
> struct segment_register *reg);
> - unsigned long (*get_shadow_gs_base)(struct vcpu *v);
>
> /*
> * Re-set the value of CR3 that Xen runs on when handling VM exits.
> @@ -469,11 +468,6 @@ hvm_get_cpl(struct vcpu *v)
> return alternative_call(hvm_funcs.get_cpl, v);
> }
>
> -static inline unsigned long hvm_get_shadow_gs_base(struct vcpu *v)
> -{
> - return alternative_call(hvm_funcs.get_shadow_gs_base, v);
> -}
> -
> #define has_hvm_params(d) \
> ((d)->arch.hvm.params != NULL)
>
> @@ -753,7 +747,6 @@ void hvm_set_reg(struct vcpu *v, unsigned int reg,
> uint64_t val);
> * needed because DCE will kick in.
> */
> int hvm_guest_x86_mode(struct vcpu *v);
> -unsigned long hvm_get_shadow_gs_base(struct vcpu *v);
> void hvm_cpuid_policy_changed(struct vcpu *v);
> void hvm_set_tsc_offset(struct vcpu *v, uint64_t offset, uint64_t at_tsc);
>
> diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
> index d661d7ffcaaf..d97bf07b22bc 100644
> --- a/xen/arch/x86/x86_64/traps.c
> +++ b/xen/arch/x86/x86_64/traps.c
> @@ -79,7 +79,7 @@ static void get_hvm_registers(struct vcpu *v, struct
> cpu_user_regs *regs,
> hvm_get_segment_register(v, x86_seg_ss, &sreg);
> regs->ss = sreg.sel;
>
> - crs[7] = hvm_get_shadow_gs_base(v);
> + crs[7] = hvm_get_reg(v, MSR_SHADOW_GS_BASE);
> }
>
> static void _show_registers(
> --
> 2.11.0
