[4/5] x86/HVM: drop tsc_scaling.setup() hook

Commit Message

Jan Beulich Nov. 16, 2023, 1:32 p.m. UTC

This was used by VMX only, and the intended VMCS write can as well
happen from vmx_set_tsc_offset(), invoked (directly or indirectly)
almost immediately after the present call sites of the hook.
vmx_set_tsc_offset() isn't invoked frequently elsewhere, so the extra
VMCS write shouldn't raise performance concerns.

Signed-off-by: Jan Beulich <[e-mail]>

Comments

Roger Pau Monne Nov. 22, 2023, 9:06 a.m. UTC | #1

On Thu, Nov 16, 2023 at 02:32:47PM +0100, Jan Beulich wrote:
> This was used by VMX only, and the intended VMCS write can as well
> happen from vmx_set_tsc_offset(), invoked (directly or indirectly)
> almost immediately after the present call sites of the hook.
> vmx_set_tsc_offset() isn't invoked frequently elsewhere, so the extra
> VMCS write shouldn't raise performance concerns.
> 
> Signed-off-by: Jan Beulich <[e-mail]>

Acked-by: Roger Pau Monné <[e-mail]>

I would be lying if I told you I understand guest TSC management in
Xen, but patch does keep the previous call sites with adding others
that are indeed not hot paths.

I do wonder whether it would be possible to set this before the domain
is started, as AFAICT the TSC scaling ratio is only set before the
domain is started (either by domain create or other toolstack
hypercalls).

Could we maybe further limit the write to the case where the vCPU is
not running?

Thanks, Roger.

Jan Beulich Nov. 22, 2023, 9:13 a.m. UTC | #2

On 22.11.2023 10:06, Roger Pau Monné wrote:
> On Thu, Nov 16, 2023 at 02:32:47PM +0100, Jan Beulich wrote:
>> This was used by VMX only, and the intended VMCS write can as well
>> happen from vmx_set_tsc_offset(), invoked (directly or indirectly)
>> almost immediately after the present call sites of the hook.
>> vmx_set_tsc_offset() isn't invoked frequently elsewhere, so the extra
>> VMCS write shouldn't raise performance concerns.
>>
>> Signed-off-by: Jan Beulich <[e-mail]>
> 
> Acked-by: Roger Pau Monné <[e-mail]>

Thanks.

> I would be lying if I told you I understand guest TSC management in
> Xen, but patch does keep the previous call sites with adding others
> that are indeed not hot paths.
> 
> I do wonder whether it would be possible to set this before the domain
> is started, as AFAICT the TSC scaling ratio is only set before the
> domain is started (either by domain create or other toolstack
> hypercalls).
> 
> Could we maybe further limit the write to the case where the vCPU is
> not running?

Possibly, but not straightforwardly: E.g. I think we'd need to actually
enforce tsc_set_info() to only ever act on non-running (perhaps even
not-yet-started) domains. And I mean not by looking where it's being
called from at present (which may already provide such kind-of-
guarantees), but in the function itself.

Jan

Patch

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1086,9 +1086,6 @@  static int cf_check hvm_load_cpu_ctxt(st
     v->arch.hvm.guest_cr[2] = ctxt.cr2;
     hvm_update_guest_cr(v, 2);
 
-    if ( hvm_funcs.tsc_scaling.setup )
-        alternative_vcall(hvm_funcs.tsc_scaling.setup, v);
-
     v->arch.msrs->tsc_aux = ctxt.msr_tsc_aux;
 
     hvm_set_guest_tsc_fixed(v, ctxt.tsc, d->arch.hvm.sync_tsc);
@@ -4033,9 +4030,6 @@  void hvm_vcpu_reset_state(struct vcpu *v
     hvm_set_segment_register(v, x86_seg_gdtr, &reg);
     hvm_set_segment_register(v, x86_seg_idtr, &reg);
 
-    if ( hvm_funcs.tsc_scaling.setup )
-        alternative_vcall(hvm_funcs.tsc_scaling.setup, v);
-
     /* Sync AP's TSC with BSP's. */
     v->arch.hvm.cache_tsc_offset =
         v->domain->vcpu[0]->arch.hvm.cache_tsc_offset;
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1454,20 +1454,13 @@  static void cf_check vmx_handle_cd(struc
     }
 }
 
-static void cf_check vmx_setup_tsc_scaling(struct vcpu *v)
-{
-    if ( v->domain->arch.vtsc )
-        return;
-
-    vmx_vmcs_enter(v);
-    __vmwrite(TSC_MULTIPLIER, hvm_tsc_scaling_ratio(v->domain));
-    vmx_vmcs_exit(v);
-}
-
 static void cf_check vmx_set_tsc_offset(struct vcpu *v, u64 offset, u64 at_tsc)
 {
     vmx_vmcs_enter(v);
 
+    if ( !v->domain->arch.vtsc && cpu_has_vmx_tsc_scaling )
+        __vmwrite(TSC_MULTIPLIER, hvm_tsc_scaling_ratio(v->domain));
+
     if ( nestedhvm_vcpu_in_guestmode(v) )
         offset += nvmx_get_tsc_offset(v);
 
@@ -3030,10 +3023,7 @@  const struct hvm_function_table * __init
     }
 
     if ( cpu_has_vmx_tsc_scaling )
-    {
         vmx_function_table.tsc_scaling.ratio_frac_bits = 48;
-        vmx_function_table.tsc_scaling.setup = vmx_setup_tsc_scaling;
-    }
 
     model_specific_lbr = get_model_specific_lbr();
     lbr_tsx_fixup_check();
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -240,9 +240,6 @@  struct hvm_function_table {
         uint8_t  ratio_frac_bits;
         /* maximum-allowed TSC scaling ratio */
         uint64_t max_ratio;
-
-        /* Architecture function to setup TSC scaling ratio */
-        void (*setup)(struct vcpu *v);
     } tsc_scaling;
 };