| Message ID | 20180302105503.24428-2-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 2, 2018 at 10:55 AM, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > vmx_save_host_state() is only called from kvm_arch_vcpu_ioctl_run() so > the context is pretty well defined > True. > and MSR_FS_BASE should always be > equal to current->thread.fsbase. Not true. current->thread.fsbase is almost entirely undefined in this context. What you *could* do is export save_fsgs() and call it first. When FSGSBASE support lands (which will happen eventually!), the code in your patch will be completely wrong. Admittedly, your patch isn't 100% bogus, but the reason is subtle and you need lots of comments there *and* in save_fsgs().
Andy Lutomirski <luto@kernel.org> writes: > On Fri, Mar 2, 2018 at 10:55 AM, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >> vmx_save_host_state() is only called from kvm_arch_vcpu_ioctl_run() so >> the context is pretty well defined >> > > True. > >> and MSR_FS_BASE should always be >> equal to current->thread.fsbase. > > Not true. current->thread.fsbase is almost entirely undefined in this > context. What you *could* do is export save_fsgs() and call it first. > When FSGSBASE support lands (which will happen eventually!), the code > in your patch will be completely wrong. > > Admittedly, your patch isn't 100% bogus, but the reason is subtle and > you need lots of comments there *and* in save_fsgs(). Thank you Andy, I'll take a look.
Andy Lutomirski <luto@kernel.org> writes: > On Fri, Mar 2, 2018 at 10:55 AM, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >> vmx_save_host_state() is only called from kvm_arch_vcpu_ioctl_run() so >> the context is pretty well defined >> > > True. > >> and MSR_FS_BASE should always be >> equal to current->thread.fsbase. > > Not true. current->thread.fsbase is almost entirely undefined in this > context. What you *could* do is export save_fsgs() and call it first. > When FSGSBASE support lands (which will happen eventually!), the code > in your patch will be completely wrong. > > Admittedly, your patch isn't 100% bogus, but the reason is subtle and > you need lots of comments there *and* in save_fsgs(). Just to make sure I understand the reason, Currently, the only way for processes to change FS/GS base is to call ARCH_SET_FS/GS prctls and these reflect the changes they make in thread.fs/gsbase so *conceptually* reading them is OK now. Now there's so called X86_BUG_NULL_SEG: on Intel CPUs writing '0' to FS/GS selectors zeroes the base (and on AMDs it doesn't). save_fsgs() checks fs/gs selectors and adjusts thread.fs/gsbase accordingly. (de-facto no-issue for my patch as it only touches Intel's VMX but we don't want to rely on vendor, detect_null_seg_behavior() does real check of the behavior). Now FSGSBASE support comes to play. Userspace will start changing FS/GS base without kernel's intervention so we really need to do read if we want to figure out what's there. Luckily, reads are now cheaper thanks to new instructions. So what I think we need to do here is introduce "sync_process_fs_gs()" ('save_fsgs' now) api which we will call from both __switch_to() and KVM's vmx_save_host_state() before reading thread.fs/gsbase.
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index cab6ea1f8be5..ddefb7aeb96d 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -2157,7 +2157,7 @@ static void vmx_save_host_state(struct kvm_vcpu *vcpu) #endif #ifdef CONFIG_X86_64 - vmcs_writel(HOST_FS_BASE, read_msr(MSR_FS_BASE)); + vmcs_writel(HOST_FS_BASE, current->thread.fsbase); vmcs_writel(HOST_GS_BASE, read_msr(MSR_GS_BASE)); #else vmcs_writel(HOST_FS_BASE, segment_base(vmx->host_state.fs_sel));
vmx_save_host_state() is only called from kvm_arch_vcpu_ioctl_run() so the context is pretty well defined and MSR_FS_BASE should always be equal to current->thread.fsbase. Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- arch/x86/kvm/vmx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)