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(-)
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.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@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
> From: Andrew Cooper <andrew.cooper3@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.cooper3@citrix.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@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
On 21.01.2022 12:22, Andrew Cooper wrote:
> 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.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Just one minor request for consideration at the end.
> 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.
Hmm. Maybe best to leave alone in stable trees?
> @@ -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;
> }
I think it wasn't too long ago that I saw you ask for an "else" like
this one to be dropped (in someone else's patch). May I ask that you
consider doing so here, perhaps going straight to the more compact
case MSR_SHADOW_GS_BASE:
if ( v != curr )
return v->arch.hvm.vmx.shadow_gs;
rdmsrl(MSR_SHADOW_GS_BASE, val);
return val;
?
Actually, as I notice only now: Would you mind making "curr" here and
in the VMX equivalent pointer-to-const?
Jan
© 2016 - 2024 Red Hat, Inc.