Date: Tue, 25 Apr 2017 20:24:43 +0200
From: Borislav Petkov <bp@alien8.de>
To: Juergen Gross <jgross@suse.com>
Cc: linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org,
        x86@kernel.org, boris.ostrovsky@oracle.com, hpa@zytor.com,
        tglx@linutronix.de, mingo@redhat.com
Subject: Re: [PATCH] x86/amd: don't set X86_BUG_SYSRET_SS_ATTRS if forced to
 zero
Message-ID: <20170425182443.3ab75tkfosol2yk4@pd.tnic>
References: <20170425180014.7533-1-jgross@suse.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20170425180014.7533-1-jgross@suse.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1885
Lines: 51

On Tue, Apr 25, 2017 at 08:00:14PM +0200, Juergen Gross wrote:
> When running as Xen pv guest X86_BUG_SYSRET_SS_ATTRS must not be set
> on AMD cpus. Xen will disable this via setup_clear_cpu_cap(), so test
> cpu_caps_cleared to not have disabled this bit.
> 
> This bug/feature bit is kind of special as it will be used very early
> when switching threads. Setting the bit and clearing it a little bit
> later leaves a critical window where things can go wrong. This time
> window has enlarged a little bit by using setup_clear_cpu_cap() instead
> of the hypervisor's set_cpu_features callback. It seems this larger
> window now makes it rather easy to hit the problem.
> 
> The proper solution is to never set the bit in case of Xen.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  arch/x86/kernel/cpu/amd.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index c36140d788fe..f659b6f534b7 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -800,7 +800,9 @@ static void init_amd(struct cpuinfo_x86 *c)
>  			set_cpu_cap(c, X86_FEATURE_3DNOWPREFETCH);
>  
>  	/* AMD CPUs don't reset SS attributes on SYSRET */
> -	set_cpu_bug(c, X86_BUG_SYSRET_SS_ATTRS);
> +	if (!test_bit(X86_BUG_SYSRET_SS_ATTRS,
> +		      (unsigned long *)cpu_caps_cleared))
> +		set_cpu_bug(c, X86_BUG_SYSRET_SS_ATTRS);
>  }

Hold on, AFAICT we have this order:

c_init() -> init_amd() sets X86_BUG_SYSRET_SS_ATTRS
init_hypervisor->x86_hyper->set_cpu_features-> clear_cpu_bug(c, X86_BUG_SYSRET_SS_ATTRS);

and all is good.

And I remember seeing a patchset doing some xen cpuid cleanup so I'm
assuming you're doing setup_clear_cpu_cap() now? And now we have to wag
the dog?

Confused.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.