linux-kernel - RE: [PATCH v2 09/25] KVM: VMX: Switch FRED RSP0 between host and guest

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <SA1PR11MB67348CCC02DE80B2DAA4C7A0A8A32@SA1PR11MB6734.namprd11.prod.outlook.com>
Date: Wed, 17 Jul 2024 17:31:45 +0000
From: "Li, Xin3" <xin3.li@...el.com>
To: "Li, Xin3" <xin3.li@...el.com>, Sean Christopherson <seanjc@...gle.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>, "linux-doc@...r.kernel.org"
	<linux-doc@...r.kernel.org>, "linux-kselftest@...r.kernel.org"
	<linux-kselftest@...r.kernel.org>, "pbonzini@...hat.com"
	<pbonzini@...hat.com>, "corbet@....net" <corbet@....net>,
	"tglx@...utronix.de" <tglx@...utronix.de>, "mingo@...hat.com"
	<mingo@...hat.com>, "bp@...en8.de" <bp@...en8.de>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>, "x86@...nel.org"
	<x86@...nel.org>, "hpa@...or.com" <hpa@...or.com>, "shuah@...nel.org"
	<shuah@...nel.org>, "vkuznets@...hat.com" <vkuznets@...hat.com>,
	"peterz@...radead.org" <peterz@...radead.org>, "Shankar, Ravi V"
	<ravi.v.shankar@...el.com>, "xin@...or.com" <xin@...or.com>
Subject: RE: [PATCH v2 09/25] KVM: VMX: Switch FRED RSP0 between host and
 guest

> > > > E.g. if it's somewhere in task_struct, then kvm_on_user_return()
> > > > would restore the current task's desired RSP0.
> > >
> > > So you're suggesting to extend the framework to allow per task constants?
> >
> > Yeah, or more likely, special case MSR_IA32_FRED_RSP0.  If KVM didn't
> > already have the user return framework, I wouldn't suggest this as I
> > doubt avoiding WRMSR when switching between vCPU tasks will be very
> > meaningful, but it's easy to handle FRED_RSP0, so why not.
> 
> Great, I will take the patch.

I tried to make this work, however because FRED RSP0 is per task and
keeps changing during context switch[1], we lose track of FRED RSP0
values from both host and guest, thus we need to:

1) *always* save guest FRED RSP0 in vmx_prepare_switch_to_host().

2) *always* restore guest FRED RSP0 in vmx_prepare_switch_to_guest(),
   because sometimes context switches happen but the CPU does NOT
   return to user mode thus the user return framework detects no change.

So it essentially becomes the same as what the original patch does.

I guess It's probably not worth the change, how do you think?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9356c4b8886c4f7d3436c3f7fe31715bdcf1c79e


> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index
> > 1783986d8626..ebecb205e5de 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -352,6 +352,7 @@ static void kvm_on_user_return(struct
> > user_return_notifier *urn)
> >                 = container_of(urn, struct kvm_user_return_msrs, urn);
> >         struct kvm_user_return_msr_values *values;
> >         unsigned long flags;
> > +       u64 host_val;
> >
> >         /*
> >          * Disabling irqs at this point since the following code could
> > be @@ -365,9
> > +366,15 @@ static void kvm_on_user_return(struct user_return_notifier
> > +*urn)
> >         local_irq_restore(flags);
> >         for (slot = 0; slot < kvm_nr_uret_msrs; ++slot) {
> >                 values = &msrs->values[slot];
> > -               if (values->host != values->curr) {
> > -                       wrmsrl(kvm_uret_msrs_list[slot], values->host);
> > -                       values->curr = values->host;
> > +
> > +               if (kvm_uret_msrs_list[slot] == MSR_IA32_FRED_RSP0)
> > +                       host_val = get_current_fred_rsp0();
> > +               else
> > +                       host_val = values->host;
> > +
> > +               if (host_val != values->curr) {
> > +                       wrmsrl(kvm_uret_msrs_list[slot], host_val);
> > +                       values->curr = host_val;
> >                 }
> >         }
> >  }