lkml.org 
[lkml]   [2020]   [Dec]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] KVM: SVM: use vmsave/vmload for saving/restoring additional host state
On Thu, Dec 10, 2020 at 9:52 AM Michael Roth <michael.roth@amd.com> wrote:
> MSR_STAR, MSR_LSTAR, MSR_CSTAR,
> MSR_SYSCALL_MASK, MSR_KERNEL_GS_BASE,
> MSR_IA32_SYSENTER_CS,
> MSR_IA32_SYSENTER_ESP,
> MSR_IA32_SYSENTER_EIP,
> MSR_FS_BASE, MSR_GS_BASE

Can you get rid of all the old FS/GS manipulation at the same time?

> + for (i = 0; i < NR_HOST_SAVE_USER_MSRS; i++) {
> + rdmsrl(host_save_user_msrs[i], svm->host_user_msrs[i]);
> + }
> +
> + asm volatile(__ex("vmsave")
> + : : "a" (page_to_pfn(sd->save_area) << PAGE_SHIFT)
> + : "memory");
> + /*
> + * Host FS/GS segment registers might be restored soon after
> + * vmexit, prior to vmload of host save area. Even though this
> + * state is now saved in the host's save area, we cannot use
> + * per-cpu accesses until these registers are restored, so we
> + * store a copy in the VCPU struct to make sure they are
> + * accessible.
> + */
> #ifdef CONFIG_X86_64
> - rdmsrl(MSR_GS_BASE, to_svm(vcpu)->host.gs_base);
> + svm->host.gs_base = hostsa->gs.base;
> #endif

For example, this comment makes no sense to me. Just let VMLOAD
restore FS/GS and be done with it. Don't copy those gs_base and
gs.base fields -- just delete them please. (Or are they needed for
nested virt for some reason? If so, please document that.)

> - savesegment(fs, svm->host.fs);
> - savesegment(gs, svm->host.gs);
> - svm->host.ldt = kvm_read_ldt();
> -
> - for (i = 0; i < NR_HOST_SAVE_USER_MSRS; i++)
> - rdmsrl(host_save_user_msrs[i], svm->host_user_msrs[i]);
> + svm->host.fs = hostsa->fs.selector;
> + svm->host.gs = hostsa->gs.selector;

This too. Why is the host code thinking about selectors at all?

> - kvm_load_ldt(svm->host.ldt);

I have a patch that deletes this, too. Don't worry about the conflict
-- I'll sort it out.

> @@ -120,7 +115,6 @@ struct vcpu_svm {
> struct {
> u16 fs;
> u16 gs;
> - u16 ldt;
> u64 gs_base;
> } host;

Shouldn't you be about to delete fs, gs, and gs_base too?

\
 
 \ /
  Last update: 2020-12-10 20:26    [W:0.194 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site