Re: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

From: Paolo Bonzini <pbonzini@redhat.com>
To: Borislav Petkov <bp@alien8.de>, Maxim Levitsky <mlevitsk@redhat.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	Pawan Gupta <pawan.kumar.gupta@linux.intel.com>,
	Ingo Molnar <mingo@redhat.com>,
	Josh Poimboeuf <jpoimboe@kernel.org>,
	Namhyung Kim <namhyung@kernel.org>,
	Tony Luck <tony.luck@intel.com>, "H. Peter Anvin" <hpa@zytor.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Tim Chen <tim.c.chen@linux.intel.com>,
	"David S. Miller" <davem@davemloft.net>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	"Chang S. Bae" <chang.seok.bae@intel.com>,
	Jane Malalane <jane.malalane@citrix.com>,
	Kees Cook <keescook@chromium.org>,
	Kan Liang <kan.liang@linux.intel.com>,
	Peter Zijlstra <peterz@infradead.org>,
	"maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)"
	<x86@kernel.org>, Herbert Xu <herbert@gondor.apana.org.au>,
	Jiri Olsa <jolsa@kernel.org>, Mark Rutland <mark.rutland@arm.com>,
	linux-perf-users@vger.kernel.org,
	"open list:CRYPTO API" <linux-crypto@vger.kernel.org>
Subject: Re: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap
Date: Wed, 2 Nov 2022 14:40:18 +0100	[thread overview]
Message-ID: <fd2cf028-bd83-57ff-7e6d-ef3ee11852a1@redhat.com> (raw)
In-Reply-To: <Y1EOBAaLbv2CXBDL@zn.tnic>

On 10/20/22 10:59, Borislav Petkov wrote:
> On Wed, Sep 28, 2022 at 01:49:34PM +0300, Maxim Levitsky wrote:
>> Patch 5 is the main fix - it makes the kernel to be tolerant to a
>> broken CPUID config (coming hopefully from hypervisor), where you have
>> a feature (AVX2 in my case) but not a feature on which this feature
>> depends (AVX).
> 
> I really really don't like it when people are fixing the wrong thing.
> 
> Why does the kernel need to get fixed when something else can't get its
> CPUID dependencies straight? I don't even want to know why something
> would set AVX2 without AVX?!?!

Users do so because they just "disable AVX" (e.g. in QEMU -cpu 
host,-avx) and that removes the AVX bit.  Userspace didn't bother to 
implement the whole set of CPUID bit dependencies for AVX because:

1) Intel is adding AVX features every other week and probably half the 
time people would forget to add the dependency

2) anyway you absolutely need to check XCR0 before using AVX, which in 
the kernel is done using cpu_has_xfeatures(XFEATURE_MASK_YMM), and 
userspace *does* remove the XSAVE state from 0Dh leaf if you remove AVX.

(2) in particular holds even on bare metal.  The kernel bug here is that 
X86_FEATURE_AVX only tells you if the instructions are _present_, not if 
they are _usable_.   Indeed, the XCR0 check is present for all other 
files in arch/x86/crypto, either instead or in addition to 
boot_cpu_has(X86_FEATURE_AVX).

Maxim had sent a patch about a year ago to do it in aesni-intel-glue.c 
but Dave told him to fix the dependencies instead 
(https://lore.kernel.org/all/20211103124614.499580-1-mlevitsk@redhat.com/). 
  What do you think of applying that patch instead?

Thanks,

Paolo