| Message ID | d6a8ce0e-fcd9-4391-83c1-d9f709ada3f1@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/HVM: misc tidying | expand |
On Thu, Nov 16, 2023 at 02:32:07PM +0100, Jan Beulich wrote: > While generally benign, doing so is still at best misleading. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > Using set_in_cr4() seems favorable over updating mmu_cr4_features > despite the resulting redundant CR4 update. But I certainly could be > talked into going the alternative route. Hm, yes I wondered the same, overall I guess it's safer to just use set_in_cr4() and do the redundant CR4 write. It's just for the BSP anyway. > > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -2959,7 +2959,7 @@ static bool __init has_if_pschange_mc(vo > > const struct hvm_function_table * __init start_vmx(void) > { > - set_in_cr4(X86_CR4_VMXE); > + write_cr4(read_cr4() | X86_CR4_VMXE); > > if ( vmx_vmcs_init() ) > { > @@ -2967,6 +2967,9 @@ const struct hvm_function_table * __init > return NULL; Do we want to also clear VMXE from CR4 here? Thanks, Roger.
On 21.11.2023 18:30, Roger Pau Monné wrote: > On Thu, Nov 16, 2023 at 02:32:07PM +0100, Jan Beulich wrote: >> --- a/xen/arch/x86/hvm/vmx/vmx.c >> +++ b/xen/arch/x86/hvm/vmx/vmx.c >> @@ -2959,7 +2959,7 @@ static bool __init has_if_pschange_mc(vo >> >> const struct hvm_function_table * __init start_vmx(void) >> { >> - set_in_cr4(X86_CR4_VMXE); >> + write_cr4(read_cr4() | X86_CR4_VMXE); >> >> if ( vmx_vmcs_init() ) >> { >> @@ -2967,6 +2967,9 @@ const struct hvm_function_table * __init >> return NULL; > > Do we want to also clear VMXE from CR4 here? Yes, definitely. That was the point of the patch (as far as the BSP is concerned); I clearly meant to have that there, but then didn't put it there. Jan
--- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -2959,7 +2959,7 @@ static bool __init has_if_pschange_mc(vo const struct hvm_function_table * __init start_vmx(void) { - set_in_cr4(X86_CR4_VMXE); + write_cr4(read_cr4() | X86_CR4_VMXE); if ( vmx_vmcs_init() ) { @@ -2967,6 +2967,9 @@ const struct hvm_function_table * __init return NULL; } + /* Arrange for APs to have CR4.VMXE set early on. */ + set_in_cr4(X86_CR4_VMXE); + vmx_function_table.singlestep_supported = cpu_has_monitor_trap_flag; if ( cpu_has_vmx_dt_exiting )
While generally benign, doing so is still at best misleading. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Using set_in_cr4() seems favorable over updating mmu_cr4_features despite the resulting redundant CR4 update. But I certainly could be talked into going the alternative route.