linux-kernel - Re: [RFC 3/3] arch/x86/kvm: SVM: Introduce pause loop exit logic in SVM

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]

Date:   Wed, 14 Mar 2018 14:26:14 +0100
From:   Radim Krčmář <rkrcmar@...hat.com>
To:     "Moger, Babu" <Babu.Moger@....com>
Cc:     "joro@...tes.org" <joro@...tes.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "hpa@...or.com" <hpa@...or.com>, "x86@...nel.org" <x86@...nel.org>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC 3/3] arch/x86/kvm: SVM: Introduce pause loop exit logic in
 SVM

2018-03-10 05:07+0000, Moger, Babu:
> Radim,
>  Thanks for the comments. Taken care of most of the comments.
>  I have few questions/comments. Please see inline.
> 
> > -----Original Message-----
> > From: Radim Krčmář <rkrcmar@...hat.com>
> > Sent: Friday, March 9, 2018 12:13 PM
> > To: Moger, Babu <Babu.Moger@....com>
> > Cc: joro@...tes.org; tglx@...utronix.de; mingo@...hat.com;
> > hpa@...or.com; x86@...nel.org; pbonzini@...hat.com;
> > kvm@...r.kernel.org; linux-kernel@...r.kernel.org
> > Subject: Re: [RFC 3/3] arch/x86/kvm: SVM: Introduce pause loop exit logic in
> > SVM
> > 
> > 2018-03-02 11:17-0500, Babu Moger:
> > > Bring the PLE(pause loop exit) logic to AMD svm driver.
> > > We have noticed it help in situations where numerous pauses are
> > generated
> > > due to spinlock or other scenarios. Tested it with idle=poll and noticed
> > > pause interceptions go down considerably.
> > >
> > > Signed-off-by: Babu Moger <babu.moger@....com>
> > > ---
> > > @@ -1046,6 +1095,58 @@ static int avic_ga_log_notifier(u32 ga_tag)
> > >  	return 0;
> > >  }
> > >
> > > +static void grow_ple_window(struct kvm_vcpu *vcpu)
> > > +{
> > > +	struct vcpu_svm *svm = to_svm(vcpu);
> > > +	struct vmcb_control_area *control = &svm->vmcb->control;
> > > +	int old = control->pause_filter_count;
> > > +
> > > +	control->pause_filter_count = __grow_ple_window(old,
> > > +							pause_filter_count,
> > > +							ple_window_grow,
> > > +
> > 	ple_window_actual_max);
> > > +
> > > +	if (control->pause_filter_count != old)
> > > +		mark_dirty(svm->vmcb, VMCB_INTERCEPTS);
> > > +
> > > +	trace_kvm_ple_window_grow(vcpu->vcpu_id,
> > > +				  control->pause_filter_count, old);
> > 
> > Please move the tracing into __shrink_ple_window to share the code.
> > This probably belongs to patch [2/3].
> 
> I will have to pass vcpu_id, and have to make few changes to display old and new values.
> I am afraid it might add few more extra instructions.

Right, vcpu_id isn't available in that function.
Keeping it like this is ok.

> > 
> > > +/*
> > > + * ple_window_actual_max is computed to be one grow_ple_window()
> > below
> > > + * ple_window_max. (See __grow_ple_window for the reason.)
> > > + * This prevents overflows, because ple_window_max is int.
> > > + * ple_window_max effectively rounded down to a multiple of
> > ple_window_grow in
> > > + * this process.
> > > + * ple_window_max is also prevented from setting control-
> > >pause_filter_count <
> > > + * pause_filter_count.
> > > + */
> > > +static void update_ple_window_actual_max(void)
> > > +{
> > > +	ple_window_actual_max =
> > > +		__shrink_ple_window(max(ple_window_max,
> > pause_filter_count),
> > 
> > (I have no idea what I was thinking when I wrote that for VMX. :[
> >  I'll write a patch to get rid of ple_window_actual_max, because its
> >  benefits are really minuscule and the logic is complicated.)
> 
> If you are thinking of just straight forward removal, I can take care of it.

And tweaking the overflow handling to account for that.  Go ahead if
you'd like to.

> > 
> > > +				    pause_filter_count,
> > > +				    ple_window_grow, SHRT_MIN);
> > > +}
> > >  static __init int svm_hardware_setup(void)
> > >  {
> > >  	int cpu;
> > > @@ -1309,7 +1412,11 @@ static void init_vmcb(struct vcpu_svm *svm)
> > >  	svm->vcpu.arch.hflags = 0;
> > >
> > >  	if (boot_cpu_has(X86_FEATURE_PAUSEFILTER)) {
> > > -		control->pause_filter_count = 3000;
> > > +		control->pause_filter_count = pause_filter_count;
> > > +		if (boot_cpu_has(X86_FEATURE_PFTHRESHOLD))
> > > +			control->pause_filter_thresh = pause_filter_thresh;
> > > +		else
> > > +			pause_filter_thresh = 0;
> > 
> > Please move this to hardware_setup and also clear pause_filter_count if
> 
> Moving this to hardware_setup will be a problem.  We don't have access to svm data structure in hardware_setup.

I mean just the pause_filter_thresh = 0 and pause_filter_count = 0 logic
based on boot_cpu_has (it's weird if the user-visible parameters are
corrected after starting a VM);  VMCB configuration stays,

thanks.